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

[Eric Sunshine <sunshine@sunshineco.com>]

[+cc:Roberto Tyley]

On Fri, Mar 11, 2016 at 05:45:27AM +0530, Pranit Bauva wrote:
> On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.

That's unfortunate. Your separate "cover letter" often arrives hours
later than the patch itself. Perhaps Roberto can comment on submitGit
and per-patch commentary.

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

It's a bit tricky if you're not used to it, but check-for-diff
actually does what you want, and does so in a more direct way. While
it's true that it's not an "editor" per se, it does get access to the
entire block of text that would normally appear in your editor during
an interactive commit. And, this is happening before the commit has
been written to history. So, check-for-diff gets a chance to look at
the full text that would appear in your editor, and can therefore
check if it contains the expected "diff --git" string.

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

'test_i18ngrep' is intended for strings which may be translated,
however, since the expected "diff --git" string should never be
translated, check-for-diff's use of 'grep' is correct, whereas
'test_i18ngrep' would be misleading (if not actively wrong).

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

As an experiment, I rewrote the four new tests in terms of
check-for-diff (with "test_set_editor check-for-diff" already in
effect). Here's what they look like, and they function as expected:

    test_expect_success 'commit.verbose true and --verbose omitted' '
        git -c commit.verbose=true commit --amend
    '

    test_expect_success 'commit.verbose true and --no-verbose' '
        test_must_fail git -c commit.verbose=true commit --amend --no-verbose
    '

    test_expect_success 'commit.verbose false and --verbose' '
        git -c commit.verbose=false commit --amend --verbose
    '

    test_expect_success 'commit.verbose false and --verbose omitted' '
        test_must_fail git -c commit.verbose=false commit --amend
    '

These are modeled after the "initial commit shows verbose diff" test
earlier in the script.