# [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script

23 messages from 2016-03-24 to 2016-03-28. Participants: Pranit Bauva, SZEDER Gábor, Eric Sunshine.
Thread: https://gitlist.dev/t/41807

## Pranit Bauva, 2016-03-24 08:25

Subject: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <01020153a7ba4eae-9b88e119-0505-418f-a137-595250edaa9d-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020153a7ba4eae-9b88e119-0505-418f-a137-595250edaa9d-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com>

```
Also remove test_set_editor from global scope and use it in whichever
test it is required.
---
 t/t7507-commit-verbose.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2ddf28c..cf95efb 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -3,12 +3,11 @@
 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 &&
+test $(wc -l <out) = 1
 EOF
 chmod +x check-for-diff
-test_set_editor "$PWD/check-for-diff"
 
 cat >message <<'EOF'
 subject
@@ -23,6 +22,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'initial commit shows verbose diff' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git commit --amend -v
 '
 
@@ -38,11 +38,13 @@ check_message() {
 }
 
 test_expect_success 'verbose diff is stripped out' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git commit --amend -v &&
 	check_message message
 '
 
 test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git config diff.mnemonicprefix true &&
 	git commit --amend -v &&
 	check_message message
@@ -66,11 +68,13 @@ test_expect_success 'diff in message is retained without -v' '
 '
 
 test_expect_success 'diff in message is retained with -v' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git commit --amend -F diff -v &&
 	check_message diff
 '
 
 test_expect_success 'submodule log is stripped out too with -v' '
+	test_set_editor "$PWD/check-for-diff" &&
 	git config diff.submodule log &&
 	git submodule add ./. sub &&
 	git commit -m "sub added" &&

--
https://github.com/git/git/pull/218

```

## Pranit Bauva, 2016-03-24 08:25

Subject: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000%40eu-west-1.amazonses.com
In-Reply-To: <010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com>

```
The reason to make it consider negative values or more specifically
"unspecified" values is to give the ability to differentiate between
once, multiple time or with --no-option.

Eg. :
initialize verbose = -1
`git commit` => verbose = -1
`git commit -v` => verbose = 1
`git commit -v -v` => verbose = 2
`git commit --no-verbose` => verbose = 0

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

Changes introduced:
Use a different language in commit message to make the change and its
utility more clear.
---
 Documentation/technical/api-parse-options.txt | 6 ++++--
 parse-options.c                               | 8 +++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 5f0757d..a908d8a 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -144,8 +144,10 @@ 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`.
+	If the `int_var` is negative and `--option` is absent,
+	then it will retain its value. Otherwise it will first set
+	`int_var` to 0 and then increment it on each use of `--option`,
+	and reset to zero with `--no-option`.
 
 `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:

--
https://github.com/git/git/pull/218

```

## Pranit Bauva, 2016-03-24 08:25

Subject: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com>

```
Add commit.verbose configuration variable as a convenience for those
who always prefer --verbose.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
The previous version of the patch are:
 - [v8] $gmane/288820
 - [v7] $gmane/288820
 - [v6] $gmane/288728
 - [v5] $gmane/288728
 - [v4] $gmane/288652
 - [v3] $gmane/288634
 - [v2] $gmane/288569
 - [v1] $gmane/287540

Changes with respect to the previous patch:
 - Compare with -1 as only -1 value is used for unspecified behavior.
 - Write clean tests
---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             | 13 ++++++++++-
 t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..1d0ec2e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1110,6 +1110,10 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.verbose::
+	A boolean or int to specify the level of verbose with `git commit`.
+	See linkgit:git-commit[1].
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9ec6b3c..d474226 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].
 	what changes the commit has.
 	Note that this diff output doesn't have its
 	lines prefixed with '#'. This diff will not be a part
-	of the commit message.
+	of the commit message. See the `commit.verbose` configuration
+	variable in linkgit:git-config[1].
 +
 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged
diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..db65c03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -113,7 +113,9 @@ static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int config_verbose = -1; /* unspecified */
+static int verbose = -1; /* unspecified */
+static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
@@ -1505,6 +1507,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
 	}
+	if (!strcmp(k, "commit.verbose")) {
+		int is_bool;
+		config_verbose = git_config_bool_or_int(k, v, &is_bool);
+		return 0;
+	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
@@ -1654,6 +1661,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
index cf95efb..66deac3 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -9,6 +9,12 @@ test $(wc -l <out) = 1
 EOF
 chmod +x check-for-diff
 
+write_script "check-for-double-diff" <<-\EOF &&
+grep '# Changes not staged for commit' "$1" >out &&
+test $(wc -l <out) = 2
+EOF
+chmod +x check-for-double-diff
+
 cat >message <<'EOF'
 subject
 
@@ -100,4 +106,49 @@ 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 'commit.verbose true and --verbose omitted' '
+	test_set_editor "$PWD/check-for-diff" &&
+	git -c commit.verbose=true commit --amend
+'
+
+test_expect_success 'commit.verbose true and --verbose' '
+	test_set_editor "$PWD/check-for-diff" &&
+	git -c commit.verbose=true commit --amend --verbose
+'
+
+test_expect_success 'commit.verbose true and -v -v' '
+	test_set_editor "$PWD/check-for-double-diff" &&
+	git -c commit.verbose=true commit --amend -v -v
+'
+
+test_expect_success 'commit.verbose true and --no-verbose' '
+	test_set_editor "$PWD/check-for-diff" &&
+	test_must_fail git -c commit.verbose=true commit --amend --no-verbose
+'
+
+test_expect_success 'commit.verbose false and --verbose' '
+	test_set_editor "$PWD/check-for-diff" &&
+	git -c commit.verbose=false commit --amend --verbose
+'
+
+test_expect_success 'commit.verbose false and -v -v' '
+	test_set_editor "$PWD/check-for-double-diff" &&
+	git -c commit.verbose=false commit --amend -v -v
+'
+
+test_expect_success 'commit.verbose false and --verbose omitted' '
+	test_set_editor "$PWD/check-for-diff" &&
+	test_must_fail git -c commit.verbose=false commit --amend
+'
+
+test_expect_success 'commit.verbose false and --no-verbose' '
+	test_set_editor "$PWD/check-for-diff" &&
+	test_must_fail git -c commit.verbose=false commit --amend --no-verbose
+'
+
+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

```

