v10, 3 partscommit: add a commit.verbose config variable

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> Changes with respect to the previous patch:
>  - Fixed a status related bug pointed out by SZEDER
>  - Change some tests

> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>         finalize_colopts(&s.colopts, -1);
>         finalize_deferred_config(&s);
>
> +       if (verbose == -1)
> +               verbose = 0;
> +

>         handle_untracked_files_arg(&s);
>         if (show_ignored_in_status)
>                 s.show_ignored_files = 1;
> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>         argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>                                           builtin_commit_usage,
>                                           prefix, current_head, &s);
> +
> +       if (verbose == -1)
> +               verbose = (config_verbose == -1) ? 0 : config_verbose;
> +

>         if (dry_run)
>                 return dry_run_commit(argc, argv, prefix, current_head, &s);
>         index_file = prepare_index(argc, argv, prefix, current_head, 0);
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
> +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
> +'

> +test_expect_success 'commit.verbose true and --verbose' '
> +       git -c commit.verbose=true commit --amend --verbose &&
> +       test_line_count = 1 out
> +'
> +
> +test_expect_success 'commit.verbose true and -v -v' '
> +       git -c commit.verbose=true commit --amend -v -v &&
> +       test_line_count = 2 out
> +'
> +
> +test_expect_success 'commit.verbose true and --no-verbose' '
> +       git -c commit.verbose=true commit --amend --no-verbose &&
> +       ! test -s out
> +'
> +
> +test_expect_success 'commit.verbose false and --verbose' '
> +       git -c commit.verbose=false commit --amend --verbose &&
> +       test_line_count = 1 out
> +'
> +
> +test_expect_success 'commit.verbose false and -v -v' '
> +       git -c commit.verbose=false commit --amend -v -v &&
> +       test_line_count = 2 out
> +'
> +
> +test_expect_success 'commit.verbose false and --verbose omitted' '
> +       git -c commit.verbose=false commit --amend &&
> +       ! test -s out
> +'
> +
> +test_expect_success 'commit.verbose false and --no-verbose' '
> +       git -c commit.verbose=false commit --amend --no-verbose &&
> +       ! test -s out
> +'
> +
> +test_expect_success 'status ignores commit.verbose=true' '
> +       git -c commit.verbose=true status >actual &&
> +       ! grep "^diff --git" actual
> +'

> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose taking care of multiple levels of verbosity.
>
> What does "taking care of multiple levels of verbosity" mean? I
> suppose you mean that commit.verbose specifies a verbosity level
> (rather than being merely boolean), but that phrase is difficult to
> decipher. And, since it's obvious from the patch itself that verbosity
> is not a mere boolean, there isn't really a need to mention it here.
> The commit message would be perfectly fine without that bit, so
> perhaps just drop it.

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> ---
>> Changes with respect to the previous patch:
>>  - Fixed a status related bug pointed out by SZEDER
>>  - Change some tests
>
> Please help reviewers out by being a bit more verbose when describing
> the changes. For instance, what bug did you fix pointed out by SZEDER?
> And, "change some tests" says nothing useful. What did you change in
> the tests?

>> ---
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>>         finalize_colopts(&s.colopts, -1);
>>         finalize_deferred_config(&s);
>>
>> +       if (verbose == -1)
>> +               verbose = 0;
>> +
>
> Nit: It might be good to drop the blank line above this new
> conditional to make it clear that it is part of the
> init_config/parse_options processing (the tail of which is visible in
> the context above).

>>         handle_untracked_files_arg(&s);
>>         if (show_ignored_in_status)
>>                 s.show_ignored_files = 1;
>> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>         argc = parse_and_validate_options(argc, argv, builtin_commit_options,
>>                                           builtin_commit_usage,
>>                                           prefix, current_head, &s);
>> +
>> +       if (verbose == -1)
>> +               verbose = (config_verbose == -1) ? 0 : config_verbose;
>> +
>
> Nit: For consistency, dropping the blank line above this new
> conditional wouldn't hurt either.
>
>>         if (dry_run)
>>                 return dry_run_commit(argc, argv, prefix, current_head, &s);
>>         index_file = prepare_index(argc, argv, prefix, current_head, 0);
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
>> +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?

> More below...
>
>> +test_expect_success 'commit.verbose true and --verbose' '
>> +       git -c commit.verbose=true commit --amend --verbose &&
>> +       test_line_count = 1 out
>> +'
>> +
>> +test_expect_success 'commit.verbose true and -v -v' '
>> +       git -c commit.verbose=true commit --amend -v -v &&
>> +       test_line_count = 2 out
>> +'
>> +
>> +test_expect_success 'commit.verbose true and --no-verbose' '
>> +       git -c commit.verbose=true commit --amend --no-verbose &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose' '
>> +       git -c commit.verbose=false commit --amend --verbose &&
>> +       test_line_count = 1 out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and -v -v' '
>> +       git -c commit.verbose=false commit --amend -v -v &&
>> +       test_line_count = 2 out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --verbose omitted' '
>> +       git -c commit.verbose=false commit --amend &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'commit.verbose false and --no-verbose' '
>> +       git -c commit.verbose=false commit --amend --no-verbose &&
>> +       ! test -s out
>> +'
>> +
>> +test_expect_success 'status ignores commit.verbose=true' '
>> +       git -c commit.verbose=true status >actual &&
>> +       ! grep "^diff --git" actual
>> +'
>
> The fact that v9 broke a number of tests in other scripts which use
> git-status (as SZEDER pointed out[1]), due to initializing 'verbose'
> to -1 in builtin/commit.c, suggests that you probably ought to add
> another test here to protect against that sort of breakage, as well.
> Specifically, git-status should remain non-verbose when neither
> commit.verbose nor --verbose is specified.

