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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

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

Thanks,
Jonathan