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

From: Alex Henrie <alexhenrie24@gmail.com>
Date: May 13, 2017, 01:23 UTC
Message-ID: <CAMMLpeRkVsvp_fzt+bHSQ_F9gsMhYqej_oyqrjhAXsY1xs7=Qw@mail.gmail.com>
In-Reply-To: <20170512234814.GG27400@aiede.svl.corp.google.com>

2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnieder@gmail.com>:

Show 8 quoted lines

> 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.

Show 25 quoted lines

>> 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