> 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?

> 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
> expecting 'config_verbose' to be is -1.

> +'
> +
> +test_expect_success 'commit.verbose false and --verbose' '
> +	git -c commit.verbose=false commit --amend --verbose &&
> +	test_line_count = 1 out
> +'
> +
> +test_expect_success 'commit.verbose false and -v -v' '
> +	git -c commit.verbose=false commit --amend -v -v &&
> +	test_line_count = 2 out
> +'
> +
> +test_expect_success 'commit.verbose false and --verbose omitted' '
> +	git -c commit.verbose=false commit --amend &&
> +	! test -s out
> +'
> +
> +test_expect_success 'commit.verbose false and --no-verbose' '
> +	git -c commit.verbose=false commit --amend --no-verbose &&
> +	! test -s out
> +'
> +
> +test_expect_success 'status ignores commit.verbose=true' '
> +	git -c commit.verbose=true status >actual &&
> +	! grep "^diff --git" actual
> +'
> +
>  test_done
> 
> --
> https://github.com/git/git/pull/218
> 
> 

>> +test_expect_success 'commit.verbose true and --no-verbose' '
>> +     git -c commit.verbose=true commit --amend --no-verbose &&
>> +     ! test -s out
>
> Please use the test_must_be_empty helper instead, because it has a
> nice, human-readable name and it complains with a helpful error
> message if something goes wrong, whereas 'test -s' just fails
> silently.

> On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> +test_expect_success 'commit.verbose true and --no-verbose' '
>>> +     git -c commit.verbose=true commit --amend --no-verbose &&
>>> +     ! test -s out
>>
>> Please use the test_must_be_empty helper instead, because it has a
>> nice, human-readable name and it complains with a helpful error
>> message if something goes wrong, whereas 'test -s' just fails
>> silently.
>
> Thanks for pointing it out. I was unsure whether 'test -s' is a good
> choice but used it since I did not know any other alternative.

> On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> +test_expect_success 'commit.verbose true and --no-verbose' '
>>> +     git -c commit.verbose=true commit --amend --no-verbose &&
>>> +     ! test -s out
>>
>> Please use the test_must_be_empty helper instead, because it has a
>> nice, human-readable name and it complains with a helpful error
>> message if something goes wrong, whereas 'test -s' just fails
>> silently.
>
> Thanks for pointing it out. I was unsure whether 'test -s' is a good
> choice but used it since I did not know any other alternative.

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -3,9 +3,11 @@
> -cat >check-for-diff <<EOF
> -#!$SHELL_PATH
> -exec grep '^diff --git' "\$1"
> +write_script "check-for-diff" <<'EOF' &&

>  test_set_editor "$PWD/check-for-diff"
> @@ -23,7 +25,8 @@ test_expect_success 'setup' '
>  test_expect_success 'initial commit shows verbose diff' '
> -       git commit --amend -v
> +       git commit --amend -v &&
> +       test_line_count = 1 out
>  '
>
>  test_expect_success 'second commit' '
> @@ -39,13 +42,15 @@ check_message() {
>
>  test_expect_success 'verbose diff is stripped out' '
>         git commit --amend -v &&
> -       check_message message
> +       check_message message &&
> +       test_line_count = 1 out
>  '
>
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>         git config diff.mnemonicprefix true &&
>         git commit --amend -v &&
> -       check_message message
> +       check_message message &&
> +       test_line_count = 1 out
>  '

> > +! test -s out ||
> > +rm out &&
> 
> Why not just "rm -f out"? But, more importantly, why do you need to
> remove the file at all? The '>' redirection operator (used below) will
> overwrite the file, so no need to remove it beforehand.
> 
> > +! grep '^diff --git' "$1" ||
> > +grep '^diff --git' "$1" >out
> 
> Um, what? Why two greps? I would have expected you to simply re-use
> the existing grep (minus the backslash) while adding the redirection:
> 
>     -exec grep '^diff --git' "\$1"
>     +exec grep '^diff --git' "$1" >out
> 
> Am I missing something obvious?

>> > +! test -s out ||
>> > +rm out &&
>>
>> Why not just "rm -f out"? But, more importantly, why do you need to
>> remove the file at all? The '>' redirection operator (used below) will
>> overwrite the file, so no need to remove it beforehand.
>>
>> > +! grep '^diff --git' "$1" ||
>> > +grep '^diff --git' "$1" >out
>>
>> Um, what? Why two greps? I would have expected you to simply re-use
>> the existing grep (minus the backslash) while adding the redirection:
>>
>>     -exec grep '^diff --git' "\$1"
>>     +exec grep '^diff --git' "$1" >out
>>
>> Am I missing something obvious?
>
> In the non-verbose cases no diff is included in the commit message
> template, thus the pattern looking for it doesn't match anything, grep
> exits with error code, which in turn becomes the editor's exit
> code, ultimately making 'git commit' fail.  Not good.
>
> I suppose both the explicit 'rm out' and the '! grep ... || ...' is
> there to deal with this situation.

> Hmph.
>
> I think we could:
>
>   - either revive the idea of two editor scripts: one for the
>     non-verbose case checking with '! grep ...' that there are no
>     diffs in the commit message template, and one for all verbose
>     cases storing those diff lines in a file to be counted later.
>
>   - or use a fake editor that merely copies the whole commit message
>     template to a separate file, and we do the greping in the tests
>     themselves as well.
>
>   - or simply stick a 'true' at the end of the editor script ensuring
>     that it returns success even when grep can't find the pattern, but
>     I kind of feel ashamed of myself for even mentioning this
>     possibility ;)
>
> I would go for the second possibility, but don't feel strong about it.

