threads / patch / 41807

v9, 3 partst7507-commit-verbose: make test suite use write_script

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

## tl;dr

23 messages between Mar 24, 2016 and Mar 28, 2016. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Pranit Bauva· Mar 24, 2016, 08:25 UTC · lore

[PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

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(-)
Show changes to 2 files +11 −4

Documentation/technical/api-parse-options.txt, parse-options.c

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· Mar 24, 2016, 08:25 UTC · re: Pranit Bauva · lore
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(-)
Show changes to t/t7507-commit-verbose.sh +8 −5
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
SZEDER Gábor· Mar 24, 2016, 11:00 UTC · re: Pranit Bauva · lore
> 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.
Show 18 quoted lines
> ---
>  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.

Show 46 quoted lines
>  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· Mar 24, 2016, 23:57 UTC · re: SZEDER Gábor · lore

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

On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 8 quoted lines
>> 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.

Show 18 quoted lines
>> 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

Pranit Bauva· Mar 25, 2016, 06:06 UTC · re: Eric Sunshine · lore

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

On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 16 quoted lines
> 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
Show 30 quoted lines
> 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.

Show 6 quoted lines
>> 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.
Show 7 quoted lines
>>> -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· Mar 25, 2016, 06:24 UTC · re: Pranit Bauva · lore

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

On Fri, Mar 25, 2016 at 2:06 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 9 quoted lines
> 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...
Show 18 quoted lines
>>>> -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.

Show 5 quoted lines
>>>>  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· Mar 25, 2016, 06:55 UTC · re: Eric Sunshine · lore

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

On Fri, Mar 25, 2016 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 12 quoted lines
> 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."

Show 22 quoted lines
>
>>>>> -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!
Show 13 quoted lines
>>>>>  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· Mar 25, 2016, 14:46 UTC · re: Eric Sunshine · lore

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

Quoting Eric Sunshine <sunshine@sunshineco.com>:
> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 22 quoted lines
>>> 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· Mar 25, 2016, 14:50 UTC · re: SZEDER Gábor · lore

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

On Fri, Mar 25, 2016 at 8:16 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 8 quoted lines
> 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· Mar 25, 2016, 17:04 UTC · re: SZEDER Gábor · lore

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

On Fri, Mar 25, 2016 at 10:46 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 27 quoted lines
> 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· Mar 25, 2016, 18:15 UTC · re: Eric Sunshine · lore

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

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· Mar 25, 2016, 23:06 UTC · re: Pranit Bauva · lore

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

On Fri, Mar 25, 2016 at 2:15 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 10 quoted lines
> 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· Mar 24, 2016, 08:25 UTC · re: Pranit Bauva · lore

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

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(-)
Show changes to 4 files +69 −3

Documentation/config.txt, Documentation/git-commit.txt, builtin/commit.c, t/t7507-commit-verbose.sh

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· Mar 24, 2016, 10:04 UTC · re: Pranit Bauva · lore

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

Show 27 quoted lines
> 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· Mar 24, 2016, 10:22 UTC · re: SZEDER Gábor · lore

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

On Thu, Mar 24, 2016 at 3:34 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 32 quoted lines
>> 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· Mar 24, 2016, 10:26 UTC · re: Pranit Bauva · lore

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

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:
Show 41 quoted lines
> 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.
Eric Sunshine· Mar 25, 2016, 00:05 UTC · re: Pranit Bauva · lore

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

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

Show 6 quoted lines
> 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').

Show 5 quoted lines
> +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· Mar 25, 2016, 06:15 UTC · re: Eric Sunshine · lore

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

On Fri, Mar 25, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 18 quoted lines
> 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.

Show 8 quoted lines
>> +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.
SZEDER Gábor· Mar 24, 2016, 10:33 UTC · re: Pranit Bauva · lore

[PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

Show 23 quoted lines
> 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.

Show 41 quoted lines
> 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· Mar 24, 2016, 10:38 UTC · re: SZEDER Gábor · lore

Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

On Thu, Mar 24, 2016 at 4:03 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 31 quoted lines
>> 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.
Show 50 quoted lines
>> ---
>>  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
>>
>>
Eric Sunshine· Mar 24, 2016, 23:34 UTC · re: SZEDER Gábor · lore

Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

On Thu, Mar 24, 2016 at 6:33 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 24 quoted lines
>> 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.

Show 15 quoted lines
>   - 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.
Pranit Bauva· Mar 28, 2016, 18:42 UTC · re: SZEDER Gábor · lore

Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

> 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· Mar 28, 2016, 19:03 UTC · re: Pranit Bauva · lore

Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values

On Mon, Mar 28, 2016 at 2:42 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 23 quoted lines
>> 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.

← back to recent threads