Re: [PATCH v2] builtin/log: honor log.decorate
- Date: Fri, 12 May 2017 19:23:04 -0600
- From: Alex Henrie <alexhenrie24@xxxxxxxxx>
- Subject: Re: [PATCH v2] builtin/log: honor log.decorate
2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnieder@xxxxxxxxx>:
> 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
> 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
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.