> > > +! test -s out ||
> > > +rm out &&
> > 
> > Why not just "rm -f out"? But, more importantly, why do you need to
> > remove the file at all? The '>' redirection operator (used below) will
> > overwrite the file, so no need to remove it beforehand.
> > 
> > > +! grep '^diff --git' "$1" ||
> > > +grep '^diff --git' "$1" >out
> > 
> > Um, what? Why two greps? I would have expected you to simply re-use
> > the existing grep (minus the backslash) while adding the redirection:
> > 
> >     -exec grep '^diff --git' "\$1"
> >     +exec grep '^diff --git' "$1" >out
> > 
> > Am I missing something obvious?
> 
> In the non-verbose cases no diff is included in the commit message
> template, thus the pattern looking for it doesn't match anything, grep
> exits with error code, which in turn becomes the editor's exit
> code, ultimately making 'git commit' fail.  Not good.
> 
> I suppose both the explicit 'rm out' and the '! grep ... || ...' is
> there to deal with this situation.

> I think we could:
> 
>   - either revive the idea of two editor scripts: one for the
>     non-verbose case checking with '! grep ...' that there are no
>     diffs in the commit message template, and one for all verbose
>     cases storing those diff lines in a file to be counted later.
> 
>   - or use a fake editor that merely copies the whole commit message
>     template to a separate file, and we do the greping in the tests
>     themselves as well.
> 
>   - or simply stick a 'true' at the end of the editor script ensuring
>     that it returns success even when grep can't find the pattern, but
>     I kind of feel ashamed of myself for even mentioning this
>     possibility ;)
> 
> I would go for the second possibility, but don't feel strong about it.

> On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote:
>> > > +! test -s out ||
>> > > +rm out &&
>> >
>> > Why not just "rm -f out"? But, more importantly, why do you need to
>> > remove the file at all? The '>' redirection operator (used below) will
>> > overwrite the file, so no need to remove it beforehand.
>> >
>> > > +! grep '^diff --git' "$1" ||
>> > > +grep '^diff --git' "$1" >out
>> >
>> > Um, what? Why two greps? I would have expected you to simply re-use
>> > the existing grep (minus the backslash) while adding the redirection:
>> >
>> >     -exec grep '^diff --git' "\$1"
>> >     +exec grep '^diff --git' "$1" >out
>> >
>> > Am I missing something obvious?
>>
>> In the non-verbose cases no diff is included in the commit message
>> template, thus the pattern looking for it doesn't match anything, grep
>> exits with error code, which in turn becomes the editor's exit
>> code, ultimately making 'git commit' fail.  Not good.
>>
>> I suppose both the explicit 'rm out' and the '! grep ... || ...' is
>> there to deal with this situation.
>
> Sure, I understand that, but that's not what I meant. What I wanted
> to know was why Pranit didn't just use the simpler:
>
>     grep '^diff --git' "$1" >out
>     exit 0
>
> and then, in tests:
>
>     test_line_count = n out
>
> where 'n' is 0, 1, or 2 as expected by the test.

>
> Unfortunately, you missed the rest of the discussion since Pranit
> (presumably) accidentally dropped the mailing list when he replied,
> and I didn't notice the omission when replying to him with the above
> suggestion, which would have saved you the bother of going through
> this, as well.

