Re: [PATCH v10 3/3] commit: add a commit.verbose config variable

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Mar 27, 2016 at 3:00 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success 'commit.verbose true and --verbose omitted' '
>>> +       echo content >file2 &&
>>> +       echo content >>file &&
>>> +       git add file2 &&
>>> +       git -c commit.verbose=true commit -F message &&
>>> +       test_line_count = 1 out
>>> +'
>>
>> Why is this test so utterly different than it was in v9 (even though
>> the title is the same), and why is it so different from other tests
>> below?
>
> This is because the "editor" in v9 checked for "# Changes"... While
> this "editor" checks for 'diff --git'. And submodules don't give a
> proper diff to verify (I tried this out and noticed this behavior by
> tweaking some parts). In fact submodules don't give diff at all. But
> they do give "# Changes"... So its important to setup up a little
> before getting started. If this seems unnecessary, then should I move
> all the tests which were introduced here above the submodule test?

Let's ignore submodules when discussing this since they don't need to
factor into the issue. What you are actually saying (and what took me
a while to understand due to the "submodules" misdirection) is that
you need to do some additional setup to test the "-v -v" cases. In
particular, you need to introduce some change to the worktree which is
not in the index.

The typical way to satisfy this requirement (which doesn't require
relocating tests) is to add a "setup" test before the tests which
depend upon that additional setup, rather than adding that setup to
the first test which needs it. Just about the simplest setup test
which satisfies your needs is the following (inserted just before the
first of the new tests):

    test_expect_success 'setup -v -v' '
        echo dirty >file
    '

And, then you can restore the "commit.verbose true and --verbose
omitted" test to its simple form:

    test_expect_success 'commit.verbose true and --verbose omitted' '
        git -c commit.verbose=true commit --amend &&
        test_line_count = 1 out
    '


By the way, now that commit.verbose is no longer a mere boolean,
you're going to need some additional tests beyond the
commit.verbose={true,false} ones you've already added. In particular,
you should be testing commit.verbose with several numeric values to
verify that it works as expected. For instance:

    commit.verbose=-2
    commit.verbose=-1
    commit.verbose=0
    commit.verbose=1
    commit.verbose=2
    commit.verbose=3

The -2 case is interesting; I'm pretty sure the current implementation
of this patch will misbehave since the only negative value it's