Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Apr 13, 2016 at 5:15 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success 'status does not verbose without --verbose' '
>>> +       git status >actual &&
>>> +       ! grep "^diff --git" actual
>>> +'
>>
>> But what is this test checking?
>
> status is also a consumer of the verbose whose initial value is set to
> -1. This makes it include verbose in status output. This bug was fixed
> by explicitly initializing verbose to 0 if -1. SZEDER pointed out a
> bug[1] which broke some tests in and then when I fixed it, you
> requested me to include tests even in this patch[2] which I found
> convincing enough.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289730
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289993

Okay, makes sense, but it's not at all obvious from the context of
this patch or its commit message. It probably would have been clearer
had the two git-status tests been added in a separate preparatory test
with a commit message explaining that the tests are to ensure that a
subsequent patch (adding commit.verbose) won't break the existing
behavior of git-status. Having a separate commit explaining that would
also help future readers of the test script who wonder what this test
is doing (since it's not obvious and it's not explained by the current
commit message) when they use git-blame to try to figure out its
purpose. If you do re-roll, you might consider breaking them out to a
new patch or, at the very least, document their purpose in the commit
message of this patch.