## SZEDER Gábor, 2016-03-24 10:04

Subject: Re: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <1458813876-10591-1-git-send-email-szeder@ira.uka.de>
URL: https://gitlist.dev/e/1458813876-10591-1-git-send-email-szeder%40ira.uka.de
In-Reply-To: <01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com>

```
> Add commit.verbose configuration variable as a convenience for those
> who always prefer --verbose.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> 
> ---
> The previous version of the patch are:
>  - [v8] $gmane/288820
>  - [v7] $gmane/288820
>  - [v6] $gmane/288728
>  - [v5] $gmane/288728
>  - [v4] $gmane/288652
>  - [v3] $gmane/288634
>  - [v2] $gmane/288569
>  - [v1] $gmane/287540
> 
> Changes with respect to the previous patch:
>  - Compare with -1 as only -1 value is used for unspecified behavior.
>  - Write clean tests
> ---
>  Documentation/config.txt     |  4 ++++
>  Documentation/git-commit.txt |  3 ++-
>  builtin/commit.c             | 13 ++++++++++-
>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 69 insertions(+), 2 deletions(-)

Please always run the full test suite before submitting patches to
make sure that your changes do not inadvertently break something.
This patch breaks several tests in t7512-status-help.sh,
t7508-status.sh and t7060-wtstatus.sh.

```

## Pranit Bauva, 2016-03-24 10:22

Subject: Re: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <CAFZEwPN2+RwrE583XHOv9=05XQaWZnL=cK6wa0rMC-3AU275yg@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPN2%2BRwrE583XHOv9%3D05XQaWZnL%3DcK6wa0rMC-3AU275yg%40mail.gmail.com
In-Reply-To: <1458813876-10591-1-git-send-email-szeder@ira.uka.de>

```
On Thu, Mar 24, 2016 at 3:34 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> ---
>> The previous version of the patch are:
>>  - [v8] $gmane/288820
>>  - [v7] $gmane/288820
>>  - [v6] $gmane/288728
>>  - [v5] $gmane/288728
>>  - [v4] $gmane/288652
>>  - [v3] $gmane/288634
>>  - [v2] $gmane/288569
>>  - [v1] $gmane/287540
>>
>> Changes with respect to the previous patch:
>>  - Compare with -1 as only -1 value is used for unspecified behavior.
>>  - Write clean tests
>> ---
>>  Documentation/config.txt     |  4 ++++
>>  Documentation/git-commit.txt |  3 ++-
>>  builtin/commit.c             | 13 ++++++++++-
>>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 69 insertions(+), 2 deletions(-)
>
> Please always run the full test suite before submitting patches to
> make sure that your changes do not inadvertently break something.
> This patch breaks several tests in t7512-status-help.sh,
> t7508-status.sh and t7060-wtstatus.sh.

Sorry for that. I will make sure I do run the complete test suite. I
currently ran only commit based tests. But now that I think about it
that since status and commit share a lot of things, it might be
possible to break parts of status. I will investigate further as to
what cased this problem though I kind of get a hint that it is because
of verbose being the parent and others consuming it. There are a lot
of tests failing.

```

## Pranit Bauva, 2016-03-24 10:26

Subject: Re: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <CAFZEwPNyELB2BZ1q95HroV0a3vfNK0eTtzcy-2aEmUY5QVZe-g@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPNyELB2BZ1q95HroV0a3vfNK0eTtzcy-2aEmUY5QVZe-g%40mail.gmail.com
In-Reply-To: <CAFZEwPN2+RwrE583XHOv9=05XQaWZnL=cK6wa0rMC-3AU275yg@mail.gmail.com>

