Web lists-archives.com

Re: [PATCH v2] builtin/log: honor log.decorate




2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnieder@xxxxxxxxx>:
> Hi,
>
> brian m. carlson wrote:
>
>> Does anyone else have views on whether this is good thing to test for?
>
> I know you don't mean to be rude, but this comes across as a bit of
> a dismissive question.

The question sounded neutral to me.

>> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:
>>> brian m. carlson wrote:
>
>>>> The recent change that introduced autodecorating of refs accidentally
>>>> broke the ability of users to set log.decorate = false to override it.
>>>
>>> Yikes.  It sounds to me like we need a test to ensure we don't regress
>>> it again later.
>>
>> I can add one, but it's going to be a bit odd.  The issue is that as far
>> as I can tell, the option is honored only if it's the last one read, so
>> it necessarily has to be in the per-repository configuration.
>>
>> I'm not sure it makes that much sense to add a test for this case.  Do
>> we generally want to write such tests for all config options?  I don't
>> suppose it's that common a mistake to make.
>
> In my humble opinion, the bug being subtle makes it especially useful
> to have a test that describes that bug.  That way, if someone is
> refactoring this code later then they know what to watch out for not
> reintroducing.
>
> I'm happy to hear other opinions, especially if they come with data or
> a principle attached that I can use when writing future patches of my
> own.

When I saw Brian's email today, my first thought was "What was I
thinking?" My mistake was pretty obvious. Then I remembered that when
I wrote the original patch, I wasn't sure where to set the default
value, because there were no clear examples in this file. Now that
we've established a clear precedent for setting the log.decorate
default (and other defaults like it) in init_log_defaults, I don't
expect any more problems with log.decorate. And since it's not
practical to add tests for similar bugs for every command and
configuration option in Git, we'll just have to be a little more
vigilant about code review.

Again, I apologize for the trouble.

-Alex