Re: [PATCH] blame: add blame.showemail config option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 27, 2015, 19:57 UTC
Message-ID: <CAPig+cRYUng43rxCJExMEs1ti6wp0oangTzwJbbesmsqM3c+tA@mail.gmail.com>
In-Reply-To: <CACfD1vvaAGkx2P3yMfZPOZrRvG3-H96zQVOCevnd-O0rBJ7wjw@mail.gmail.com>

On Mon, Apr 27, 2015 at 9:46 AM, Quentin Neill <quentin.neill@gmail.com> wrote:

Show 15 quoted lines

> On Fri, Apr 24, 2015 at 12:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> It's not clear why you relocated documentation of --show-email from
>> git-blame.txt to blame-options.txt, and the commit message does not
>> explain the move. If there's a good reason for the relocation, the
>> justification should be spelled out so that people reviewing the patch
>> or looking through history later on will not have to guess about it.
>
> I moved it to be with the other variables that had configuration
> options, but I will move it back.
>
>> It might also make sense to do the relocation as a separate
>> preparatory patch of a 2-patch series, in which the patch adding
>> blame.showemail is the second of the two.
>
> If you think it should be relocated, I will address in a separate patch.

Junio's response[1] addresses both points nicely. To be clear, I wasn't suggesting that you should do the relocation, but instead that the relocation seemed unrelated to the overall intent of the patch and that its purpose wasn't clear. So, as a general statement, when the motive for a change is unclear, it deserves explanation in the commit message; and when a change is not directly related to the patch itself, it often deserves to be placed in its own patch. In this case, neither applies since the relocation is unwarranted.

Show 18 quoted lines

>>> -                       if (opt & OUTPUT_SHOW_EMAIL)
>>> +                       if ((opt & OUTPUT_SHOW_EMAIL) || show_email)
>>
>> The desired behavior is for a configuration setting to provide a
>> fallback, and for a command-line option to override the fallback. So,
>> for instance, if blame.showemail is "true", then --no-show-email
>> should countermand that. Unfortunately, the way this patch is
>> implemented, it's impossible to override the setting from the
>> command-line since show_email==true will always win (when
>> blame.showemail is "true").
>>
>> More below.
>
> I followed the code for blame.showRoot and blame.blankboundary.
>
> I think the desired behavior for the other switches would go in a
> separate patch, the question is should it precede this one adding
> 'blame.showemail'?

As per Junio's response[1], logic for the other configuration options seems to be fine, so I'm not quite sure what changes you propose.

Show 7 quoted lines

>> You'll also want to add tests for the new blame.showemail
>> configuration. There's already one test in t8002-blame.sh which checks
>> that --show-email works, but you will want tests to ensure that you
>> get the expected results for all combinations of blame.showemail and
>> --show-email (including when --show-email is not specified).
>
> Agreed, but again I don't see tests for the other switches with options.

Unfortunately, test coverage is sometimes sparse, however, patches with accompanying tests are looked upon with favor and instill greater confidence, so they are encouraged. If you need assistance with the tests, feel free to ask.

It's not your responsibility to fill the gaps of missing tests for other options which you're not touching, but you're welcome to add tests for them if you want to.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/267720/focus=267862