```
I got it. The verbose is initialised to -1 before. When cmd_commit
runs, it changes the value of verbose accordingly to 0 or positive.
But when cmd_status runs, it will retain the value -1 and the if
clause which accepts all values except 0 will execute. I guess a if
statement inside cmd_status which reinitialises it to 0 or positive
depending on the situation will solve the problem.

On Thu, Mar 24, 2016 at 3:52 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Thu, Mar 24, 2016 at 3:34 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> Add commit.verbose configuration variable as a convenience for those
>>> who always prefer --verbose.
>>>
>>> Helped-by: Junio C Hamano <gitster@pobox.com>
>>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>
>>> ---
>>> The previous version of the patch are:
>>>  - [v8] $gmane/288820
>>>  - [v7] $gmane/288820
>>>  - [v6] $gmane/288728
>>>  - [v5] $gmane/288728
>>>  - [v4] $gmane/288652
>>>  - [v3] $gmane/288634
>>>  - [v2] $gmane/288569
>>>  - [v1] $gmane/287540
>>>
>>> Changes with respect to the previous patch:
>>>  - Compare with -1 as only -1 value is used for unspecified behavior.
>>>  - Write clean tests
>>> ---
>>>  Documentation/config.txt     |  4 ++++
>>>  Documentation/git-commit.txt |  3 ++-
>>>  builtin/commit.c             | 13 ++++++++++-
>>>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 69 insertions(+), 2 deletions(-)
>>
>> Please always run the full test suite before submitting patches to
>> make sure that your changes do not inadvertently break something.
>> This patch breaks several tests in t7512-status-help.sh,
>> t7508-status.sh and t7060-wtstatus.sh.
>
> Sorry for that. I will make sure I do run the complete test suite. I
> currently ran only commit based tests. But now that I think about it
> that since status and commit share a lot of things, it might be
> possible to break parts of status. I will investigate further as to
> what cased this problem though I kind of get a hint that it is because
> of verbose being the parent and others consuming it. There are a lot
> of tests failing.

```

## SZEDER Gábor, 2016-03-24 10:33

Subject: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <1458815593-11060-1-git-send-email-szeder@ira.uka.de>
URL: https://gitlist.dev/e/1458815593-11060-1-git-send-email-szeder%40ira.uka.de
In-Reply-To: <01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com>

```
> The reason to make it consider negative values or more specifically
> "unspecified" values is to give the ability to differentiate between
> once, multiple time or with --no-option.
> 
> Eg. :
> initialize verbose = -1
> `git commit` => verbose = -1
> `git commit -v` => verbose = 1
> `git commit -v -v` => verbose = 2
> `git commit --no-verbose` => verbose = 0
> 
> 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
> 
> Changes introduced:
> Use a different language in commit message to make the change and its
> utility more clear.

I don't see the mentioned change in the commit message.  In
particular:

  - As Eric pointed out in the previous round, the commit message
    misses the single most important point of justification: to
    determine whether '--option' or '--no-option' was specified at
    all.  Explaining this would also make the examples unnecessary.

  - OPT_COUNTUP is used in several places, mostly indirecty, but the
    commit message doesn't explain that possible side-effects to these
    callers were considered and that they are not harmed by this
    change.

> ---
>  Documentation/technical/api-parse-options.txt | 6 ++++--
>  parse-options.c                               | 8 +++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)

A couple of new tests to t0040-parse-options.sh would be great to
ensure that starting from a negative value works as advertised, i.e.
at least that '--option' jumps to 1 and '--no-option' resets to 0.

> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 5f0757d..a908d8a 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -144,8 +144,10 @@ 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`.
> +	If the `int_var` is negative and `--option` is absent,
> +	then it will retain its value. Otherwise it will first set
> +	`int_var` to 0 and then increment it on each use of `--option`,
> +	and reset to zero with `--no-option`.
>  
>  `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:
> 
> --
> https://github.com/git/git/pull/218
> 
> 

```

## Pranit Bauva, 2016-03-24 10:38

Subject: Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <CAFZEwPOwmJuLzUsAwqZttYcHaZsekpR_o9bXtXv73JBSFuxNYQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPOwmJuLzUsAwqZttYcHaZsekpR_o9bXtXv73JBSFuxNYQ%40mail.gmail.com
In-Reply-To: <1458815593-11060-1-git-send-email-szeder@ira.uka.de>

```
On Thu, Mar 24, 2016 at 4:03 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> The reason to make it consider negative values or more specifically
>> "unspecified" values is to give the ability to differentiate between
>> once, multiple time or with --no-option.
>>
>> Eg. :
>> initialize verbose = -1
>> `git commit` => verbose = -1
>> `git commit -v` => verbose = 1
>> `git commit -v -v` => verbose = 2
>> `git commit --no-verbose` => verbose = 0
>>
>> 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
>>
>> Changes introduced:
>> Use a different language in commit message to make the change and its
>> utility more clear.
>
> I don't see the mentioned change in the commit message.  In
> particular:
>
>   - As Eric pointed out in the previous round, the commit message
>     misses the single most important point of justification: to
>     determine whether '--option' or '--no-option' was specified at
>     all.  Explaining this would also make the examples unnecessary.

I will justify it in the commit message. Also will remove the examples.

>   - OPT_COUNTUP is used in several places, mostly indirecty, but the
>     commit message doesn't explain that possible side-effects to these
>     callers were considered and that they are not harmed by this
>     change.

I will include bits from my and Jeff's conversation into this commit message.

>> ---
>>  Documentation/technical/api-parse-options.txt | 6 ++++--
>>  parse-options.c                               | 8 +++++++-
>>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> A couple of new tests to t0040-parse-options.sh would be great to
> ensure that starting from a negative value works as advertised, i.e.
> at least that '--option' jumps to 1 and '--no-option' resets to 0.
>
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> index 5f0757d..a908d8a 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,10 @@ 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`.
>> +     If the `int_var` is negative and `--option` is absent,
>> +     then it will retain its value. Otherwise it will first set
>> +     `int_var` to 0 and then increment it on each use of `--option`,
>> +     and reset to zero with `--no-option`.
>>
>>  `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:
>>
>> --
>> https://github.com/git/git/pull/218
>>
>>

```