>> I think we could:
>>
>>   - either revive the idea of two editor scripts: one for the
>>     non-verbose case checking with '! grep ...' that there are no
>>     diffs in the commit message template, and one for all verbose
>>     cases storing those diff lines in a file to be counted later.
>>
>>   - or use a fake editor that merely copies the whole commit message
>>     template to a separate file, and we do the greping in the tests
>>     themselves as well.
>>
>>   - or simply stick a 'true' at the end of the editor script ensuring
>>     that it returns success even when grep can't find the pattern, but
>>     I kind of feel ashamed of myself for even mentioning this
>>     possibility ;)
>>
>> I would go for the second possibility, but don't feel strong about it.
>
> Your #3 is effectively what I had suggested, as well (which is
> reproduced above). I had already made this change locally along with
> some other changes I suggested in other responses, and those changes
> look like this (atop Pranit's two patches), which isn't too bad:
>
> --- 8< ---
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 569fd8b..ea26b57 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -4,12 +4,9 @@ test_description='verbose commit template'
>  . ./test-lib.sh
>
>  write_script "check-for-diff" <<'EOF' &&
> -! test -s out ||
> -rm out &&
> -! grep '^diff --git' "$1" ||
>  grep '^diff --git' "$1" >out
> +exit 0
>  EOF
> -chmod +x check-for-diff
>  test_set_editor "$PWD/check-for-diff"
>
>  cat >message <<'EOF'
> @@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
>         test_i18ngrep "Aborting commit due to empty commit message." err
>  '
>
> +test_expect_success 'setup -v -v' '
> +       echo dirty >file
> +'
> +
>  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 &&
> +       git -c commit.verbose=true commit --amend &&
>         test_line_count = 1 out
>  '
>
> @@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' '
>
>  test_expect_success 'commit.verbose true and --no-verbose' '
>         git -c commit.verbose=true commit --amend --no-verbose &&
> -       ! test -s out
> +       test_line_count = 0 out
>  '
>
>  test_expect_success 'commit.verbose false and --verbose' '
> @@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' '
>
>  test_expect_success 'commit.verbose false and --verbose omitted' '
>         git -c commit.verbose=false commit --amend &&
> -       ! test -s out
> +       test_line_count = 0 out
>  '
>
>  test_expect_success 'commit.verbose false and --no-verbose' '
>         git -c commit.verbose=false commit --amend --no-verbose &&
> -       ! test -s out
> +       test_line_count = 0 out
>  '
>
>  test_expect_success 'status ignores commit.verbose=true' '
> --

> Many uses of COUNTUP have now been replaced with BOOL and what remains
> are verbose/quiet/force. This change will not affect existing users of
> COUNTUP at all, as long as they use the initial value of 0 (or more), as
> there is no mechanism to decrement. The only thing the command line can
> do is to reset it to zero with "--no-foo".

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> The discussion about this patch:
> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027
>
> Previous version of the patch:
> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061

> ---
> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> @@ -144,8 +144,12 @@ There are some macros to easily define options:
>
>  `OPT_COUNTUP(short, long, &int_var, description)`::
>         Introduce a count-up option.
> -       `int_var` is incremented on each use of `--option`, and
> -       reset to zero with `--no-option`.
> +       Each use of `--option` increments `int_var`, starting from zero
> +       (even if initially negative), and `--no-option` resets it to
> +       zero. To determine if `--option` or `--no-option` was set at
> +       all, set `int_var` to a negative value, and if it is still

> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> parse-options.c: make OPTION__COUNTUP understand "unspecified" values
>
> A bit clearer: s/understand/respect/
> Also: s/__/_/

>> The reason to make it understand negative values or more specifically
>> "unspecified" values is to give the ability to differentiate whether
>> `--option` or `--no-option` was specified at all.
>
> The word "negative" shows up far too early in this paragraph, even
> before "unspecified". It also fails utterly to explain what
> "unspecified" means and how to use it. It does vaguely explain why
> respecting "unspecified" is desirable (to know if --[no-]option was
> seen), but otherwise leaves the reader quite clueless.
>
>> Many uses of COUNTUP have now been replaced with BOOL and what remains
>> are verbose/quiet/force. This change will not affect existing users of
>> COUNTUP at all, as long as they use the initial value of 0 (or more), as
>> there is no mechanism to decrement. The only thing the command line can
>> do is to reset it to zero with "--no-foo".
>
> Copying this paragraph verbatim from Junio's email[1] isn't
> necessarily the best way to compose a commit message. Junio was
> "thinking out loud" while justifying the change to himself as being
> safe, but you ought to reframe this reasoning into a more concise form
> which flows with the rest of the commit message, keeping the important
> bits and dropping others.

>> Verbose or quiet don't use negative values before this commit but force
>> uses it in a different way to handle its config and then munges the "-1"
>> into 0 before we get to parse_options.
>
> Presumably, you're talking about a very specific instance of -1 in
> conjunction with OPT__FORCE in builtin/clean.c, but that information
> is entirely missing from this paragraph, thus this explanation serves
> only to confuse rather than clarify. It is a good idea to cite this
> specific unusual case when justifying that this change is safe, but it
> must be accompanied by sufficient context for the reader to understand
> what is being said.

>> There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c
>> and they are both fine.
>
> What does "fine" mean? I know from reading the code that "fine" means
> that these clients initialize the values to 0, thus should see no
> behavioral difference. But why are these two cases called out
> specially when they are already covered by the above "as long as they
> use initial value of 0" explanation? As a reader, having these two
> cases mentioned specially makes me wonder if I'm missing something
> non-obvious about them.

> It would probably be better to avoid mentioning cmd_hash_object() and
> test-parse-options.c at all, and instead explain generally that, with
> one exception (builtin/clean.c), all current clients of OPTION_COUNTUP
> use an initial value of zero, thus are not impacted by this change.
> And, then go on to explain builtin/clean.c's special case and why it
> is safe, as well.
>
>
> Sorry for seeming to be very picky, but if I hadn't already been
> following this topic (and hadn't in fact suggested this particular
> change), as a reader, I think I would find this commit message utterly
> baffling, and wouldn't have a clue what this change is about or why it
> is desirable.
>
> Perhaps it would be a good idea to re-read "(2) Describe your changes
> well" in Documentation/SubmittingPatches for hints about writing a
> good commit message, as well as Junio's recent re-stating[2] of those
> hints.

> Try to write the commit message as if you were speaking to someone who
> wasn't already familiar with the issue you're trying to solve.
> Specifically, explain the problem ("no way to distinguish between
> --[no-]option being seen or not"); explain the solution ("introduce an
> 'unspecified' value"); the implementation of the "unspecified" value
> ("any negative number" plus an example of how to make use of it);
> justify that the change is safe ("existing clients of OPTION_COUNTUP
> are not impacted because ...").

>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> The discussion about this patch:
>> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027
>>
>> Previous version of the patch:
>> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061
>
> Unless I'm mistaken, the previous version was v9[3], not v1.
>
>> Changes introduced:
>> Use a different language in commit message to make the change and its
>> utility more clear. Also added some points as to where this patch could
>> break but it doesn't.
>
> This version forgets to add the new tests to t0040-parse-options.sh
> which SZEDER requested[4] to ensure that the new behavior works as
> expected.
>
>> ---
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,12 @@ There are some macros to easily define options:
>>
>>  `OPT_COUNTUP(short, long, &int_var, description)`::
>>         Introduce a count-up option.
>> -       `int_var` is incremented on each use of `--option`, and
>> -       reset to zero with `--no-option`.
>> +       Each use of `--option` increments `int_var`, starting from zero
>> +       (even if initially negative), and `--no-option` resets it to
>> +       zero. To determine if `--option` or `--no-option` was set at
>> +       all, set `int_var` to a negative value, and if it is still
>
> I realize that you copied this text verbatim from the example I
> gave[5], but in retrospect, I think s/set/initialize/ would be a bit
> more clear:
>
>     all, initialize `int_var` to a negative value, ...
>
>> +       negative after parse_options(), then neither `--option` nor
>> +       `--no-option` was seen.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289264/
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289757/
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724
> [4]: http://article.gmane.org/gmane.comp.version-control.git/289733
> [5]: http://article.gmane.org/gmane.comp.version-control.git/289822

> Make the fake "editor" store output of grep in a file so that we can
> see how many diffs were contained in the message and use them in
> individual tests where ever it is required. Also use write_script()
> to create the fake "editor".
>
> A subsequent commit will introduce scenarios where it is important to be
> able to exactly determine how many diffs were present.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> Previous version of this patch:
>  - [v10]: $gmane/288820
>
> Changes this version wrt previous one:
> I decided to include no of diffs in every test and rewrote the commit
> message so as to sell this idea. This was given as an option to me by
> Eric and the other option being to drop unnecessary testing of lines
> where it isn't required. Also this patch uses a suggestion given by Eric
> to make the "editor" look more clean as compared to the editor in my
> previous version.
> ---

>  t/t7507-commit-verbose.sh | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2ddf28c..0f28a86 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -3,11 +3,10 @@
>  test_description='verbose commit template'
>  . ./test-lib.sh
>  
> -cat >check-for-diff <<EOF
> -#!$SHELL_PATH
> -exec grep '^diff --git' "\$1"
> +write_script "check-for-diff" <<\EOF &&
> +grep '^diff --git' "$1" >out
> +exit 0
>  EOF
> -chmod +x check-for-diff
>  test_set_editor "$PWD/check-for-diff"
>  
>  cat >message <<'EOF'
> @@ -23,7 +22,8 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'initial commit shows verbose diff' '
> -	git commit --amend -v
> +	git commit --amend -v &&
> +	test_line_count = 1 out
>  '
>  
>  test_expect_success 'second commit' '
> @@ -39,13 +39,15 @@ check_message() {
>  
>  test_expect_success 'verbose diff is stripped out' '
>  	git commit --amend -v &&
> -	check_message message
> +	check_message message &&
> +	test_line_count = 1 out
>  '
>  
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>  	git config diff.mnemonicprefix true &&
>  	git commit --amend -v &&
> -	check_message message
> +	check_message message &&
> +	test_line_count = 1 out
>  '
>  
>  cat >diff <<'EOF'
>
> --
> https://github.com/git/git/pull/218

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> Make the fake "editor" store output of grep in a file so that we can
>> see how many diffs were contained in the message and use them in
>> individual tests where ever it is required. Also use write_script()
>> to create the fake "editor".
>>
>> A subsequent commit will introduce scenarios where it is important to be
>> able to exactly determine how many diffs were present.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> Previous version of this patch:
>>  - [v10]: $gmane/288820
>>
>> Changes this version wrt previous one:
>> I decided to include no of diffs in every test and rewrote the commit
>> message so as to sell this idea. This was given as an option to me by
>> Eric and the other option being to drop unnecessary testing of lines
>> where it isn't required. Also this patch uses a suggestion given by Eric
>> to make the "editor" look more clean as compared to the editor in my
>> previous version.
>> ---
>
> OK, by always exiting 0 from the editor, you do not interfere with
> the "git commit" that invoked it, and you inspect the editor's
> finding after "git commit" returns.  The approach taken by this
> patch looks a lot more sensible than the previous one.
>
> You'd need the three-dash right before "Previous version of..."
> line, though.

>>  t/t7507-commit-verbose.sh | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> index 2ddf28c..0f28a86 100755
>> --- a/t/t7507-commit-verbose.sh
>> +++ b/t/t7507-commit-verbose.sh
>> @@ -3,11 +3,10 @@
>>  test_description='verbose commit template'
>>  . ./test-lib.sh
>>
>> -cat >check-for-diff <<EOF
>> -#!$SHELL_PATH
>> -exec grep '^diff --git' "\$1"
>> +write_script "check-for-diff" <<\EOF &&
>> +grep '^diff --git' "$1" >out
>> +exit 0
>>  EOF
>> -chmod +x check-for-diff
>>  test_set_editor "$PWD/check-for-diff"
>>
>>  cat >message <<'EOF'
>> @@ -23,7 +22,8 @@ test_expect_success 'setup' '
>>  '
>>
>>  test_expect_success 'initial commit shows verbose diff' '
>> -     git commit --amend -v
>> +     git commit --amend -v &&
>> +     test_line_count = 1 out
>>  '
>>
>>  test_expect_success 'second commit' '
>> @@ -39,13 +39,15 @@ check_message() {
>>
>>  test_expect_success 'verbose diff is stripped out' '
>>       git commit --amend -v &&
>> -     check_message message
>> +     check_message message &&
>> +     test_line_count = 1 out
>>  '
>>
>>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>>       git config diff.mnemonicprefix true &&
>>       git commit --amend -v &&
>> -     check_message message
>> +     check_message message &&
>> +     test_line_count = 1 out
>>  '
>>
>>  cat >diff <<'EOF'
>>
>> --
>> https://github.com/git/git/pull/218

> This change will not affect existing users of COUNTUP at all, as long as
> they use the initial value of 0 (or more).
>
> Force uses the "unspecified" value in conjunction with OPT__FORCE in
> builtin/clean.c in a different way to handle its config which
> munges the "-1" into 0 before we get to parse_options.

>  `OPT_COUNTUP(short, long, &int_var, description)`::
>  	Introduce a count-up option.
> -	`int_var` is incremented on each use of `--option`, and
> -	reset to zero with `--no-option`.
> +	Each use of `--option` increments `int_var`, starting from zero
> +	(even if initially negative), and `--no-option` resets it to
> +	zero. To determine if `--option` or `--no-option` was set at
> +	all, set `int_var` to a negative value, and if it is still
> +	negative after parse_options(), then neither `--option` nor
> +	`--no-option` was seen.
>  
>  `OPT_BIT(short, long, &int_var, description, mask)`::
>  	Introduce a boolean option.
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192..86d30bd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>  		return 0;
>  
>  	case OPTION_COUNTUP:
> -		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> +		if (unset)
> +			*(int *)opt->value = 0;
> +		else {
> +			if (*(int *)opt->value < 0)
> +				*(int *)opt->value = 0;
> +			*(int *)opt->value += 1;
> +		}
>  		return 0;
>  
>  	case OPTION_SET_INT:

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> This change will not affect existing users of COUNTUP at all, as long as
>> they use the initial value of 0 (or more).
>>
>> Force uses the "unspecified" value in conjunction with OPT__FORCE in
>> builtin/clean.c in a different way to handle its config which
>> munges the "-1" into 0 before we get to parse_options.
>
> These two paragraphs leave the readers wondering what the conclusion
> is.  "it is OK as long as..." makes us wonder "so are there users
> that do not satisfy that condition?"  "in a different way" makes us
> wonder "Does this change break builtin/clean.c because COUNTUP is
> used in a different way?".
>
>         This change does not affect existing users of COUNTUP,
>         because they all use the initial value of 0 (or more).
>
>         Note that builtin/clean.c initializes the variable used with
>         OPT__FORCE (which uses OPT_COUNTUP()) to a negative value,
>         but it is set to either 0 or 1 by reading the configuration
>         before the code calls parse_options(), i.e. as far as
>         parse_options() is concerned, the initial value of the
>         variable is not negative.
>
> perhaps?

>>  `OPT_COUNTUP(short, long, &int_var, description)`::
>>       Introduce a count-up option.
>> -     `int_var` is incremented on each use of `--option`, and
>> -     reset to zero with `--no-option`.
>> +     Each use of `--option` increments `int_var`, starting from zero
>> +     (even if initially negative), and `--no-option` resets it to
>> +     zero. To determine if `--option` or `--no-option` was set at
>> +     all, set `int_var` to a negative value, and if it is still
>> +     negative after parse_options(), then neither `--option` nor
>> +     `--no-option` was seen.
>>
>>  `OPT_BIT(short, long, &int_var, description, mask)`::
>>       Introduce a boolean option.
>> diff --git a/parse-options.c b/parse-options.c
>> index 47a9192..86d30bd 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>>               return 0;
>>
>>       case OPTION_COUNTUP:
>> -             *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>> +             if (unset)
>> +                     *(int *)opt->value = 0;
>> +             else {
>> +                     if (*(int *)opt->value < 0)
>> +                             *(int *)opt->value = 0;
>> +                     *(int *)opt->value += 1;
>> +             }
>>               return 0;
>>
>>       case OPTION_SET_INT:
>
> The new behaviour given by the patch looks very sensible, but I
> wonder if this is easier to reason about:
>
>         case OPTION_COUNTUP:
> +               if (*(int *)opt->value < 0)
> +                       *(int *)opt->value = 0;
>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, upon hitting this case arm, we know that an explicit option
> was given from the command line, so the "unspecified" initial value,
> if it is still there, gets reset to 0, and after doing that, we just
> do the usual thing.

> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>         case OPTION_COUNTUP:
>> +               if (*(int *)opt->value < 0)
>> +                       *(int *)opt->value = 0;
>>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>>
>> That is, upon hitting this case arm, we know that an explicit option
>> was given from the command line, so the "unspecified" initial value,
>> if it is still there, gets reset to 0, and after doing that, we just
>> do the usual thing.
>
> This does look cleaner. Nice thinking that there is no need to
> actually specify when it gets 0. It gets 0 no matter what as long as
> OPTION_COUNTUP is speficied in any format (with or without --no) and
> variable is "unspecified".

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>         case OPTION_COUNTUP:
>>> +               if (*(int *)opt->value < 0)
>>> +                       *(int *)opt->value = 0;
>>>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>>>
>>> That is, upon hitting this case arm, we know that an explicit option
>>> was given from the command line, so the "unspecified" initial value,
>>> if it is still there, gets reset to 0, and after doing that, we just
>>> do the usual thing.
>>
>> This does look cleaner. Nice thinking that there is no need to
>> actually specify when it gets 0. It gets 0 no matter what as long as
>> OPTION_COUNTUP is speficied in any format (with or without --no) and
>> variable is "unspecified".
>
> I do not think there is any planned users of such an enhancement,
> but the above points us into a future possibility to be able to do
> this:
>
>         case OPTION_COUNTUP:
> +               if (*(int *)opt->value < 0)
> +                       *(int *)opt->value = -*(int *)opt->value - 1;
>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, by using "-2" as the "unspecified" value, you can start
> counting up from 2 (i.e. the presence of the option resets the
> variable to 1 and then the option being not "unset" increments it)
> if your application wants to.

> Also, when such an idea for new feature comes up, it is better to
> implement it because let's say some developer is stuck in future and
> this new feature could help him but he doesn't have a clue that such a
> discussion happened some time ago. Thus saving him time and further
> effort.
>
> Anyways, How is the convention in git for these type of futuristic ideas?

> Current implementation of parse-options.c treats OPT__QUIET() as integer
> and not boolean and thus it is more appropriate to print it as integer
> to avoid confusion.
>
> While at it, fix some style issues.

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> Current implementation of parse-options.c treats OPT__QUIET() as integer
>> and not boolean and thus it is more appropriate to print it as integer
>> to avoid confusion.
>>
>> While at it, fix some style issues.
>
> I counted the changes in t0040 and you have _more_ style fixes than
> the real change.  That is not "while at it".
>
> While I welcome the style fix, it is better done as a preparatory
> clean-up step before the real change.

>> -cat > typo.err << EOF
>> +cat >typo.err <<EOF
>>  error: did you mean \`--boolean\` (with two dashes ?)
>>  EOF
>
> If your "style cleanup" patch were separate, you could fix this (and
> other that have backslash escape inside the here-document) further
> to something like this:
>
>         cat >type.err <<\EOF
>         error: did you mean `--boolean` (with two dashes ?)
>         EOF
>
> Thanks.

> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> So that we can see how many diffs were contained in the message and use
>>> them in individual tests where ever it is required. Also use
>>> write_script() to create the fake "editor".
>>
>> It is important to explain *why* you want to be able to count how many
>> diffs were in the editor message. In particular, you will be adding
>> new tests in a subsequent patch which will expect a specific number of
>> diffs (rather than any number of diffs)
>>
>> Also, you need to explain why you're changing the existing tests to
>> count the number of diffs. Is it simply "because you can" or is it
>> because you suspect that a change you're making in a subsequent patch
>> might accidentally cause too many diffs to show up in the existing
>> test cases?
>
> Sorry for not writing commit messages properly. It is a part I am working on.
> How does this paragraph sound to you in addition to the previous commit message?
> "A subsequent commit will introduce a scenario where it is important
> to be able to exactly determine how many diffs were present."
>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>> ---
>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>>> @@ -3,9 +3,11 @@
>>> -cat >check-for-diff <<EOF
>>> -#!$SHELL_PATH
>>> -exec grep '^diff --git' "\$1"
>>> +write_script "check-for-diff" <<'EOF' &&
>>
>> The 'EOF' is more commonly written as \EOF in Git test scripts.
>>
>>> +! test -s out ||
>>> +rm out &&
>>
>> Why not just "rm -f out"? But, more importantly, why do you need to
>> remove the file at all? The '>' redirection operator (used below) will
>> overwrite the file, so no need to remove it beforehand.
>
> I wasn't aware about '-f' option. It is important to remove the file.
> I initially tried without removing the file and it caused problems.
> This is because let's say the previous test had 1 diff which was
> stored in out. Now, if in the next test, it is expected to have 0
> diffs then grep would fail to execute and nothing will be re-written
> to out and out will contain the 1 diff from previous test. An
> explanation for this should be mentioned in the commit message?
>
>>> +! grep '^diff --git' "$1" ||
>>> +grep '^diff --git' "$1" >out
>>
>> Um, what? Why two greps? I would have expected you to simply re-use
>> the existing grep (minus the backslash) while adding the redirection:
>>
>>     -exec grep '^diff --git' "\$1"
>>     +exec grep '^diff --git' "$1" >out
>
> The reason of two greps in that if in some test it fails to find any
> diffs, the editor will return an error. Of course we could test this
> scenario by using test_must_fail as in previous patches, but I think
> it would be more clearer if we could test for the existence of out
> which is done in patch 3/3. I will explanation for this in commit
> message.
>
>>>  EOF
>>>  chmod +x check-for-diff
>>
>> Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you.
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/289832
>
> Then you mentioned that you were talking[1] about some other review.
> So I thought you mean to keep as it is. I guess I misinterpreted it.
> And I did not test that before. Now, I have tested that it works if we
> remove chmod.
> [1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289832
>
>>>  test_set_editor "$PWD/check-for-diff"
>>> @@ -23,7 +25,8 @@ test_expect_success 'setup' '
>>>  test_expect_success 'initial commit shows verbose diff' '
>>> -       git commit --amend -v
>>> +       git commit --amend -v &&
>>> +       test_line_count = 1 out
>>>  '
>>>
>>>  test_expect_success 'second commit' '
>>> @@ -39,13 +42,15 @@ check_message() {
>>>
>>>  test_expect_success 'verbose diff is stripped out' '
>>>         git commit --amend -v &&
>>> -       check_message message
>>> +       check_message message &&
>>> +       test_line_count = 1 out
>>>  '
>>>
>>>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>>>         git config diff.mnemonicprefix true &&
>>>         git commit --amend -v &&
>>> -       check_message message
>>> +       check_message message &&
>>> +       test_line_count = 1 out
>>>  '

> On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> It is important to explain *why* you want to be able to count how many
>>> diffs were in the editor message. In particular, you will be adding
>>> new tests in a subsequent patch which will expect a specific number of
>>> diffs (rather than any number of diffs)
>>>
>>> Also, you need to explain why you're changing the existing tests to
>>> count the number of diffs. Is it simply "because you can" or is it
>>> because you suspect that a change you're making in a subsequent patch
>>> might accidentally cause too many diffs to show up in the existing
>>> test cases?
>>
>> Sorry for not writing commit messages properly. It is a part I am working on.
>> How does this paragraph sound to you in addition to the previous commit message?
>> "A subsequent commit will introduce a scenario where it is important
>> to be able to exactly determine how many diffs were present."
>
> That's fine for explaining why 'check-for-diff' is being updated to
> store the output, but you still need to explain why you're changing
> the existing tests.
>
>>>> -cat >check-for-diff <<EOF
>>>> -#!$SHELL_PATH
>>>> -exec grep '^diff --git' "\$1"
>>>> +write_script "check-for-diff" <<'EOF' &&
>>>> +! test -s out ||
>>>> +rm out &&
>>>
>>> Why not just "rm -f out"? But, more importantly, why do you need to
>>> remove the file at all? The '>' redirection operator (used below) will
>>> overwrite the file, so no need to remove it beforehand.
>>
>> I wasn't aware about '-f' option. It is important to remove the file.
>> I initially tried without removing the file and it caused problems.
>> This is because let's say the previous test had 1 diff which was
>> stored in out. Now, if in the next test, it is expected to have 0
>> diffs then grep would fail to execute and nothing will be re-written
>> to out and out will contain the 1 diff from previous test. An
>> explanation for this should be mentioned in the commit message?
>
> No. Rather than trying to explain all this in the commit message, it
> would be better to retain a simple implementation of 'check-for-diff'
> rather than adding several levels of complication. More about this
> below...
>
>>>> +! grep '^diff --git' "$1" ||
>>>> +grep '^diff --git' "$1" >out
>>>
>>> Um, what? Why two greps? I would have expected you to simply re-use
>>> the existing grep (minus the backslash) while adding the redirection:
>>>
>>>     -exec grep '^diff --git' "\$1"
>>>     +exec grep '^diff --git' "$1" >out
>>
>> The reason of two greps in that if in some test it fails to find any
>> diffs, the editor will return an error. Of course we could test this
>> scenario by using test_must_fail as in previous patches, but I think
>> it would be more clearer if we could test for the existence of out
>> which is done in patch 3/3. I will explanation for this in commit
>> message.
>
> This sounds unnecessarily complicated. It's not too difficult to
> reason about a script named 'check-for-diff' actually "checking for
> presence of diffs" and failing if no diff is present, from which it
> follows naturally that new tests which don't expect any diffs would
> use test_must_fail. Keeping it simple like this also makes this patch
> much less noisy since it doesn't require changing existing tests.
>
> Likewise, it keeps most of the new tests in 3/3 small because the bulk
> of them only want to know that a diff was present, but don't care
> about the number of diffs. However, it's not necessarily a bad thing
> to ensure that you got the number of diffs you expected (for instance,
> that a single -v really behaved as -v, and not as -v -v), so that can
> be used as an argument for overhauling the old tests, as well as using
> counting in all new tests.
>
> However, even if you take the approach of making 'check-for-diff'
> succeed unconditionally and always count diffs, the current
> implementation is still overly complicated. It would be much simpler
> to let 'check-for-diff' always create the output file, and then use
> "test_line_count = 0 out" when expecting no diffs than to sometimes
> create the output file and sometimes not. The shell '>' operator will
> truncate the file to zero size even before grep is invoked, so you
> don't need to worry that results from an earlier test will pollute
> 'out' for a subsequent test, even if grep finds no matches. Thus,
> 'check-for-diff' collapses to this tiny implementation:
>
>     grep '^diff --git' "$1" >out
>     exit 0
>
> Or, if you want to be terse:
>
>     grep '^diff --git' "$1" || exit 0 >out
>
> A couple notes: First, "out" isn't a great name for this file; perhaps
> come up with a better name. Second, under this scenario,
> "check-for-diff" isn't the most accurate name; it's now simply
> collecting diffs rather than checking for presence.
>
> An alternative would be for the output file to contain the actual
> count of diffs, rather than the diff lines themselves. For instance:
>
>     write_script count-diffs <<\EOF &&
>     grep -c '^diff --git' "$1" >out
>     exit 0
>     EOF
>
> Whether that's actually better is subjective.
>
> So, what's my final word? I quite like the idea of keeping everything
> as simple as possible, thus not altering existing tests and only doing
> line count in the two or three new tests which actually care about the
> number of diffs. However, I can also be sold on retrofitting existing
> tests and having new tests do line counting as a way to improve test
> coverage by catching cases where "-v" incorrectly behaves as "-v -v".
> If you go that route, then the commit message of this patch needs to
> sell it as such (an improvement in test coverage).

> On Sun, Mar 27, 2016 at 5:05 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, Mar 27, 2016 at 12:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> However, even if you take the approach of making 'check-for-diff'
>>> succeed unconditionally and always count diffs, the current
>>> implementation is still overly complicated. It would be much simpler
>>> to let 'check-for-diff' always create the output file, and then use
>>> "test_line_count = 0 out" when expecting no diffs than to sometimes
>>> create the output file and sometimes not. The shell '>' operator will
>>> truncate the file to zero size even before grep is invoked, so you
>>> don't need to worry that results from an earlier test will pollute
>>> 'out' for a subsequent test, even if grep finds no matches. Thus,
>>> 'check-for-diff' collapses to this tiny implementation:
>>>
>>>     grep '^diff --git' "$1" >out
>>>     exit 0
>>>
>>> Or, if you want to be terse:
>>>
>>>     grep '^diff --git' "$1" || exit 0 >out
>>
>> I tried using both of this and it gives an error
>
> These worked fine for me when I rewrote the test script before making
> the suggestion. You'll need to provide more information if you want to
> get to the bottom of the problem you experienced (for instance, show
> the exact code you used and the actual error message).