Re: [PATCH v4] commit: add a commit.verbose config variable

> On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> 
> wrote:
>
>>     Add commit.verbose configuration variable as a convenience
>>     for those who always prefer --verbose.
>>
>> or something.
>
> Sure!
>
>> As a convenience to reviewers, please use this area below the "---"
>> line to provide links and explain what changed since the previous
>> round rather than doing so in a separate email.
>
> Actually I am sending the patches with submitGit herokuapp because my
> institute proxy does not allow IMAP/POP3 connections.

>
>> The "permanently" bit sounds scary. A more concise way to state this 
>> might be:
>>
>>     See the `commit.verbose` configuration variable in
>>     linkgit:git-config[1].
>>
>> which doesn't bother spelling out what the intelligent reader should
>> infer from the reference.
>> Style: space before {
>
> Sure!
>
>>> +test_expect_success 'commit with commit.verbose true and no arguments' 
>>> '
>>
>> "no arguments" doesn't convey much; how about "--verbose omitted" or
>> something? Ditto for the titles of other tests.
>
> Will change the language construct.
>>> +       echo content >file &&
>>> +       git add file &&
>>> +       test_config commit.verbose true &&
>>> +       (
>>> +               GIT_EDITOR=cat &&
>>> +               export GIT_EDITOR &&
>>> +               test_must_fail git commit >output
>>> +       ) &&
>>> +       test_i18ngrep "diff --git" output
>>> +'
>>
>> Making git-commit fail unconditionally with "aborting due to empty
>> commit message" is a rather sneaky way to perform this test. I would
>> have expected to see these new tests re-use the existing machinery
>> provided by this script (the check-for-diff "editor") rather than
>> inventing an entirely new and unintuitive mechanism. Doing so would
>> also reduce the size of each new test.
>
> I agree on the fact that making git-commit fail unconditionally is not
> a good way to perform the test. "check-for-diff" is not really an
> "editor" and it checks for the commit message after it has been
> written to the history. The verbose output is stripped when it is
> written to the history so we won't be able to test whether this patch
> works. This is where purposely breaking the code is required as when
> the commit fails, it gives the output of the contents present at that
> time (which will contain the verbose output). More over the
> 'check-for-diff' uses grep which is not preferred. Many tests are now
> using test_i18ngrep (eg. f79ce8db). I had planned on using
> 'check-for-diff' before but it took me some time to figure out this
> behavior and thus I began searching for another mechanism (breaking
> code).
>
>> Some additional tests[1][2] are probably warranted.
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/288648
>> [2]: http://article.gmane.org/gmane.comp.version-control.git/288657
>
> I think these tests also are better included in this file as this
> patch triggers it and it would not make much of a difference between
> t7507 and t7502 but in fact improve its readability.
>>>  test_done