## SZEDER Gábor, 2016-03-24 11:00

Subject: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <1458817259-11675-1-git-send-email-szeder@ira.uka.de>
URL: https://gitlist.dev/e/1458817259-11675-1-git-send-email-szeder%40ira.uka.de
In-Reply-To: <01020153a7ba4eae-9b88e119-0505-418f-a137-595250edaa9d-000000@eu-west-1.amazonses.com>

```
> Also remove test_set_editor from global scope and use it in whichever
> test it is required.

Why?

test_set_editor sets and exports shell variables.  Since you don't
invoke test_set_editor in a subshell, after the first invocation the
editor will be part of the global scope anyway.

Also missing signoff.

> ---
>  t/t7507-commit-verbose.sh | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2ddf28c..cf95efb 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -3,12 +3,11 @@
>  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 &&
> +test $(wc -l <out) = 1

Our test lib offers the test_line_count helper function, which
outputs a helpful error message in case the number of lines do not
match.

The original didn't check the number of lines.  This change is not
mentioned at all in the commit message.

>  EOF
>  chmod +x check-for-diff
> -test_set_editor "$PWD/check-for-diff"
>  
>  cat >message <<'EOF'
>  subject
> @@ -23,6 +22,7 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'initial commit shows verbose diff' '
> +	test_set_editor "$PWD/check-for-diff" &&
>  	git commit --amend -v
>  '
>  
> @@ -38,11 +38,13 @@ check_message() {
>  }
>  
>  test_expect_success 'verbose diff is stripped out' '
> +	test_set_editor "$PWD/check-for-diff" &&
>  	git commit --amend -v &&
>  	check_message message
>  '
>  
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
> +	test_set_editor "$PWD/check-for-diff" &&
>  	git config diff.mnemonicprefix true &&
>  	git commit --amend -v &&
>  	check_message message
> @@ -66,11 +68,13 @@ test_expect_success 'diff in message is retained without -v' '
>  '
>  
>  test_expect_success 'diff in message is retained with -v' '
> +	test_set_editor "$PWD/check-for-diff" &&
>  	git commit --amend -F diff -v &&
>  	check_message diff
>  '
>  
>  test_expect_success 'submodule log is stripped out too with -v' '
> +	test_set_editor "$PWD/check-for-diff" &&
>  	git config diff.submodule log &&
>  	git submodule add ./. sub &&
>  	git commit -m "sub added" &&
> 
> --
> https://github.com/git/git/pull/218
> 

```

## Eric Sunshine, 2016-03-24 23:34

Subject: Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <CAPig+cRszLsZh1ka7+y3EehQxUK67jMrVJse1EOzwLCcs-7x9Q@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcRszLsZh1ka7%2By3EehQxUK67jMrVJse1EOzwLCcs-7x9Q%40mail.gmail.com
In-Reply-To: <1458815593-11060-1-git-send-email-szeder@ira.uka.de>

```
On Thu, Mar 24, 2016 at 6:33 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> The reason to make it consider negative values or more specifically
>> "unspecified" values is to give the ability to differentiate between
>> once, multiple time or with --no-option.
>>
>> Eg. :
>> initialize verbose = -1
>> `git commit` => verbose = -1
>> `git commit -v` => verbose = 1
>> `git commit -v -v` => verbose = 2
>> `git commit --no-verbose` => verbose = 0
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> Changes introduced:
>> Use a different language in commit message to make the change and its
>> utility more clear.
>
> I don't see the mentioned change in the commit message.  In
> particular:
>
>   - As Eric pointed out in the previous round, the commit message
>     misses the single most important point of justification: to
>     determine whether '--option' or '--no-option' was specified at
>     all.  Explaining this would also make the examples unnecessary.

Agreed. It would be more helpful to explain that you are changing
OPT_COUNTUP so that --option always counts up from zero, even if the
initial value was negative, in order to allow the caller to determine
whether --option or --no-option was seen at all, by setting the
initial value to -1 and then checking if it still -1 after
parse_options(). If you explain that well, then the tabular example in
your commit message can go away altogether.

The subject ("make OPTION__COUNTUP consider negative values") could
use a little work too. OPTION__COUNTUP already works with negative
values, but not in the way you want. Instead, you want it to treat
negative values specially as "unspecified", and that's the real gist
of this patch, thus the subject ought, at the very least, have the
word "unspecified" in it.

>   - OPT_COUNTUP is used in several places, mostly indirecty, but the
>     commit message doesn't explain that possible side-effects to these
>     callers were considered and that they are not harmed by this
>     change.
>
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,10 @@ 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`.
>> +     If the `int_var` is negative and `--option` is absent,
>> +     then it will retain its value. Otherwise it will first set
>> +     `int_var` to 0 and then increment it on each use of `--option`,
>> +     and reset to zero with `--no-option`.

This reads a little bit backward, but, more importantly, doesn't do a
good job of conveying to the reader that a negative value can
represent "unspecified". The best way to convey that would probably be
via example. For instance, something like this:

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

```

## Eric Sunshine, 2016-03-24 23:57

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg%40mail.gmail.com
In-Reply-To: <1458817259-11675-1-git-send-email-szeder@ira.uka.de>

```
On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> Also remove test_set_editor from global scope and use it in whichever
>> test it is required.
>
> Why?
>
> test_set_editor sets and exports shell variables.  Since you don't
> invoke test_set_editor in a subshell, after the first invocation the
> editor will be part of the global scope anyway.

Agreed that this needs proper justification in the commit message.
And, the justification is to make each test more self-contained,
particularly because a subsequent patch will introduce a second fake
"editor", and by making tests responsible for setting the editor they
need, they don't have to worry about bad interactions from "editors"
set by earlier tests[1][2].

Another issue is that the commit message is backward. The subject
("t7507-commit-verbose: make test suite use write_script") tries to
sell this as primarily being about write_script(), but the real gist
of the patch is that it is making each test more self-contained, and
the subject should say so. The write_script() bit is just a minor
aside which can be introduced with a "While here let's use
write_script to..." at the end of the commit message.

>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> index 2ddf28c..cf95efb 100755
>> --- a/t/t7507-commit-verbose.sh
>> +++ b/t/t7507-commit-verbose.sh
>> @@ -3,12 +3,11 @@
>>  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 &&
>> +test $(wc -l <out) = 1
>
> Our test lib offers the test_line_count helper function, which
> outputs a helpful error message in case the number of lines do not
> match.

test_line_count() was mentioned in [2], however, this line counting is
done in the fake "editor" script, not in the test script proper, so
the spelled-out form $(wc ...) was proposed[2].

> The original didn't check the number of lines.  This change is not
> mentioned at all in the commit message.

Right, and this particular change really belongs in patch 3/3 where
it's needed[2], and should be properly explained by the 3/3 commit
message.

>>  EOF
>>  chmod +x check-for-diff

Drop the 'chmod' line; write_script() does this for you.

>> -test_set_editor "$PWD/check-for-diff"
>>
>>  cat >message <<'EOF'
>>  subject

[1]: http://article.gmane.org/gmane.comp.version-control.git/289329
[2]: http://article.gmane.org/gmane.comp.version-control.git/289370

```

## Eric Sunshine, 2016-03-25 00:05

Subject: Re: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <CAPig+cQ3TnKV1GcViq9Qwtq8bkp19jqvQRzBT_v5-MSfSfXgZA@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcQ3TnKV1GcViq9Qwtq8bkp19jqvQRzBT_v5-MSfSfXgZA%40mail.gmail.com
In-Reply-To: <01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com>

```
On Thu, Mar 24, 2016 at 4:25 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Add commit.verbose configuration variable as a convenience for those
> who always prefer --verbose.

The implementation looks better in this version. A couple comments
below about the test script...

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -9,6 +9,12 @@ test $(wc -l <out) = 1
>  EOF
>  chmod +x check-for-diff

Mentioned in patch 2/3 review, this patch (3/3) is where you should
update 'check-for-diff' to also check the line count, and the commit
message should explain the reason for doing so (and don't forget to
mention that it won't harm existing clients of 'check-for-diff').

> +write_script "check-for-double-diff" <<-\EOF &&
> +grep '# Changes not staged for commit' "$1" >out &&
> +test $(wc -l <out) = 2
> +EOF
> +chmod +x check-for-double-diff

Also mentioned in patch 2/3 review: drop 'chmod'; write_script() does
it for you.

```

## Pranit Bauva, 2016-03-25 06:06

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAFZEwPNMB2Ti2d=qdZKyoYzneHxUPATiFWd_fSPxktZuF3Q+pw@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPNMB2Ti2d%3DqdZKyoYzneHxUPATiFWd_fSPxktZuF3Q%2Bpw%40mail.gmail.com
In-Reply-To: <CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> Also remove test_set_editor from global scope and use it in whichever
>>> test it is required.
>>
>> Why?
>>
>> test_set_editor sets and exports shell variables.  Since you don't
>> invoke test_set_editor in a subshell, after the first invocation the
>> editor will be part of the global scope anyway.
>
> Agreed that this needs proper justification in the commit message.
> And, the justification is to make each test more self-contained,
> particularly because a subsequent patch will introduce a second fake
> "editor", and by making tests responsible for setting the editor they
> need, they don't have to worry about bad interactions from "editors"
> set by earlier tests[1][2].

This shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave
> Another issue is that the commit message is backward. The subject
> ("t7507-commit-verbose: make test suite use write_script") tries to
> sell this as primarily being about write_script(), but the real gist
> of the patch is that it is making each test more self-contained, and
> the subject should say so. The write_script() bit is just a minor
> aside which can be introduced with a "While here let's use
> write_script to..." at the end of the commit message.
>
>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>>> index 2ddf28c..cf95efb 100755
>>> --- a/t/t7507-commit-verbose.sh
>>> +++ b/t/t7507-commit-verbose.sh
>>> @@ -3,12 +3,11 @@
>>>  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 &&
>>> +test $(wc -l <out) = 1
>>
>> Our test lib offers the test_line_count helper function, which
>> outputs a helpful error message in case the number of lines do not
>> match.
>
> test_line_count() was mentioned in [2], however, this line counting is
> done in the fake "editor" script, not in the test script proper, so
> the spelled-out form $(wc ...) was proposed[2].

I have a slight doubt regarding this. Can the functions from test-lib
work in such scripts flawlessly? If yes, then its probably better to
use them.

>> The original didn't check the number of lines.  This change is not
>> mentioned at all in the commit message.
>
> Right, and this particular change really belongs in patch 3/3 where
> it's needed[2], and should be properly explained by the 3/3 commit
> message.

Sure! It should have been mentioned.

>>>  EOF
>>>  chmod +x check-for-diff
>
> Drop the 'chmod' line; write_script() does this for you.

I was unaware about this. I will drop it off.

>>> -test_set_editor "$PWD/check-for-diff"
>>>
>>>  cat >message <<'EOF'
>>>  subject
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289329
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289370

```

## Pranit Bauva, 2016-03-25 06:15

Subject: Re: [PATCH v9 3/3] commit: add a commit.verbose config variable
Message-ID: <CAFZEwPNXPDWA2R4p_pUWYEa+CeQDEpQiHz_b+UknfuA4+5KB5g@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPNXPDWA2R4p_pUWYEa%2BCeQDEpQiHz_b%2BUknfuA4%2B5KB5g%40mail.gmail.com
In-Reply-To: <CAPig+cQ3TnKV1GcViq9Qwtq8bkp19jqvQRzBT_v5-MSfSfXgZA@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 24, 2016 at 4:25 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose.
>
> The implementation looks better in this version. A couple comments
> below about the test script...
>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> @@ -9,6 +9,12 @@ test $(wc -l <out) = 1
>>  EOF
>>  chmod +x check-for-diff
>
> Mentioned in patch 2/3 review, this patch (3/3) is where you should
> update 'check-for-diff' to also check the line count, and the commit
> message should explain the reason for doing so (and don't forget to
> mention that it won't harm existing clients of 'check-for-diff').

Agree that check the line count should belong to this patch. And will
add more details in the commit message.

>> +write_script "check-for-double-diff" <<-\EOF &&
>> +grep '# Changes not staged for commit' "$1" >out &&
>> +test $(wc -l <out) = 2
>> +EOF
>> +chmod +x check-for-double-diff
>
> Also mentioned in patch 2/3 review: drop 'chmod'; write_script() does
> it for you.

```

## Eric Sunshine, 2016-03-25 06:24

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAPig+cRY8P2WtqR4y0zW+netCHa3=9Wnb8LmcB35SqP4OGC6Vg@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcRY8P2WtqR4y0zW%2BnetCHa3%3D9Wnb8LmcB35SqP4OGC6Vg%40mail.gmail.com
In-Reply-To: <CAFZEwPNMB2Ti2d=qdZKyoYzneHxUPATiFWd_fSPxktZuF3Q+pw@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 2:06 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Agreed that this needs proper justification in the commit message.
>> And, the justification is to make each test more self-contained,
>> particularly because a subsequent patch will introduce a second fake
>> "editor", and by making tests responsible for setting the editor they
>> need, they don't have to worry about bad interactions from "editors"
>> set by earlier tests[1][2].
>
> This shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave

Hmm, yes, what you say makes perfect sense... er, wait...

>>>> -cat >check-for-diff <<EOF
>>>> -#!$SHELL_PATH
>>>> -exec grep '^diff --git' "\$1"
>>>> +write_script "check-for-diff" <<-\EOF &&
>>>> +grep '^diff --git' "$1" >out &&
>>>> +test $(wc -l <out) = 1
>>>
>>> Our test lib offers the test_line_count helper function, which
>>> outputs a helpful error message in case the number of lines do not
>>> match.
>>
>> test_line_count() was mentioned in [2], however, this line counting is
>> done in the fake "editor" script, not in the test script proper, so
>> the spelled-out form $(wc ...) was proposed[2].
>
> I have a slight doubt regarding this. Can the functions from test-lib
> work in such scripts flawlessly? If yes, then its probably better to
> use them.

Probably not easily, and it's not worth complicating the "editor"
script by trying to import the test_line_count() function.

>>>>  chmod +x check-for-diff
>>
>> Drop the 'chmod' line; write_script() does this for you.
>
> I was unaware about this. I will drop it off.

I thought I had mentioned it in the review in which I had suggested
using write_script() or one of the follow-up emails, but upon looking
back at those messages, I saw it was not so. It turns out that I was
probably thinking about a different patch review in which I had
mentioned dropping 'chmod'[1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/288085/

```

## Pranit Bauva, 2016-03-25 06:55

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAFZEwPPXPL=d2S+Htc+uFyrUfrWLZRk=7YFDevHNpuEB5ofVDQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPPXPL%3Dd2S%2BHtc%2BuFyrUfrWLZRk%3D7YFDevHNpuEB5ofVDQ%40mail.gmail.com
In-Reply-To: <CAPig+cRY8P2WtqR4y0zW+netCHa3=9Wnb8LmcB35SqP4OGC6Vg@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Mar 25, 2016 at 2:06 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> Agreed that this needs proper justification in the commit message.
>>> And, the justification is to make each test more self-contained,
>>> particularly because a subsequent patch will introduce a second fake
>>> "editor", and by making tests responsible for setting the editor they
>>> need, they don't have to worry about bad interactions from "editors"
>>> set by earlier tests[1][2].
>>
>> This shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave
>
> Hmm, yes, what you say makes perfect sense... er, wait...

haha :) Sorry for this. My browser crashed and it sent out some crap
(don't know how). I mean to say, "This should have been mentioned in
the commit message that scope of editor is reduced so as to not worry
about bad interactions from other "editors" which may be used after
wards."
>
>>>>> -cat >check-for-diff <<EOF
>>>>> -#!$SHELL_PATH
>>>>> -exec grep '^diff --git' "\$1"
>>>>> +write_script "check-for-diff" <<-\EOF &&
>>>>> +grep '^diff --git' "$1" >out &&
>>>>> +test $(wc -l <out) = 1
>>>>
>>>> Our test lib offers the test_line_count helper function, which
>>>> outputs a helpful error message in case the number of lines do not
>>>> match.
>>>
>>> test_line_count() was mentioned in [2], however, this line counting is
>>> done in the fake "editor" script, not in the test script proper, so
>>> the spelled-out form $(wc ...) was proposed[2].
>>
>> I have a slight doubt regarding this. Can the functions from test-lib
>> work in such scripts flawlessly? If yes, then its probably better to
>> use them.
>
> Probably not easily, and it's not worth complicating the "editor"
> script by trying to import the test_line_count() function.

Sure!

>>>>>  chmod +x check-for-diff
>>>
>>> Drop the 'chmod' line; write_script() does this for you.
>>
>> I was unaware about this. I will drop it off.
>
> I thought I had mentioned it in the review in which I had suggested
> using write_script() or one of the follow-up emails, but upon looking
> back at those messages, I saw it was not so. It turns out that I was
> probably thinking about a different patch review in which I had
> mentioned dropping 'chmod'[1].
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/288085/
Oh! I hadn't tested by removing chmod. I will try it for fun.

```

## SZEDER Gábor, 2016-03-25 14:46

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu>
URL: https://gitlist.dev/e/20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl%40webmail.informatik.kit.edu
In-Reply-To: <CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com>

```

Quoting Eric Sunshine <sunshine@sunshineco.com>:

> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:

>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>>> index 2ddf28c..cf95efb 100755
>>> --- a/t/t7507-commit-verbose.sh
>>> +++ b/t/t7507-commit-verbose.sh
>>> @@ -3,12 +3,11 @@
>>>  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 &&
>>> +test $(wc -l <out) = 1
>>
>> Our test lib offers the test_line_count helper function, which
>> outputs a helpful error message in case the number of lines do not
>> match.
>
> test_line_count() was mentioned in [2], however, this line counting is
> done in the fake "editor" script, not in the test script proper, so
> the spelled-out form $(wc ...) was proposed[2].

Ah, yes, of course.

But then the question is: why is the line counting in the editor script
in the first place?

By redirecting grep's output to a file in the editor script, like this
patch wanted to, we can count the lines in the test script itself after
'git commit' finished.  This way we could use test_line_count, with
all its error reporting benefits, and we could use the same editor
script for all tests.

And if you insist on doing the line counting in the editor script, then
why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?

```

## Pranit Bauva, 2016-03-25 14:50

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAFZEwPMxESunF2FojXY0AwBQuNqJuRrqWxnDwHESjypi5zJdXA@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPMxESunF2FojXY0AwBQuNqJuRrqWxnDwHESjypi5zJdXA%40mail.gmail.com
In-Reply-To: <20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu>

```
On Fri, Mar 25, 2016 at 8:16 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

> By redirecting grep's output to a file in the editor script, like this
> patch wanted to, we can count the lines in the test script itself after
> 'git commit' finished.  This way we could use test_line_count, with
> all its error reporting benefits, and we could use the same editor
> script for all tests.
>
> And if you insist on doing the line counting in the editor script, then
> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?

Nice way. Hadn't thought of this before. This will also eliminate
having 2 different scripts to test for 1 line and other to test for 2
lines.

```

## Eric Sunshine, 2016-03-25 17:04

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAPig+cQLOwAimmHeUmi5+KScgOP0hr9-MiX8gyGcEk6Zj0U_jQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcQLOwAimmHeUmi5%2BKScgOP0hr9-MiX8gyGcEk6Zj0U_jQ%40mail.gmail.com
In-Reply-To: <20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu>

```
On Fri, Mar 25, 2016 at 10:46 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Quoting Eric Sunshine <sunshine@sunshineco.com>:
>> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>>> -cat >check-for-diff <<EOF
>>>> -#!$SHELL_PATH
>>>> -exec grep '^diff --git' "\$1"
>>>> +write_script "check-for-diff" <<-\EOF &&
>>>> +grep '^diff --git' "$1" >out &&
>>>> +test $(wc -l <out) = 1
>>>
>>> Our test lib offers the test_line_count helper function, which
>>> outputs a helpful error message in case the number of lines do not
>>> match.
>>
>> test_line_count() was mentioned in [2], however, this line counting is
>> done in the fake "editor" script, not in the test script proper, so
>> the spelled-out form $(wc ...) was proposed[2].
>
> Ah, yes, of course.
>
> But then the question is: why is the line counting in the editor script
> in the first place?
>
> By redirecting grep's output to a file in the editor script, like this
> patch wanted to, we can count the lines in the test script itself after
> 'git commit' finished.  This way we could use test_line_count, with
> all its error reporting benefits, and we could use the same editor
> script for all tests.

That works too, simplifying the overall implementation, and
eliminating the need for the introductory patch which moves
'test_set_editor' into each test.

> And if you insist on doing the line counting in the editor script, then
> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?

Ugh, I utterly forgot about -c.

```

## Pranit Bauva, 2016-03-25 18:15

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAFZEwPPRYBjdDabLJHsXTtzD2y=vqSZM4OiY4G=BtEF485JP_A@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPPRYBjdDabLJHsXTtzD2y%3DvqSZM4OiY4G%3DBtEF485JP_A%40mail.gmail.com
In-Reply-To: <CAPig+cQLOwAimmHeUmi5+KScgOP0hr9-MiX8gyGcEk6Zj0U_jQ@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 10:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

> That works too, simplifying the overall implementation, and
> eliminating the need for the introductory patch which moves
> 'test_set_editor' into each test.

Wouldn't it be cleaner if the introductory patch contain:
1. using write_script()
2. grep the output to a file
3. test for no of lines=1 in required tests
?

Then the patch 3/3
>> And if you insist on doing the line counting in the editor script, then
>> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?
>
> Ugh, I utterly forgot about -c.

```

## Eric Sunshine, 2016-03-25 23:06

Subject: Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script
Message-ID: <CAPig+cTFCB-FMqKPApJw2z8ZWZaE7zJ70kod_kQHvLv2U4=DXA@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcTFCB-FMqKPApJw2z8ZWZaE7zJ70kod_kQHvLv2U4%3DDXA%40mail.gmail.com
In-Reply-To: <CAFZEwPPRYBjdDabLJHsXTtzD2y=vqSZM4OiY4G=BtEF485JP_A@mail.gmail.com>

```
On Fri, Mar 25, 2016 at 2:15 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 10:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> That works too, simplifying the overall implementation, and
>> eliminating the need for the introductory patch which moves
>> 'test_set_editor' into each test.
>
> Wouldn't it be cleaner if the introductory patch contain:
> 1. using write_script()
> 2. grep the output to a file
> 3. test for no of lines=1 in required tests
> ?

You could do that.

```

## Pranit Bauva, 2016-03-28 18:42

Subject: Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <CAFZEwPP5Hq0fM3Fhg+JV-aY6bWozjL6m_bNpb=JhjkUjDJu2sw@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPP5Hq0fM3Fhg%2BJV-aY6bWozjL6m_bNpb%3DJhjkUjDJu2sw%40mail.gmail.com
In-Reply-To: <1458815593-11060-1-git-send-email-szeder@ira.uka.de>

```
> A couple of new tests to t0040-parse-options.sh would be great to
> ensure that starting from a negative value works as advertised, i.e.
> at least that '--option' jumps to 1 and '--no-option' resets to 0.

I think adding tests to t0040-parse-options.sh cannot reflect the
behavior introduced by this patch.
This is because setting the initial value of the variable (which is
going to be modified by the argument) is set in test-parse-options.c .
A possible solution will be to modify the test-parse-options.c and
initialize the required variable (verbose or quiet) to "unspecified"
value. But then this will leave one part of the untested ie. when the
initial value of the variable is 0. I could do one thing to test these
both behaviors which is to set verbose initially to -1 and leave quiet
= 0. Since verbose and quiet are both consumers of OPT_COUNTUP, this
can test both the behaviors.

I tried searching for alternatives but could not find any. Is there
something else which you had thought before that would test this
behavior?

If not, then we are left with 2 options, either modify
test-parse-options.c or just hand test it whenever there seems to be a
problematic case.

```

## Eric Sunshine, 2016-03-28 19:03

Subject: Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
Message-ID: <CAPig+cR63D2L8dXJe9Gx-zFXr4-gNaFC6GdfWNPnt4d=Ds8tWg@mail.gmail.com>
URL: https://gitlist.dev/e/CAPig%2BcR63D2L8dXJe9Gx-zFXr4-gNaFC6GdfWNPnt4d%3DDs8tWg%40mail.gmail.com
In-Reply-To: <CAFZEwPP5Hq0fM3Fhg+JV-aY6bWozjL6m_bNpb=JhjkUjDJu2sw@mail.gmail.com>

```
On Mon, Mar 28, 2016 at 2:42 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> A couple of new tests to t0040-parse-options.sh would be great to
>> ensure that starting from a negative value works as advertised, i.e.
>> at least that '--option' jumps to 1 and '--no-option' resets to 0.
>
> I think adding tests to t0040-parse-options.sh cannot reflect the
> behavior introduced by this patch.
> This is because setting the initial value of the variable (which is
> going to be modified by the argument) is set in test-parse-options.c .
> A possible solution will be to modify the test-parse-options.c and
> initialize the required variable (verbose or quiet) to "unspecified"
> value. But then this will leave one part of the untested ie. when the
> initial value of the variable is 0. I could do one thing to test these
> both behaviors which is to set verbose initially to -1 and leave quiet
> = 0. Since verbose and quiet are both consumers of OPT_COUNTUP, this
> can test both the behaviors.
>
> I tried searching for alternatives but could not find any. Is there
> something else which you had thought before that would test this
> behavior?
>
> If not, then we are left with 2 options, either modify
> test-parse-options.c or just hand test it whenever there seems to be a
> problematic case.

Modifying test-parse-options.c, if needed, was implied by SZEDER's
suggestion to add tests for this new behavior. test-parse-options.c
exists strictly for testing option parsing, so don't feel constrained
about modifying or extending it in order to test the new count-up
behavior if the existing implementation doesn't directly support what
you want it to do.

```