# [PATCH v10 3/3] commit: add a commit.verbose config variable 35 messages from 2016-03-26 to 2016-03-31. Participants: Pranit Bauva, Eric Sunshine, SZEDER Gábor, Junio C Hamano. Thread: https://gitlist.dev/t/41835 ## Pranit Bauva, 2016-03-26 19:48 Subject: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: <01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com> ``` Add commit.verbose configuration variable as a convenience for those who always prefer --verbose taking care of multiple levels of verbosity. Helped-by: Junio C Hamano Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- The previous version of the patch are: - [v9] $gmane/288820 - [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: - Fixed a status related bug pointed out by SZEDER - Change some tests --- Documentation/config.txt | 4 ++++ Documentation/git-commit.txt | 3 ++- builtin/commit.c | 16 ++++++++++++++- t/t7507-commit-verbose.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++ 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..6a80a38 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; @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) finalize_colopts(&s.colopts, -1); finalize_deferred_config(&s); + if (verbose == -1) + verbose = 0; + handle_untracked_files_arg(&s); if (show_ignored_in_status) s.show_ignored_files = 1; @@ -1505,6 +1510,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 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) argc = parse_and_validate_options(argc, argv, builtin_commit_options, builtin_commit_usage, prefix, current_head, &s); + + if (verbose == -1) + verbose = (config_verbose == -1) ? 0 : config_verbose; + if (dry_run) return dry_run_commit(argc, argv, prefix, current_head, &s); index_file = prepare_index(argc, argv, prefix, current_head, 0); diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index b40eb3c..569fd8b 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' test_i18ngrep "Aborting commit due to empty commit message." err ' +test_expect_success 'commit.verbose true and --verbose omitted' ' + echo content >file2 && + echo content >>file && + git add file2 && + git -c commit.verbose=true commit -F message && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and --verbose' ' + git -c commit.verbose=true commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and -v -v' ' + git -c commit.verbose=true commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose true and --no-verbose' ' + git -c commit.verbose=true commit --amend --no-verbose && + ! test -s out +' + +test_expect_success 'commit.verbose false and --verbose' ' + git -c commit.verbose=false commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose false and -v -v' ' + git -c commit.verbose=false commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose false and --verbose omitted' ' + git -c commit.verbose=false commit --amend && + ! test -s out +' + +test_expect_success 'commit.verbose false and --no-verbose' ' + git -c commit.verbose=false commit --amend --no-verbose && + ! test -s out +' + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-26 19:48 Subject: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: <01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com> ``` So that we can see how many diffs were contained in the message and use them in individual tests where ever it is required. Also use write_script() to create the fake "editor". Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- t/t7507-commit-verbose.sh | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 2ddf28c..b40eb3c 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -3,9 +3,11 @@ test_description='verbose commit template' . ./test-lib.sh -cat >check-for-diff <out EOF chmod +x check-for-diff test_set_editor "$PWD/check-for-diff" @@ -23,7 +25,8 @@ test_expect_success 'setup' ' ' test_expect_success 'initial commit shows verbose diff' ' - git commit --amend -v + git commit --amend -v && + test_line_count = 1 out ' test_expect_success 'second commit' ' @@ -39,13 +42,15 @@ check_message() { test_expect_success 'verbose diff is stripped out' ' git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' git config diff.mnemonicprefix true && git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' cat >diff <<'EOF' -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-26 19:48 Subject: [PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand "unspecified" values Message-ID: <01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com> ``` The reason to make it understand negative values or more specifically "unspecified" values is to give the ability to differentiate whether `--option` or `--no-option` was specified at all. Many uses of COUNTUP have now been replaced with BOOL and what remains are verbose/quiet/force. This change will not affect existing users of COUNTUP at all, as long as they use the initial value of 0 (or more), as there is no mechanism to decrement. The only thing the command line can do is to reset it to zero with "--no-foo". Verbose or quiet don't use negative values before this commit but force uses it in a different way to handle its config and then munges the "-1" into 0 before we get to parse_options. There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c and they are both fine. Helped-by: Jeff King Helped-by: Eric Sunshine Helped-by: Junio C Hamano Signed-off-by: Pranit Bauva --- 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. Also added some points as to where this patch could break but it doesn't. --- Documentation/technical/api-parse-options.txt | 8 ++++++-- parse-options.c | 8 +++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 5f0757d..8908bf7 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -144,8 +144,12 @@ There are some macros to easily define options: `OPT_COUNTUP(short, long, &int_var, description)`:: Introduce a count-up option. - `int_var` is incremented on each use of `--option`, and - reset to zero with `--no-option`. + Each use of `--option` increments `int_var`, starting from zero + (even if initially negative), and `--no-option` resets it to + zero. To determine if `--option` or `--no-option` was set at + all, set `int_var` to a negative value, and if it is still + negative after parse_options(), then neither `--option` nor + `--no-option` was seen. `OPT_BIT(short, long, &int_var, description, mask)`:: Introduce a boolean option. diff --git a/parse-options.c b/parse-options.c index 47a9192..86d30bd 100644 --- a/parse-options.c +++ b/parse-options.c @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, return 0; case OPTION_COUNTUP: - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; + if (unset) + *(int *)opt->value = 0; + else { + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; + *(int *)opt->value += 1; + } return 0; case OPTION_SET_INT: -- https://github.com/git/git/pull/218 ``` ## Eric Sunshine, 2016-03-27 02:45 Subject: Re: [PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTB%2BxZmwALTY2Oy2_dVXknSEjaBiQ8NNr-qOVOZKjHvXw%40mail.gmail.com In-Reply-To: <01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com> ``` On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: > parse-options.c: make OPTION__COUNTUP understand "unspecified" values A bit clearer: s/understand/respect/ Also: s/__/_/ > The reason to make it understand negative values or more specifically > "unspecified" values is to give the ability to differentiate whether > `--option` or `--no-option` was specified at all. The word "negative" shows up far too early in this paragraph, even before "unspecified". It also fails utterly to explain what "unspecified" means and how to use it. It does vaguely explain why respecting "unspecified" is desirable (to know if --[no-]option was seen), but otherwise leaves the reader quite clueless. > Many uses of COUNTUP have now been replaced with BOOL and what remains > are verbose/quiet/force. This change will not affect existing users of > COUNTUP at all, as long as they use the initial value of 0 (or more), as > there is no mechanism to decrement. The only thing the command line can > do is to reset it to zero with "--no-foo". Copying this paragraph verbatim from Junio's email[1] isn't necessarily the best way to compose a commit message. Junio was "thinking out loud" while justifying the change to himself as being safe, but you ought to reframe this reasoning into a more concise form which flows with the rest of the commit message, keeping the important bits and dropping others. > Verbose or quiet don't use negative values before this commit but force > uses it in a different way to handle its config and then munges the "-1" > into 0 before we get to parse_options. Presumably, you're talking about a very specific instance of -1 in conjunction with OPT__FORCE in builtin/clean.c, but that information is entirely missing from this paragraph, thus this explanation serves only to confuse rather than clarify. It is a good idea to cite this specific unusual case when justifying that this change is safe, but it must be accompanied by sufficient context for the reader to understand what is being said. > There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c > and they are both fine. What does "fine" mean? I know from reading the code that "fine" means that these clients initialize the values to 0, thus should see no behavioral difference. But why are these two cases called out specially when they are already covered by the above "as long as they use initial value of 0" explanation? As a reader, having these two cases mentioned specially makes me wonder if I'm missing something non-obvious about them. It would probably be better to avoid mentioning cmd_hash_object() and test-parse-options.c at all, and instead explain generally that, with one exception (builtin/clean.c), all current clients of OPTION_COUNTUP use an initial value of zero, thus are not impacted by this change. And, then go on to explain builtin/clean.c's special case and why it is safe, as well. Sorry for seeming to be very picky, but if I hadn't already been following this topic (and hadn't in fact suggested this particular change), as a reader, I think I would find this commit message utterly baffling, and wouldn't have a clue what this change is about or why it is desirable. Perhaps it would be a good idea to re-read "(2) Describe your changes well" in Documentation/SubmittingPatches for hints about writing a good commit message, as well as Junio's recent re-stating[2] of those hints. Try to write the commit message as if you were speaking to someone who wasn't already familiar with the issue you're trying to solve. Specifically, explain the problem ("no way to distinguish between --[no-]option being seen or not"); explain the solution ("introduce an 'unspecified' value"); the implementation of the "unspecified" value ("any negative number" plus an example of how to make use of it); justify that the change is safe ("existing clients of OPTION_COUNTUP are not impacted because ..."). > Signed-off-by: Pranit Bauva > --- > The discussion about this patch: > [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 > > Previous version of the patch: > [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061 Unless I'm mistaken, the previous version was v9[3], not v1. > Changes introduced: > Use a different language in commit message to make the change and its > utility more clear. Also added some points as to where this patch could > break but it doesn't. This version forgets to add the new tests to t0040-parse-options.sh which SZEDER requested[4] to ensure that the new behavior works as expected. > --- > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt > @@ -144,8 +144,12 @@ There are some macros to easily define options: > > `OPT_COUNTUP(short, long, &int_var, description)`:: > Introduce a count-up option. > - `int_var` is incremented on each use of `--option`, and > - reset to zero with `--no-option`. > + Each use of `--option` increments `int_var`, starting from zero > + (even if initially negative), and `--no-option` resets it to > + zero. To determine if `--option` or `--no-option` was set at > + all, set `int_var` to a negative value, and if it is still I realize that you copied this text verbatim from the example I gave[5], but in retrospect, I think s/set/initialize/ would be a bit more clear: all, initialize `int_var` to a negative value, ... > + negative after parse_options(), then neither `--option` nor > + `--no-option` was seen. [1]: http://article.gmane.org/gmane.comp.version-control.git/289264/ [2]: http://article.gmane.org/gmane.comp.version-control.git/289757/ [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724 [4]: http://article.gmane.org/gmane.comp.version-control.git/289733 [5]: http://article.gmane.org/gmane.comp.version-control.git/289822 ``` ## Eric Sunshine, 2016-03-27 03:07 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRQ5hOLi3539Emq%3DYexHRXqdesf%2BYo2cY%2BNkiPO_9Vhyg%40mail.gmail.com In-Reply-To: <01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000@eu-west-1.amazonses.com> ``` On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: > So that we can see how many diffs were contained in the message and use > them in individual tests where ever it is required. Also use > write_script() to create the fake "editor". It is important to explain *why* you want to be able to count how many diffs were in the editor message. In particular, you will be adding new tests in a subsequent patch which will expect a specific number of diffs (rather than any number of diffs) Also, you need to explain why you're changing the existing tests to count the number of diffs. Is it simply "because you can" or is it because you suspect that a change you're making in a subsequent patch might accidentally cause too many diffs to show up in the existing test cases? > Signed-off-by: Pranit Bauva > --- > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > @@ -3,9 +3,11 @@ > -cat >check-for-diff < -#!$SHELL_PATH > -exec grep '^diff --git' "\$1" > +write_script "check-for-diff" <<'EOF' && The 'EOF' is more commonly written as \EOF in Git test scripts. > +! test -s out || > +rm out && Why not just "rm -f out"? But, more importantly, why do you need to remove the file at all? The '>' redirection operator (used below) will overwrite the file, so no need to remove it beforehand. > +! grep '^diff --git' "$1" || > +grep '^diff --git' "$1" >out Um, what? Why two greps? I would have expected you to simply re-use the existing grep (minus the backslash) while adding the redirection: -exec grep '^diff --git' "\$1" +exec grep '^diff --git' "$1" >out Am I missing something obvious? > EOF > chmod +x check-for-diff Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you. [1]: http://article.gmane.org/gmane.comp.version-control.git/289832 > test_set_editor "$PWD/check-for-diff" > @@ -23,7 +25,8 @@ test_expect_success 'setup' ' > test_expect_success 'initial commit shows verbose diff' ' > - git commit --amend -v > + git commit --amend -v && > + test_line_count = 1 out > ' > > test_expect_success 'second commit' ' > @@ -39,13 +42,15 @@ check_message() { > > test_expect_success 'verbose diff is stripped out' ' > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' > > test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' > git config diff.mnemonicprefix true && > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' ``` ## Eric Sunshine, 2016-03-27 03:34 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTBxH7BAs2M%2BPBwTHfVJvH0ADL1OSEKdHQY59L-WiYosA%40mail.gmail.com In-Reply-To: <01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com> ``` On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: > Add commit.verbose configuration variable as a convenience for those > who always prefer --verbose taking care of multiple levels of verbosity. What does "taking care of multiple levels of verbosity" mean? I suppose you mean that commit.verbose specifies a verbosity level (rather than being merely boolean), but that phrase is difficult to decipher. And, since it's obvious from the patch itself that verbosity is not a mere boolean, there isn't really a need to mention it here. The commit message would be perfectly fine without that bit, so perhaps just drop it. > Signed-off-by: Pranit Bauva > > --- > Changes with respect to the previous patch: > - Fixed a status related bug pointed out by SZEDER > - Change some tests Please help reviewers out by being a bit more verbose when describing the changes. For instance, what bug did you fix pointed out by SZEDER? And, "change some tests" says nothing useful. What did you change in the tests? > --- > diff --git a/builtin/commit.c b/builtin/commit.c > @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) > finalize_colopts(&s.colopts, -1); > finalize_deferred_config(&s); > > + if (verbose == -1) > + verbose = 0; > + Nit: It might be good to drop the blank line above this new conditional to make it clear that it is part of the init_config/parse_options processing (the tail of which is visible in the context above). > handle_untracked_files_arg(&s); > if (show_ignored_in_status) > s.show_ignored_files = 1; > @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > argc = parse_and_validate_options(argc, argv, builtin_commit_options, > builtin_commit_usage, > prefix, current_head, &s); > + > + if (verbose == -1) > + verbose = (config_verbose == -1) ? 0 : config_verbose; > + Nit: For consistency, dropping the blank line above this new conditional wouldn't hurt either. > if (dry_run) > return dry_run_commit(argc, argv, prefix, current_head, &s); > index_file = prepare_index(argc, argv, prefix, current_head, 0); > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' > +test_expect_success 'commit.verbose true and --verbose omitted' ' > + echo content >file2 && > + echo content >>file && > + git add file2 && > + git -c commit.verbose=true commit -F message && > + test_line_count = 1 out > +' Why is this test so utterly different than it was in v9 (even though the title is the same), and why is it so different from other tests below? More below... > +test_expect_success 'commit.verbose true and --verbose' ' > + git -c commit.verbose=true commit --amend --verbose && > + test_line_count = 1 out > +' > + > +test_expect_success 'commit.verbose true and -v -v' ' > + git -c commit.verbose=true commit --amend -v -v && > + test_line_count = 2 out > +' > + > +test_expect_success 'commit.verbose true and --no-verbose' ' > + git -c commit.verbose=true commit --amend --no-verbose && > + ! test -s out > +' > + > +test_expect_success 'commit.verbose false and --verbose' ' > + git -c commit.verbose=false commit --amend --verbose && > + test_line_count = 1 out > +' > + > +test_expect_success 'commit.verbose false and -v -v' ' > + git -c commit.verbose=false commit --amend -v -v && > + test_line_count = 2 out > +' > + > +test_expect_success 'commit.verbose false and --verbose omitted' ' > + git -c commit.verbose=false commit --amend && > + ! test -s out > +' > + > +test_expect_success 'commit.verbose false and --no-verbose' ' > + git -c commit.verbose=false commit --amend --no-verbose && > + ! test -s out > +' > + > +test_expect_success 'status ignores commit.verbose=true' ' > + git -c commit.verbose=true status >actual && > + ! grep "^diff --git" actual > +' The fact that v9 broke a number of tests in other scripts which use git-status (as SZEDER pointed out[1]), due to initializing 'verbose' to -1 in builtin/commit.c, suggests that you probably ought to add another test here to protect against that sort of breakage, as well. Specifically, git-status should remain non-verbose when neither commit.verbose nor --verbose is specified. > + > test_done ``` ## Pranit Bauva, 2016-03-27 06:10 Subject: Re: [PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAFZEwPNGVihCctwq%2BHGnEsAitn7J6jwm7mm7bOQEuhrENKzuuw%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 27, 2016 at 8:15 AM, Eric Sunshine wrote: > On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >> parse-options.c: make OPTION__COUNTUP understand "unspecified" values > > A bit clearer: s/understand/respect/ > Also: s/__/_/ Sure. >> The reason to make it understand negative values or more specifically >> "unspecified" values is to give the ability to differentiate whether >> `--option` or `--no-option` was specified at all. > > The word "negative" shows up far too early in this paragraph, even > before "unspecified". It also fails utterly to explain what > "unspecified" means and how to use it. It does vaguely explain why > respecting "unspecified" is desirable (to know if --[no-]option was > seen), but otherwise leaves the reader quite clueless. > >> Many uses of COUNTUP have now been replaced with BOOL and what remains >> are verbose/quiet/force. This change will not affect existing users of >> COUNTUP at all, as long as they use the initial value of 0 (or more), as >> there is no mechanism to decrement. The only thing the command line can >> do is to reset it to zero with "--no-foo". > > Copying this paragraph verbatim from Junio's email[1] isn't > necessarily the best way to compose a commit message. Junio was > "thinking out loud" while justifying the change to himself as being > safe, but you ought to reframe this reasoning into a more concise form > which flows with the rest of the commit message, keeping the important > bits and dropping others. I will drop off some initial parts. >> Verbose or quiet don't use negative values before this commit but force >> uses it in a different way to handle its config and then munges the "-1" >> into 0 before we get to parse_options. > > Presumably, you're talking about a very specific instance of -1 in > conjunction with OPT__FORCE in builtin/clean.c, but that information > is entirely missing from this paragraph, thus this explanation serves > only to confuse rather than clarify. It is a good idea to cite this > specific unusual case when justifying that this change is safe, but it > must be accompanied by sufficient context for the reader to understand > what is being said. I will mention about OPT__FORCE in builtin/clean.c clearly. >> There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c >> and they are both fine. > > What does "fine" mean? I know from reading the code that "fine" means > that these clients initialize the values to 0, thus should see no > behavioral difference. But why are these two cases called out > specially when they are already covered by the above "as long as they > use initial value of 0" explanation? As a reader, having these two > cases mentioned specially makes me wonder if I'm missing something > non-obvious about them. Its good to drop off this paragraph > It would probably be better to avoid mentioning cmd_hash_object() and > test-parse-options.c at all, and instead explain generally that, with > one exception (builtin/clean.c), all current clients of OPTION_COUNTUP > use an initial value of zero, thus are not impacted by this change. > And, then go on to explain builtin/clean.c's special case and why it > is safe, as well. > > > Sorry for seeming to be very picky, but if I hadn't already been > following this topic (and hadn't in fact suggested this particular > change), as a reader, I think I would find this commit message utterly > baffling, and wouldn't have a clue what this change is about or why it > is desirable. > > Perhaps it would be a good idea to re-read "(2) Describe your changes > well" in Documentation/SubmittingPatches for hints about writing a > good commit message, as well as Junio's recent re-stating[2] of those > hints. I don't disagree to your point and it is a bit difficult for me to explain stuff to other people especially when I know what's going on but other might not. > Try to write the commit message as if you were speaking to someone who > wasn't already familiar with the issue you're trying to solve. > Specifically, explain the problem ("no way to distinguish between > --[no-]option being seen or not"); explain the solution ("introduce an > 'unspecified' value"); the implementation of the "unspecified" value > ("any negative number" plus an example of how to make use of it); > justify that the change is safe ("existing clients of OPTION_COUNTUP > are not impacted because ..."). I will try and do this in the commit message. >> Signed-off-by: Pranit Bauva >> --- >> The discussion about this patch: >> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 >> >> Previous version of the patch: >> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061 > > Unless I'm mistaken, the previous version was v9[3], not v1. > >> Changes introduced: >> Use a different language in commit message to make the change and its >> utility more clear. Also added some points as to where this patch could >> break but it doesn't. > > This version forgets to add the new tests to t0040-parse-options.sh > which SZEDER requested[4] to ensure that the new behavior works as > expected. > >> --- >> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt >> @@ -144,8 +144,12 @@ There are some macros to easily define options: >> >> `OPT_COUNTUP(short, long, &int_var, description)`:: >> Introduce a count-up option. >> - `int_var` is incremented on each use of `--option`, and >> - reset to zero with `--no-option`. >> + Each use of `--option` increments `int_var`, starting from zero >> + (even if initially negative), and `--no-option` resets it to >> + zero. To determine if `--option` or `--no-option` was set at >> + all, set `int_var` to a negative value, and if it is still > > I realize that you copied this text verbatim from the example I > gave[5], but in retrospect, I think s/set/initialize/ would be a bit > more clear: > > all, initialize `int_var` to a negative value, ... > >> + negative after parse_options(), then neither `--option` nor >> + `--no-option` was seen. > > [1]: http://article.gmane.org/gmane.comp.version-control.git/289264/ > [2]: http://article.gmane.org/gmane.comp.version-control.git/289757/ > [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724 > [4]: http://article.gmane.org/gmane.comp.version-control.git/289733 > [5]: http://article.gmane.org/gmane.comp.version-control.git/289822 ``` ## Pranit Bauva, 2016-03-27 07:00 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPPNMtopk9a3nVaCuujWoyeA5%2B39vEj3Mct1YckCXLh1-A%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote: > On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >> Add commit.verbose configuration variable as a convenience for those >> who always prefer --verbose taking care of multiple levels of verbosity. > > What does "taking care of multiple levels of verbosity" mean? I > suppose you mean that commit.verbose specifies a verbosity level > (rather than being merely boolean), but that phrase is difficult to > decipher. And, since it's obvious from the patch itself that verbosity > is not a mere boolean, there isn't really a need to mention it here. > The commit message would be perfectly fine without that bit, so > perhaps just drop it. I will drop it. >> Signed-off-by: Pranit Bauva >> >> --- >> Changes with respect to the previous patch: >> - Fixed a status related bug pointed out by SZEDER >> - Change some tests > > Please help reviewers out by being a bit more verbose when describing > the changes. For instance, what bug did you fix pointed out by SZEDER? > And, "change some tests" says nothing useful. What did you change in > the tests? I will try and be more specific henceforth. >> --- >> diff --git a/builtin/commit.c b/builtin/commit.c >> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) >> finalize_colopts(&s.colopts, -1); >> finalize_deferred_config(&s); >> >> + if (verbose == -1) >> + verbose = 0; >> + > > Nit: It might be good to drop the blank line above this new > conditional to make it clear that it is part of the > init_config/parse_options processing (the tail of which is visible in > the context above). Sure. >> handle_untracked_files_arg(&s); >> if (show_ignored_in_status) >> s.show_ignored_files = 1; >> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> argc = parse_and_validate_options(argc, argv, builtin_commit_options, >> builtin_commit_usage, >> prefix, current_head, &s); >> + >> + if (verbose == -1) >> + verbose = (config_verbose == -1) ? 0 : config_verbose; >> + > > Nit: For consistency, dropping the blank line above this new > conditional wouldn't hurt either. > >> if (dry_run) >> return dry_run_commit(argc, argv, prefix, current_head, &s); >> index_file = prepare_index(argc, argv, prefix, current_head, 0); >> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' >> +test_expect_success 'commit.verbose true and --verbose omitted' ' >> + echo content >file2 && >> + echo content >>file && >> + git add file2 && >> + git -c commit.verbose=true commit -F message && >> + test_line_count = 1 out >> +' > > Why is this test so utterly different than it was in v9 (even though > the title is the same), and why is it so different from other tests > below? This is because the "editor" in v9 checked for "# Changes"... While this "editor" checks for 'diff --git'. And submodules don't give a proper diff to verify (I tried this out and noticed this behavior by tweaking some parts). In fact submodules don't give diff at all. But they do give "# Changes"... So its important to setup up a little before getting started. If this seems unnecessary, then should I move all the tests which were introduced here above the submodule test? > More below... > >> +test_expect_success 'commit.verbose true and --verbose' ' >> + git -c commit.verbose=true commit --amend --verbose && >> + test_line_count = 1 out >> +' >> + >> +test_expect_success 'commit.verbose true and -v -v' ' >> + git -c commit.verbose=true commit --amend -v -v && >> + test_line_count = 2 out >> +' >> + >> +test_expect_success 'commit.verbose true and --no-verbose' ' >> + git -c commit.verbose=true commit --amend --no-verbose && >> + ! test -s out >> +' >> + >> +test_expect_success 'commit.verbose false and --verbose' ' >> + git -c commit.verbose=false commit --amend --verbose && >> + test_line_count = 1 out >> +' >> + >> +test_expect_success 'commit.verbose false and -v -v' ' >> + git -c commit.verbose=false commit --amend -v -v && >> + test_line_count = 2 out >> +' >> + >> +test_expect_success 'commit.verbose false and --verbose omitted' ' >> + git -c commit.verbose=false commit --amend && >> + ! test -s out >> +' >> + >> +test_expect_success 'commit.verbose false and --no-verbose' ' >> + git -c commit.verbose=false commit --amend --no-verbose && >> + ! test -s out >> +' >> + >> +test_expect_success 'status ignores commit.verbose=true' ' >> + git -c commit.verbose=true status >actual && >> + ! grep "^diff --git" actual >> +' > > The fact that v9 broke a number of tests in other scripts which use > git-status (as SZEDER pointed out[1]), due to initializing 'verbose' > to -1 in builtin/commit.c, suggests that you probably ought to add > another test here to protect against that sort of breakage, as well. > Specifically, git-status should remain non-verbose when neither > commit.verbose nor --verbose is specified. Yes, I should. I will include that. >> + >> test_done ``` ## Eric Sunshine, 2016-03-27 08:17 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcStkyOE4MwmFpq9V8h5M%3D6j%3DFWbDTkyMGXZsf8sBH%3D9pg%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 27, 2016 at 3:00 AM, Pranit Bauva wrote: > On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote: >> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >>> +test_expect_success 'commit.verbose true and --verbose omitted' ' >>> + echo content >file2 && >>> + echo content >>file && >>> + git add file2 && >>> + git -c commit.verbose=true commit -F message && >>> + test_line_count = 1 out >>> +' >> >> Why is this test so utterly different than it was in v9 (even though >> the title is the same), and why is it so different from other tests >> below? > > This is because the "editor" in v9 checked for "# Changes"... While > this "editor" checks for 'diff --git'. And submodules don't give a > proper diff to verify (I tried this out and noticed this behavior by > tweaking some parts). In fact submodules don't give diff at all. But > they do give "# Changes"... So its important to setup up a little > before getting started. If this seems unnecessary, then should I move > all the tests which were introduced here above the submodule test? Let's ignore submodules when discussing this since they don't need to factor into the issue. What you are actually saying (and what took me a while to understand due to the "submodules" misdirection) is that you need to do some additional setup to test the "-v -v" cases. In particular, you need to introduce some change to the worktree which is not in the index. The typical way to satisfy this requirement (which doesn't require relocating tests) is to add a "setup" test before the tests which depend upon that additional setup, rather than adding that setup to the first test which needs it. Just about the simplest setup test which satisfies your needs is the following (inserted just before the first of the new tests): test_expect_success 'setup -v -v' ' echo dirty >file ' And, then you can restore the "commit.verbose true and --verbose omitted" test to its simple form: test_expect_success 'commit.verbose true and --verbose omitted' ' git -c commit.verbose=true commit --amend && test_line_count = 1 out ' By the way, now that commit.verbose is no longer a mere boolean, you're going to need some additional tests beyond the commit.verbose={true,false} ones you've already added. In particular, you should be testing commit.verbose with several numeric values to verify that it works as expected. For instance: commit.verbose=-2 commit.verbose=-1 commit.verbose=0 commit.verbose=1 commit.verbose=2 commit.verbose=3 The -2 case is interesting; I'm pretty sure the current implementation of this patch will misbehave since the only negative value it's ``` ## Pranit Bauva, 2016-03-27 08:40 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPMSpJGgTeHm41FYXiXgx7whFT0BsqUEBJ2ZJOsfydKxVw%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 27, 2016 at 1:47 PM, Eric Sunshine wrote: > On Sun, Mar 27, 2016 at 3:00 AM, Pranit Bauva wrote: >> On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote: >>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >>>> +test_expect_success 'commit.verbose true and --verbose omitted' ' >>>> + echo content >file2 && >>>> + echo content >>file && >>>> + git add file2 && >>>> + git -c commit.verbose=true commit -F message && >>>> + test_line_count = 1 out >>>> +' >>> >>> Why is this test so utterly different than it was in v9 (even though >>> the title is the same), and why is it so different from other tests >>> below? >> >> This is because the "editor" in v9 checked for "# Changes"... While >> this "editor" checks for 'diff --git'. And submodules don't give a >> proper diff to verify (I tried this out and noticed this behavior by >> tweaking some parts). In fact submodules don't give diff at all. But >> they do give "# Changes"... So its important to setup up a little >> before getting started. If this seems unnecessary, then should I move >> all the tests which were introduced here above the submodule test? > > Let's ignore submodules when discussing this since they don't need to > factor into the issue. What you are actually saying (and what took me > a while to understand due to the "submodules" misdirection) is that > you need to do some additional setup to test the "-v -v" cases. In > particular, you need to introduce some change to the worktree which is > not in the index. Sorry for the misdirection. And you understood correctly. I do need to introduce some changes in worktree. > The typical way to satisfy this requirement (which doesn't require > relocating tests) is to add a "setup" test before the tests which > depend upon that additional setup, rather than adding that setup to > the first test which needs it. Just about the simplest setup test > which satisfies your needs is the following (inserted just before the > first of the new tests): > > test_expect_success 'setup -v -v' ' > echo dirty >file > ' > > And, then you can restore the "commit.verbose true and --verbose > omitted" test to its simple form: > > test_expect_success 'commit.verbose true and --verbose omitted' ' > git -c commit.verbose=true commit --amend && > test_line_count = 1 out > ' Having a additional setup test seems a nice way to go about. > By the way, now that commit.verbose is no longer a mere boolean, > you're going to need some additional tests beyond the > commit.verbose={true,false} ones you've already added. In particular, > you should be testing commit.verbose with several numeric values to > verify that it works as expected. For instance: > > commit.verbose=-2 > commit.verbose=-1 > commit.verbose=0 > commit.verbose=1 > commit.verbose=2 > commit.verbose=3 > > The -2 case is interesting; I'm pretty sure the current implementation > of this patch will misbehave since the only negative value it's > expecting 'config_verbose' to be is -1. -2 case will fail. I should probably expect 'config_verbose' to be negative instead. ``` ## SZEDER Gábor, 2016-03-27 11:51 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: <1459079497-19121-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1459079497-19121-1-git-send-email-szeder%40ira.uka.de In-Reply-To: <01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com> ``` > +test_expect_success 'commit.verbose true and --no-verbose' ' > + git -c commit.verbose=true commit --amend --no-verbose && > + ! test -s out Please use the test_must_be_empty helper instead, because it has a nice, human-readable name and it complains with a helpful error message if something goes wrong, whereas 'test -s' just fails silently. > +' > + > +test_expect_success 'commit.verbose false and --verbose' ' > + git -c commit.verbose=false commit --amend --verbose && > + test_line_count = 1 out > +' > + > +test_expect_success 'commit.verbose false and -v -v' ' > + git -c commit.verbose=false commit --amend -v -v && > + test_line_count = 2 out > +' > + > +test_expect_success 'commit.verbose false and --verbose omitted' ' > + git -c commit.verbose=false commit --amend && > + ! test -s out > +' > + > +test_expect_success 'commit.verbose false and --no-verbose' ' > + git -c commit.verbose=false commit --amend --no-verbose && > + ! test -s out > +' > + > +test_expect_success 'status ignores commit.verbose=true' ' > + git -c commit.verbose=true status >actual && > + ! grep "^diff --git" actual > +' > + > test_done > > -- > https://github.com/git/git/pull/218 > > ``` ## Pranit Bauva, 2016-03-27 11:59 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPOhTC_LjUPHMN0PJsc94gvmHgmQL2Xe63oRGOiEMR2jjQ%40mail.gmail.com In-Reply-To: <1459079497-19121-1-git-send-email-szeder@ira.uka.de> ``` On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote: >> +test_expect_success 'commit.verbose true and --no-verbose' ' >> + git -c commit.verbose=true commit --amend --no-verbose && >> + ! test -s out > > Please use the test_must_be_empty helper instead, because it has a > nice, human-readable name and it complains with a helpful error > message if something goes wrong, whereas 'test -s' just fails > silently. Thanks for pointing it out. I was unsure whether 'test -s' is a good choice but used it since I did not know any other alternative. ``` ## SZEDER Gábor, 2016-03-27 12:07 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: <20160327140733.Horde.hQiEcpos_hNprvjPS1zIbi2@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20160327140733.Horde.hQiEcpos_hNprvjPS1zIbi2%40webmail.informatik.kit.edu In-Reply-To: ``` Quoting Pranit Bauva : > On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote: >>> +test_expect_success 'commit.verbose true and --no-verbose' ' >>> + git -c commit.verbose=true commit --amend --no-verbose && >>> + ! test -s out >> >> Please use the test_must_be_empty helper instead, because it has a >> nice, human-readable name and it complains with a helpful error >> message if something goes wrong, whereas 'test -s' just fails >> silently. > > Thanks for pointing it out. I was unsure whether 'test -s' is a good > choice but used it since I did not know any other alternative. t/test-lib-functions.sh contains all our test helpers functions, that's where you can look for a suitable helper, should it be necessary. ``` ## Pranit Bauva, 2016-03-27 12:11 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPPwz5Woz-TXiDQz43VChueDGqzOPB0VQQiefH7igWNiQQ%40mail.gmail.com In-Reply-To: <20160327140733.Horde.hQiEcpos_hNprvjPS1zIbi2@webmail.informatik.kit.edu> ``` On Sun, Mar 27, 2016 at 5:37 PM, SZEDER Gábor wrote: > t/test-lib-functions.sh contains all our test helpers functions, that's > where you can look for a suitable helper, should it be necessary. Thanks. I will surely look into it. ``` ## SZEDER Gábor, 2016-03-27 13:27 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: <1459085245-20658-1-git-send-email-szeder@ira.uka.de> URL: https://gitlist.dev/e/1459085245-20658-1-git-send-email-szeder%40ira.uka.de In-Reply-To: ``` > > +! test -s out || > > +rm out && > > Why not just "rm -f out"? But, more importantly, why do you need to > remove the file at all? The '>' redirection operator (used below) will > overwrite the file, so no need to remove it beforehand. > > > +! grep '^diff --git' "$1" || > > +grep '^diff --git' "$1" >out > > Um, what? Why two greps? I would have expected you to simply re-use > the existing grep (minus the backslash) while adding the redirection: > > -exec grep '^diff --git' "\$1" > +exec grep '^diff --git' "$1" >out > > Am I missing something obvious? In the non-verbose cases no diff is included in the commit message template, thus the pattern looking for it doesn't match anything, grep exits with error code, which in turn becomes the editor's exit code, ultimately making 'git commit' fail. Not good. I suppose both the explicit 'rm out' and the '! grep ... || ...' is there to deal with this situation. Hmph. I think we could: - either revive the idea of two editor scripts: one for the non-verbose case checking with '! grep ...' that there are no diffs in the commit message template, and one for all verbose cases storing those diff lines in a file to be counted later. - or use a fake editor that merely copies the whole commit message template to a separate file, and we do the greping in the tests themselves as well. - or simply stick a 'true' at the end of the editor script ensuring that it returns success even when grep can't find the pattern, but I kind of feel ashamed of myself for even mentioning this possibility ;) I would go for the second possibility, but don't feel strong about it. ``` ## Pranit Bauva, 2016-03-27 13:43 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAFZEwPN_sL7_yj%3DFcHynK2L5DhuSD5XSGHnGk73yAcH%3D%2BMdY3w%40mail.gmail.com In-Reply-To: <1459085245-20658-1-git-send-email-szeder@ira.uka.de> ``` On Sun, Mar 27, 2016 at 6:57 PM, SZEDER Gábor wrote: >> > +! test -s out || >> > +rm out && >> >> Why not just "rm -f out"? But, more importantly, why do you need to >> remove the file at all? The '>' redirection operator (used below) will >> overwrite the file, so no need to remove it beforehand. >> >> > +! grep '^diff --git' "$1" || >> > +grep '^diff --git' "$1" >out >> >> Um, what? Why two greps? I would have expected you to simply re-use >> the existing grep (minus the backslash) while adding the redirection: >> >> -exec grep '^diff --git' "\$1" >> +exec grep '^diff --git' "$1" >out >> >> Am I missing something obvious? > > In the non-verbose cases no diff is included in the commit message > template, thus the pattern looking for it doesn't match anything, grep > exits with error code, which in turn becomes the editor's exit > code, ultimately making 'git commit' fail. Not good. > > I suppose both the explicit 'rm out' and the '! grep ... || ...' is > there to deal with this situation. Yes. In fact, I did this as a last resort after trying a lot of other stuff which didn't work. > Hmph. > > I think we could: > > - either revive the idea of two editor scripts: one for the > non-verbose case checking with '! grep ...' that there are no > diffs in the commit message template, and one for all verbose > cases storing those diff lines in a file to be counted later. > > - or use a fake editor that merely copies the whole commit message > template to a separate file, and we do the greping in the tests > themselves as well. > > - or simply stick a 'true' at the end of the editor script ensuring > that it returns success even when grep can't find the pattern, but > I kind of feel ashamed of myself for even mentioning this > possibility ;) > > I would go for the second possibility, but don't feel strong about it. There is one more possibility, that we could use 'test_must_fail' (as grep exits with error code with no diff) but this will send a very wrong interpretation as "This type of scenario is not meant to work". Or to put it in better words, "You cannot have no diff with commit". There is a difference between two negatives and one positive. ``` ## Eric Sunshine, 2016-03-27 16:48 Subject: Re: [PATCH v10 3/3] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRU%2BJJabUdNTVDtZsrmWgn0TWetxhCFpyR%2B%3D5LK-w-7XA%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 27, 2016 at 7:59 AM, Pranit Bauva wrote: > On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote: >>> +test_expect_success 'commit.verbose true and --no-verbose' ' >>> + git -c commit.verbose=true commit --amend --no-verbose && >>> + ! test -s out >> >> Please use the test_must_be_empty helper instead, because it has a >> nice, human-readable name and it complains with a helpful error >> message if something goes wrong, whereas 'test -s' just fails >> silently. > > Thanks for pointing it out. I was unsure whether 'test -s' is a good > choice but used it since I did not know any other alternative. For these particular tests, it would be more stylized to use: test_line_count = 0 out which would keep them consistent with other tests in the same file which use: test_line_count = 1 out ... test_line_count = 2 out however, that's a minor point. ``` ## Eric Sunshine, 2016-03-27 17:03 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQucLoednXHatos0yF66JGN%2BnjhLarK%3DWYam%3DqtwV5y%3DQ%40mail.gmail.com In-Reply-To: ``` [forwarding this to the list since Pranit (presumably) accidentally replied only to me but it's relevant to the ongoing discussion] On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva wrote: > On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine wrote: >> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >>> So that we can see how many diffs were contained in the message and use >>> them in individual tests where ever it is required. Also use >>> write_script() to create the fake "editor". >> >> It is important to explain *why* you want to be able to count how many >> diffs were in the editor message. In particular, you will be adding >> new tests in a subsequent patch which will expect a specific number of >> diffs (rather than any number of diffs) >> >> Also, you need to explain why you're changing the existing tests to >> count the number of diffs. Is it simply "because you can" or is it >> because you suspect that a change you're making in a subsequent patch >> might accidentally cause too many diffs to show up in the existing >> test cases? > > Sorry for not writing commit messages properly. It is a part I am working on. > How does this paragraph sound to you in addition to the previous commit message? > "A subsequent commit will introduce a scenario where it is important > to be able to exactly determine how many diffs were present." > >>> Signed-off-by: Pranit Bauva >>> --- >>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >>> @@ -3,9 +3,11 @@ >>> -cat >check-for-diff <>> -#!$SHELL_PATH >>> -exec grep '^diff --git' "\$1" >>> +write_script "check-for-diff" <<'EOF' && >> >> The 'EOF' is more commonly written as \EOF in Git test scripts. >> >>> +! test -s out || >>> +rm out && >> >> Why not just "rm -f out"? But, more importantly, why do you need to >> remove the file at all? The '>' redirection operator (used below) will >> overwrite the file, so no need to remove it beforehand. > > I wasn't aware about '-f' option. It is important to remove the file. > I initially tried without removing the file and it caused problems. > This is because let's say the previous test had 1 diff which was > stored in out. Now, if in the next test, it is expected to have 0 > diffs then grep would fail to execute and nothing will be re-written > to out and out will contain the 1 diff from previous test. An > explanation for this should be mentioned in the commit message? > >>> +! grep '^diff --git' "$1" || >>> +grep '^diff --git' "$1" >out >> >> Um, what? Why two greps? I would have expected you to simply re-use >> the existing grep (minus the backslash) while adding the redirection: >> >> -exec grep '^diff --git' "\$1" >> +exec grep '^diff --git' "$1" >out > > The reason of two greps in that if in some test it fails to find any > diffs, the editor will return an error. Of course we could test this > scenario by using test_must_fail as in previous patches, but I think > it would be more clearer if we could test for the existence of out > which is done in patch 3/3. I will explanation for this in commit > message. > >>> EOF >>> chmod +x check-for-diff >> >> Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you. >> >> [1]: http://article.gmane.org/gmane.comp.version-control.git/289832 > > Then you mentioned that you were talking[1] about some other review. > So I thought you mean to keep as it is. I guess I misinterpreted it. > And I did not test that before. Now, I have tested that it works if we > remove chmod. > [1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289832 > >>> test_set_editor "$PWD/check-for-diff" >>> @@ -23,7 +25,8 @@ test_expect_success 'setup' ' >>> test_expect_success 'initial commit shows verbose diff' ' >>> - git commit --amend -v >>> + git commit --amend -v && >>> + test_line_count = 1 out >>> ' >>> >>> test_expect_success 'second commit' ' >>> @@ -39,13 +42,15 @@ check_message() { >>> >>> test_expect_success 'verbose diff is stripped out' ' >>> git commit --amend -v && >>> - check_message message >>> + check_message message && >>> + test_line_count = 1 out >>> ' >>> >>> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' >>> git config diff.mnemonicprefix true && >>> git commit --amend -v && >>> - check_message message >>> + check_message message && >>> + test_line_count = 1 out >>> ' ``` ## Eric Sunshine, 2016-03-27 17:05 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQoRW3_0HPv7aUZFgAu41vkR73Z7Z%2BhpfrXZymEAvDFMg%40mail.gmail.com In-Reply-To: ``` [also forwarding this to the list since it's relevant to the ongoing discussion, and I hadn't noticed when replying that Pranit had (presumably) accidentally dropped the git list as a recipient] On Sun, Mar 27, 2016 at 3:10 AM, Eric Sunshine wrote: > On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva wrote: >> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine wrote: >>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote: >>> It is important to explain *why* you want to be able to count how many >>> diffs were in the editor message. In particular, you will be adding >>> new tests in a subsequent patch which will expect a specific number of >>> diffs (rather than any number of diffs) >>> >>> Also, you need to explain why you're changing the existing tests to >>> count the number of diffs. Is it simply "because you can" or is it >>> because you suspect that a change you're making in a subsequent patch >>> might accidentally cause too many diffs to show up in the existing >>> test cases? >> >> Sorry for not writing commit messages properly. It is a part I am working on. >> How does this paragraph sound to you in addition to the previous commit message? >> "A subsequent commit will introduce a scenario where it is important >> to be able to exactly determine how many diffs were present." > > That's fine for explaining why 'check-for-diff' is being updated to > store the output, but you still need to explain why you're changing > the existing tests. > >>>> -cat >check-for-diff <>>> -#!$SHELL_PATH >>>> -exec grep '^diff --git' "\$1" >>>> +write_script "check-for-diff" <<'EOF' && >>>> +! test -s out || >>>> +rm out && >>> >>> Why not just "rm -f out"? But, more importantly, why do you need to >>> remove the file at all? The '>' redirection operator (used below) will >>> overwrite the file, so no need to remove it beforehand. >> >> I wasn't aware about '-f' option. It is important to remove the file. >> I initially tried without removing the file and it caused problems. >> This is because let's say the previous test had 1 diff which was >> stored in out. Now, if in the next test, it is expected to have 0 >> diffs then grep would fail to execute and nothing will be re-written >> to out and out will contain the 1 diff from previous test. An >> explanation for this should be mentioned in the commit message? > > No. Rather than trying to explain all this in the commit message, it > would be better to retain a simple implementation of 'check-for-diff' > rather than adding several levels of complication. More about this > below... > >>>> +! grep '^diff --git' "$1" || >>>> +grep '^diff --git' "$1" >out >>> >>> Um, what? Why two greps? I would have expected you to simply re-use >>> the existing grep (minus the backslash) while adding the redirection: >>> >>> -exec grep '^diff --git' "\$1" >>> +exec grep '^diff --git' "$1" >out >> >> The reason of two greps in that if in some test it fails to find any >> diffs, the editor will return an error. Of course we could test this >> scenario by using test_must_fail as in previous patches, but I think >> it would be more clearer if we could test for the existence of out >> which is done in patch 3/3. I will explanation for this in commit >> message. > > This sounds unnecessarily complicated. It's not too difficult to > reason about a script named 'check-for-diff' actually "checking for > presence of diffs" and failing if no diff is present, from which it > follows naturally that new tests which don't expect any diffs would > use test_must_fail. Keeping it simple like this also makes this patch > much less noisy since it doesn't require changing existing tests. > > Likewise, it keeps most of the new tests in 3/3 small because the bulk > of them only want to know that a diff was present, but don't care > about the number of diffs. However, it's not necessarily a bad thing > to ensure that you got the number of diffs you expected (for instance, > that a single -v really behaved as -v, and not as -v -v), so that can > be used as an argument for overhauling the old tests, as well as using > counting in all new tests. > > However, even if you take the approach of making 'check-for-diff' > succeed unconditionally and always count diffs, the current > implementation is still overly complicated. It would be much simpler > to let 'check-for-diff' always create the output file, and then use > "test_line_count = 0 out" when expecting no diffs than to sometimes > create the output file and sometimes not. The shell '>' operator will > truncate the file to zero size even before grep is invoked, so you > don't need to worry that results from an earlier test will pollute > 'out' for a subsequent test, even if grep finds no matches. Thus, > 'check-for-diff' collapses to this tiny implementation: > > grep '^diff --git' "$1" >out > exit 0 > > Or, if you want to be terse: > > grep '^diff --git' "$1" || exit 0 >out > > A couple notes: First, "out" isn't a great name for this file; perhaps > come up with a better name. Second, under this scenario, > "check-for-diff" isn't the most accurate name; it's now simply > collecting diffs rather than checking for presence. > > An alternative would be for the output file to contain the actual > count of diffs, rather than the diff lines themselves. For instance: > > write_script count-diffs <<\EOF && > grep -c '^diff --git' "$1" >out > exit 0 > EOF > > Whether that's actually better is subjective. > > So, what's my final word? I quite like the idea of keeping everything > as simple as possible, thus not altering existing tests and only doing > line count in the two or three new tests which actually care about the > number of diffs. However, I can also be sold on retrofitting existing > tests and having new tests do line counting as a way to improve test > coverage by catching cases where "-v" incorrectly behaves as "-v -v". > If you go that route, then the commit message of this patch needs to > sell it as such (an improvement in test coverage). ``` ## Eric Sunshine, 2016-03-27 17:08 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQyXi3_EwZakwrL3JahqZPFsJEifaTOB-KrZLtkxn5Oew%40mail.gmail.com In-Reply-To: ``` [forwarding this to the list, as well, since I again didn't notice when replying that Pranit had accidentally dropped the mailing list as a recipient] On Sun, Mar 27, 2016 at 12:59 PM, Eric Sunshine wrote: > On Sun, Mar 27, 2016 at 5:05 AM, Pranit Bauva wrote: >> On Sun, Mar 27, 2016 at 12:40 PM, Eric Sunshine wrote: >>> However, even if you take the approach of making 'check-for-diff' >>> succeed unconditionally and always count diffs, the current >>> implementation is still overly complicated. It would be much simpler >>> to let 'check-for-diff' always create the output file, and then use >>> "test_line_count = 0 out" when expecting no diffs than to sometimes >>> create the output file and sometimes not. The shell '>' operator will >>> truncate the file to zero size even before grep is invoked, so you >>> don't need to worry that results from an earlier test will pollute >>> 'out' for a subsequent test, even if grep finds no matches. Thus, >>> 'check-for-diff' collapses to this tiny implementation: >>> >>> grep '^diff --git' "$1" >out >>> exit 0 >>> >>> Or, if you want to be terse: >>> >>> grep '^diff --git' "$1" || exit 0 >out >> >> I tried using both of this and it gives an error > > These worked fine for me when I rewrote the test script before making > the suggestion. You'll need to provide more information if you want to > get to the bottom of the problem you experienced (for instance, show > the exact code you used and the actual error message). ``` ## Eric Sunshine, 2016-03-27 17:27 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: <20160327172741.GA4005@flurp.local> URL: https://gitlist.dev/e/20160327172741.GA4005%40flurp.local In-Reply-To: <1459085245-20658-1-git-send-email-szeder@ira.uka.de> ``` On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote: > > > +! test -s out || > > > +rm out && > > > > Why not just "rm -f out"? But, more importantly, why do you need to > > remove the file at all? The '>' redirection operator (used below) will > > overwrite the file, so no need to remove it beforehand. > > > > > +! grep '^diff --git' "$1" || > > > +grep '^diff --git' "$1" >out > > > > Um, what? Why two greps? I would have expected you to simply re-use > > the existing grep (minus the backslash) while adding the redirection: > > > > -exec grep '^diff --git' "\$1" > > +exec grep '^diff --git' "$1" >out > > > > Am I missing something obvious? > > In the non-verbose cases no diff is included in the commit message > template, thus the pattern looking for it doesn't match anything, grep > exits with error code, which in turn becomes the editor's exit > code, ultimately making 'git commit' fail. Not good. > > I suppose both the explicit 'rm out' and the '! grep ... || ...' is > there to deal with this situation. Sure, I understand that, but that's not what I meant. What I wanted to know was why Pranit didn't just use the simpler: grep '^diff --git' "$1" >out exit 0 and then, in tests: test_line_count = n out where 'n' is 0, 1, or 2 as expected by the test. Unfortunately, you missed the rest of the discussion since Pranit (presumably) accidentally dropped the mailing list when he replied, and I didn't notice the omission when replying to him with the above suggestion, which would have saved you the bother of going through this, as well. > I think we could: > > - either revive the idea of two editor scripts: one for the > non-verbose case checking with '! grep ...' that there are no > diffs in the commit message template, and one for all verbose > cases storing those diff lines in a file to be counted later. > > - or use a fake editor that merely copies the whole commit message > template to a separate file, and we do the greping in the tests > themselves as well. > > - or simply stick a 'true' at the end of the editor script ensuring > that it returns success even when grep can't find the pattern, but > I kind of feel ashamed of myself for even mentioning this > possibility ;) > > I would go for the second possibility, but don't feel strong about it. Your #3 is effectively what I had suggested, as well (which is reproduced above). I had already made this change locally along with some other changes I suggested in other responses, and those changes look like this (atop Pranit's two patches), which isn't too bad: --- 8< --- diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 569fd8b..ea26b57 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -4,12 +4,9 @@ test_description='verbose commit template' . ./test-lib.sh write_script "check-for-diff" <<'EOF' && -! test -s out || -rm out && -! grep '^diff --git' "$1" || grep '^diff --git' "$1" >out +exit 0 EOF -chmod +x check-for-diff test_set_editor "$PWD/check-for-diff" cat >message <<'EOF' @@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' test_i18ngrep "Aborting commit due to empty commit message." err ' +test_expect_success 'setup -v -v' ' + echo dirty >file +' + test_expect_success 'commit.verbose true and --verbose omitted' ' - echo content >file2 && - echo content >>file && - git add file2 && - git -c commit.verbose=true commit -F message && + git -c commit.verbose=true commit --amend && test_line_count = 1 out ' @@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' ' test_expect_success 'commit.verbose true and --no-verbose' ' git -c commit.verbose=true commit --amend --no-verbose && - ! test -s out + test_line_count = 0 out ' test_expect_success 'commit.verbose false and --verbose' ' @@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' ' test_expect_success 'commit.verbose false and --verbose omitted' ' git -c commit.verbose=false commit --amend && - ! test -s out + test_line_count = 0 out ' test_expect_success 'commit.verbose false and --no-verbose' ' git -c commit.verbose=false commit --amend --no-verbose && - ! test -s out + test_line_count = 0 out ' test_expect_success 'status ignores commit.verbose=true' ' -- ``` ## Pranit Bauva, 2016-03-27 18:31 Subject: Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file Message-ID: URL: https://gitlist.dev/e/CAFZEwPOuWGoVj1s4aqoNy6eLAKHEdcH%3D7%3Do3gPxQ_eQvpp8N8Q%40mail.gmail.com In-Reply-To: <20160327172741.GA4005@flurp.local> ``` On Sun, Mar 27, 2016 at 10:57 PM, Eric Sunshine wrote: > On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote: >> > > +! test -s out || >> > > +rm out && >> > >> > Why not just "rm -f out"? But, more importantly, why do you need to >> > remove the file at all? The '>' redirection operator (used below) will >> > overwrite the file, so no need to remove it beforehand. >> > >> > > +! grep '^diff --git' "$1" || >> > > +grep '^diff --git' "$1" >out >> > >> > Um, what? Why two greps? I would have expected you to simply re-use >> > the existing grep (minus the backslash) while adding the redirection: >> > >> > -exec grep '^diff --git' "\$1" >> > +exec grep '^diff --git' "$1" >out >> > >> > Am I missing something obvious? >> >> In the non-verbose cases no diff is included in the commit message >> template, thus the pattern looking for it doesn't match anything, grep >> exits with error code, which in turn becomes the editor's exit >> code, ultimately making 'git commit' fail. Not good. >> >> I suppose both the explicit 'rm out' and the '! grep ... || ...' is >> there to deal with this situation. > > Sure, I understand that, but that's not what I meant. What I wanted > to know was why Pranit didn't just use the simpler: > > grep '^diff --git' "$1" >out > exit 0 > > and then, in tests: > > test_line_count = n out > > where 'n' is 0, 1, or 2 as expected by the test. Sorry to create extra noise. This works perfectly fine. I had put an 'exec' locally. But now I have tested this and it works fine.. > > Unfortunately, you missed the rest of the discussion since Pranit > (presumably) accidentally dropped the mailing list when he replied, > and I didn't notice the omission when replying to him with the above > suggestion, which would have saved you the bother of going through > this, as well. Sorry for this. I might have just started typing and forgot to click reply all. >> I think we could: >> >> - either revive the idea of two editor scripts: one for the >> non-verbose case checking with '! grep ...' that there are no >> diffs in the commit message template, and one for all verbose >> cases storing those diff lines in a file to be counted later. >> >> - or use a fake editor that merely copies the whole commit message >> template to a separate file, and we do the greping in the tests >> themselves as well. >> >> - or simply stick a 'true' at the end of the editor script ensuring >> that it returns success even when grep can't find the pattern, but >> I kind of feel ashamed of myself for even mentioning this >> possibility ;) >> >> I would go for the second possibility, but don't feel strong about it. > > Your #3 is effectively what I had suggested, as well (which is > reproduced above). I had already made this change locally along with > some other changes I suggested in other responses, and those changes > look like this (atop Pranit's two patches), which isn't too bad: > > --- 8< --- > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index 569fd8b..ea26b57 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -4,12 +4,9 @@ test_description='verbose commit template' > . ./test-lib.sh > > write_script "check-for-diff" <<'EOF' && > -! test -s out || > -rm out && > -! grep '^diff --git' "$1" || > grep '^diff --git' "$1" >out > +exit 0 > EOF > -chmod +x check-for-diff > test_set_editor "$PWD/check-for-diff" > > cat >message <<'EOF' > @@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' > test_i18ngrep "Aborting commit due to empty commit message." err > ' > > +test_expect_success 'setup -v -v' ' > + echo dirty >file > +' > + > test_expect_success 'commit.verbose true and --verbose omitted' ' > - echo content >file2 && > - echo content >>file && > - git add file2 && > - git -c commit.verbose=true commit -F message && > + git -c commit.verbose=true commit --amend && > test_line_count = 1 out > ' > > @@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' ' > > test_expect_success 'commit.verbose true and --no-verbose' ' > git -c commit.verbose=true commit --amend --no-verbose && > - ! test -s out > + test_line_count = 0 out > ' > > test_expect_success 'commit.verbose false and --verbose' ' > @@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' ' > > test_expect_success 'commit.verbose false and --verbose omitted' ' > git -c commit.verbose=false commit --amend && > - ! test -s out > + test_line_count = 0 out > ' > > test_expect_success 'commit.verbose false and --no-verbose' ' > git -c commit.verbose=false commit --amend --no-verbose && > - ! test -s out > + test_line_count = 0 out > ' > > test_expect_success 'status ignores commit.verbose=true' ' > -- ``` ## Pranit Bauva, 2016-03-31 14:45 Subject: [PATCH v11 1/4] test-parse-options: print quiet as integer Message-ID: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com> ``` Current implementation of parse-options.c treats OPT__QUIET() as integer and not boolean and thus it is more appropriate to print it as integer to avoid confusion. While at it, fix some style issues. --- t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------ test-parse-options.c | 2 +- 2 files changed, 48 insertions(+), 48 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 9be6411..2dcbdc0 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -7,7 +7,7 @@ test_description='our own option parser' . ./test-lib.sh -cat > expect << EOF +cat >expect < --yes get a boolean @@ -49,7 +49,7 @@ Standard options EOF test_expect_success 'test help' ' - test_must_fail test-parse-options -h > output 2> output.err && + test_must_fail test-parse-options -h >output 2>output.err && test_must_be_empty output.err && test_i18ncmp expect output ' @@ -64,7 +64,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' check magnitude: 3221225472 -m 3g ' -cat > expect << EOF +cat >expect < expect << EOF +cat >expect < expect << EOF +cat >expect < output 2> output.err && + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect << EOF +cat >expect < output 2> output.err && + test-parse-options --int 2 --boolean --no-bo >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'unambiguously abbreviated option with "="' ' - test-parse-options --int=2 > output 2> output.err && + test-parse-options --int=2 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' @@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' ' test_expect_code 129 test-parse-options --strin 123 ' -cat > expect << EOF +cat >expect < output 2> output.err && + test-parse-options --st 123 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > typo.err << EOF +cat >typo.err < output 2> output.err && + test_must_fail test-parse-options -boolean >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > typo.err << EOF +cat >typo.err < output 2> output.err && + test_must_fail test-parse-options -ambiguous >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > expect <expect < output 2> output.err && + test-parse-options --quux >output 2>output.err && test_must_be_empty output.err && - test_cmp expect output + test_cmp expect output ' -cat > expect <expect < output 2> output.err && + foo -q >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options --length=four -b -4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test_must_fail test-parse-options --no-length >output 2>output.err && test_i18ncmp expect output && test_i18ncmp expect.err output.err ' -cat > expect <expect < output 2> output.err && + test-parse-options --set23 -bbbbb --no-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' ' - test-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err && + test-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options -bb --or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() works' ' - test-parse-options -bb --no-neg-or4 > output 2> output.err && + test-parse-options -bb --no-neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' ' - test-parse-options + + + + + + > output 2> output.err && + test-parse-options + + + + + + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options -12345 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' @@ -449,7 +449,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' test_cmp expect output ' -cat >>expect <<'EOF' +cat >>expect < URL: https://gitlist.dev/e/01020153cd2341ef-eed17644-85ee-4d1d-8051-b5d56bd9ae52-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com> ``` Make the fake "editor" store output of grep in a file so that we can see how many diffs were contained in the message and use them in individual tests where ever it is required. Also use write_script() to create the fake "editor". A subsequent commit will introduce scenarios where it is important to be able to exactly determine how many diffs were present. Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva Previous version of this patch: - [v10]: $gmane/288820 Changes this version wrt previous one: I decided to include no of diffs in every test and rewrote the commit message so as to sell this idea. This was given as an option to me by Eric and the other option being to drop unnecessary testing of lines where it isn't required. Also this patch uses a suggestion given by Eric to make the "editor" look more clean as compared to the editor in my previous version. --- t/t7507-commit-verbose.sh | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 2ddf28c..0f28a86 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -3,11 +3,10 @@ test_description='verbose commit template' . ./test-lib.sh -cat >check-for-diff <out +exit 0 EOF -chmod +x check-for-diff test_set_editor "$PWD/check-for-diff" cat >message <<'EOF' @@ -23,7 +22,8 @@ test_expect_success 'setup' ' ' test_expect_success 'initial commit shows verbose diff' ' - git commit --amend -v + git commit --amend -v && + test_line_count = 1 out ' test_expect_success 'second commit' ' @@ -39,13 +39,15 @@ check_message() { test_expect_success 'verbose diff is stripped out' ' git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' git config diff.mnemonicprefix true && git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' cat >diff <<'EOF' -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-31 14:45 Subject: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: <01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com> ``` The reason to make it respect "unspecified" values is to give the ability to differentiate whether `--option` or `--no-option` was specified at all. "unspecified" values should be in the form of negative values. If initial value is set to negative and `--option` specified then it will reflect the number of occurrences (counting done from 0), if `--no-option` is specified then it will reflect 0 and if nothing at all is given then it will retain its negative value. This change will not affect existing users of COUNTUP at all, as long as they use the initial value of 0 (or more). Force uses the "unspecified" value in conjunction with OPT__FORCE in builtin/clean.c in a different way to handle its config which munges the "-1" into 0 before we get to parse_options. To test this behavior "verbose" is set to "unspecified" while quiet is set to 0 which will test the new behavior with all sets of values. Helped-by: Jeff King Helped-by: Eric Sunshine Helped-by: Junio C Hamano Signed-off-by: Pranit Bauva --- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 Previous version of the patch: [v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [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. Also added some points as to where this patch could break but it doesn't. Include tests to test this behavior by modifying test-parse-options.c and t/t0040-parse-options.sh as requested by SZEDER and reminded by Eric. Please Note: The diff might seem improper especially the part where I have introduced some continuous lines but this is a logical error by git diff (nothing could be done about it) and thus the changes will be clearly visible with the original file itself. --- Documentation/technical/api-parse-options.txt | 8 ++++-- parse-options.c | 8 +++++- t/t0040-parse-options.sh | 39 ++++++++++++++++++++------- test-parse-options.c | 3 ++- 4 files changed, 44 insertions(+), 14 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 5f0757d..8908bf7 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -144,8 +144,12 @@ There are some macros to easily define options: `OPT_COUNTUP(short, long, &int_var, description)`:: Introduce a count-up option. - `int_var` is incremented on each use of `--option`, and - reset to zero with `--no-option`. + Each use of `--option` increments `int_var`, starting from zero + (even if initially negative), and `--no-option` resets it to + zero. To determine if `--option` or `--no-option` was set at + all, set `int_var` to a negative value, and if it is still + negative after parse_options(), then neither `--option` nor + `--no-option` was seen. `OPT_BIT(short, long, &int_var, description, mask)`:: Introduce a boolean option. diff --git a/parse-options.c b/parse-options.c index 47a9192..86d30bd 100644 --- a/parse-options.c +++ b/parse-options.c @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, return 0; case OPTION_COUNTUP: - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; + if (unset) + *(int *)opt->value = 0; + else { + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; + *(int *)opt->value += 1; + } return 0; case OPTION_SET_INT: diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 2dcbdc0..275bb64 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -63,7 +63,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -211,7 +211,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -234,7 +234,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -263,7 +263,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -302,7 +302,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -322,7 +322,7 @@ magnitude: 0 timestamp: 1 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 1 dry run: no file: (not set) @@ -344,7 +344,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -373,7 +373,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -398,7 +398,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -429,7 +429,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -454,6 +454,25 @@ dry run: no file: (not set) EOF +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' + test-parse-options --no-verbose >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' + +cat >expect <output 2>output.err && test_must_be_empty output.err && diff --git a/test-parse-options.c b/test-parse-options.c index 86afa98..f02c275 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -7,7 +7,8 @@ static int integer = 0; static unsigned long magnitude = 0; static unsigned long timestamp; static int abbrev = 7; -static int verbose = 0, dry_run = 0, quiet = 0; +static int verbose = -1; /* unspecified */ +static int dry_run = 0, quiet = 0; static char *string = NULL; static char *file = NULL; static int ambiguous; -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-31 14:45 Subject: [PATCH v11 4/4] commit: add a commit.verbose config variable Message-ID: <01020153cd2341ec-341900e5-d186-401f-8cd2-dd213303ae4f-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153cd2341ec-341900e5-d186-401f-8cd2-dd213303ae4f-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-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 Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- The previous version of the patch are: - [v10] $gmane/288820 - [v9] $gmane/288820 - [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: - Before config_verbose reacted to only -1 but henceforth it will react to every negative integer. - Use test_line_count for consistency - Add 1 more test for to check whether git status breaks - Add more tests to test whether commit.verbose works fine with all integers suggested by Eric Sunshine. Note: One might think some tests are extra but I think that it will be better to include them as they "complete the continuity" thus generalising the series which will make the patch even more clearer. SZEDER pointed out a bug in v9 which broke status. verbose being set to -1, returns true in the `if(verbose)` statement which gives a verbose output. So thus, it is required to reset it to 0 if it was -1. This will work fine when status is passed with --verbose and --no-verbose as they both reflect non-negative values. --- Documentation/config.txt | 4 + Documentation/git-commit.txt | 3 +- builtin/commit.c | 14 +++- t/t7507-commit-verbose.sh | 175 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 194 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..96e6190 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; @@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix) builtin_status_usage, 0); finalize_colopts(&s.colopts, -1); finalize_deferred_config(&s); + if (verbose == -1) + verbose = 0; handle_untracked_files_arg(&s); if (show_ignored_in_status) @@ -1505,6 +1509,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 +1663,9 @@ 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 < 0) ? 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 0f28a86..7c79484 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -98,4 +98,179 @@ 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 'set up -v -v' ' + echo dirty >file && + echo dirty >file2 && + git add file2 +' +test_expect_success 'commit.verbose true and --verbose omitted' ' + git -c commit.verbose=true commit -F message && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and --verbose' ' + git -c commit.verbose=true commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and -v -v' ' + git -c commit.verbose=true commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose true and --no-verbose' ' + git -c commit.verbose=true commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose false and --verbose' ' + git -c commit.verbose=false commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose false and -v -v' ' + git -c commit.verbose=false commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose false and --verbose omitted' ' + git -c commit.verbose=false commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose false and --no-verbose' ' + git -c commit.verbose=false commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-2 and --verbose omitted' ' + git -c commit.verbose=-2 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-1 and --verbose omitted' ' + git -c commit.verbose=-1 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=0 and --verbose omitted' ' + git -c commit.verbose=0 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=1 and --verbose omitted' ' + git -c commit.verbose=1 commit --amend && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=2 and --verbose omitted' ' + git -c commit.verbose=2 commit --amend && + test_line_count = 2 out +' + +test_expect_success 'commit-verbose=3 and --verbose omitted' ' + git -c commit.verbose=3 commit --amend && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-2 and --verbose' ' + git -c commit.verbose=-2 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=-1 and --verbose' ' + git -c commit.verbose=-1 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=0 and --verbose' ' + git -c commit.verbose=0 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=1 and --verbose' ' + git -c commit.verbose=1 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=2 and --verbose' ' + git -c commit.verbose=2 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=3 and --verbose' ' + git -c commit.verbose=3 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=-2 and -v -v' ' + git -c commit.verbose=-2 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-1 and -v -v' ' + git -c commit.verbose=-1 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=0 and -v -v' ' + git -c commit.verbose=0 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=1 and -v -v' ' + git -c commit.verbose=1 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=2 and -v -v' ' + git -c commit.verbose=2 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=3 and -v -v' ' + git -c commit.verbose=3 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-2 and --no-verbose' ' + git -c commit.verbose=-2 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-1 and --no-verbose' ' + git -c commit.verbose=-1 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=0 and --no-verbose' ' + git -c commit.verbose=0 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=1 and --no-verbose' ' + git -c commit.verbose=1 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=2 and --no-verbose' ' + git -c commit.verbose=2 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=3 and --no-verbose' ' + git -c commit.verbose=3 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + +test_expect_success 'status does not verbose without --verbose' ' + git status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/218 ``` ## Junio C Hamano, 2016-03-31 18:19 Subject: Re: [PATCH v11 1/4] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/xmqqegaqo6gg.fsf%40gitster.mtv.corp.google.com In-Reply-To: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com> ``` Pranit Bauva writes: > Current implementation of parse-options.c treats OPT__QUIET() as integer > and not boolean and thus it is more appropriate to print it as integer > to avoid confusion. > > While at it, fix some style issues. I counted the changes in t0040 and you have _more_ style fixes than the real change. That is not "while at it". While I welcome the style fix, it is better done as a preparatory clean-up step before the real change. > --- Missing sign-off. > -cat > typo.err << EOF > +cat >typo.err < error: did you mean \`--boolean\` (with two dashes ?) > EOF If your "style cleanup" patch were separate, you could fix this (and other that have backslash escape inside the here-document) further to something like this: cat >type.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF Thanks. ``` ## Junio C Hamano, 2016-03-31 18:23 Subject: Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: URL: https://gitlist.dev/e/xmqqa8leo6a6.fsf%40gitster.mtv.corp.google.com In-Reply-To: <01020153cd2341ef-eed17644-85ee-4d1d-8051-b5d56bd9ae52-000000@eu-west-1.amazonses.com> ``` Pranit Bauva writes: > Make the fake "editor" store output of grep in a file so that we can > see how many diffs were contained in the message and use them in > individual tests where ever it is required. Also use write_script() > to create the fake "editor". > > A subsequent commit will introduce scenarios where it is important to be > able to exactly determine how many diffs were present. > > Helped-by: Eric Sunshine > Signed-off-by: Pranit Bauva > > Previous version of this patch: > - [v10]: $gmane/288820 > > Changes this version wrt previous one: > I decided to include no of diffs in every test and rewrote the commit > message so as to sell this idea. This was given as an option to me by > Eric and the other option being to drop unnecessary testing of lines > where it isn't required. Also this patch uses a suggestion given by Eric > to make the "editor" look more clean as compared to the editor in my > previous version. > --- OK, by always exiting 0 from the editor, you do not interfere with the "git commit" that invoked it, and you inspect the editor's finding after "git commit" returns. The approach taken by this patch looks a lot more sensible than the previous one. You'd need the three-dash right before "Previous version of..." line, though. > t/t7507-commit-verbose.sh | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index 2ddf28c..0f28a86 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -3,11 +3,10 @@ > test_description='verbose commit template' > . ./test-lib.sh > > -cat >check-for-diff < -#!$SHELL_PATH > -exec grep '^diff --git' "\$1" > +write_script "check-for-diff" <<\EOF && > +grep '^diff --git' "$1" >out > +exit 0 > EOF > -chmod +x check-for-diff > test_set_editor "$PWD/check-for-diff" > > cat >message <<'EOF' > @@ -23,7 +22,8 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'initial commit shows verbose diff' ' > - git commit --amend -v > + git commit --amend -v && > + test_line_count = 1 out > ' > > test_expect_success 'second commit' ' > @@ -39,13 +39,15 @@ check_message() { > > test_expect_success 'verbose diff is stripped out' ' > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' > > test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' > git config diff.mnemonicprefix true && > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' > > cat >diff <<'EOF' > > -- > https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-31 18:40 Subject: Re: [PATCH v11 1/4] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CAFZEwPMyVftmW8ZCCyFMUMOegqimpFxjLrHfjMKc88w%2BHDvG%2BA%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 11:49 PM, Junio C Hamano wrote: > Pranit Bauva writes: > >> Current implementation of parse-options.c treats OPT__QUIET() as integer >> and not boolean and thus it is more appropriate to print it as integer >> to avoid confusion. >> >> While at it, fix some style issues. > > I counted the changes in t0040 and you have _more_ style fixes than > the real change. That is not "while at it". > > While I welcome the style fix, it is better done as a preparatory > clean-up step before the real change. Okay. I thought this was a minor change so I squashed it together. I will separate it though. > Missing sign-off. Will include this >> -cat > typo.err << EOF >> +cat >typo.err <> error: did you mean \`--boolean\` (with two dashes ?) >> EOF > > If your "style cleanup" patch were separate, you could fix this (and > other that have backslash escape inside the here-document) further > to something like this: > > cat >type.err <<\EOF > error: did you mean `--boolean` (with two dashes ?) > EOF > > Thanks. Will include this. ``` ## Junio C Hamano, 2016-03-31 18:41 Subject: Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/xmqq60w2o5g9.fsf%40gitster.mtv.corp.google.com In-Reply-To: <01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000@eu-west-1.amazonses.com> ``` Pranit Bauva writes: > This change will not affect existing users of COUNTUP at all, as long as > they use the initial value of 0 (or more). > > Force uses the "unspecified" value in conjunction with OPT__FORCE in > builtin/clean.c in a different way to handle its config which > munges the "-1" into 0 before we get to parse_options. These two paragraphs leave the readers wondering what the conclusion is. "it is OK as long as..." makes us wonder "so are there users that do not satisfy that condition?" "in a different way" makes us wonder "Does this change break builtin/clean.c because COUNTUP is used in a different way?". This change does not affect existing users of COUNTUP, because they all use the initial value of 0 (or more). Note that builtin/clean.c initializes the variable used with OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set to either 0 or 1 by reading the configuration before the code calls parse_options(), i.e. as far as parse_options() is concerned, the initial value of the variable is not negative. perhaps? > `OPT_COUNTUP(short, long, &int_var, description)`:: > Introduce a count-up option. > - `int_var` is incremented on each use of `--option`, and > - reset to zero with `--no-option`. > + Each use of `--option` increments `int_var`, starting from zero > + (even if initially negative), and `--no-option` resets it to > + zero. To determine if `--option` or `--no-option` was set at > + all, set `int_var` to a negative value, and if it is still > + negative after parse_options(), then neither `--option` nor > + `--no-option` was seen. > > `OPT_BIT(short, long, &int_var, description, mask)`:: > Introduce a boolean option. > diff --git a/parse-options.c b/parse-options.c > index 47a9192..86d30bd 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, > return 0; > > case OPTION_COUNTUP: > - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > + if (unset) > + *(int *)opt->value = 0; > + else { > + if (*(int *)opt->value < 0) > + *(int *)opt->value = 0; > + *(int *)opt->value += 1; > + } > return 0; > > case OPTION_SET_INT: The new behaviour given by the patch looks very sensible, but I wonder if this is easier to reason about: case OPTION_COUNTUP: + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; That is, upon hitting this case arm, we know that an explicit option was given from the command line, so the "unspecified" initial value, if it is still there, gets reset to 0, and after doing that, we just do the usual thing. ``` ## Pranit Bauva, 2016-03-31 18:42 Subject: Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: URL: https://gitlist.dev/e/CAFZEwPP1iQQq5WXsBa-4vZ3wXH-DeNUw5JtqsgYmeUSo-XPYxQ%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 31, 2016 at 11:53 PM, Junio C Hamano wrote: > Pranit Bauva writes: > >> Make the fake "editor" store output of grep in a file so that we can >> see how many diffs were contained in the message and use them in >> individual tests where ever it is required. Also use write_script() >> to create the fake "editor". >> >> A subsequent commit will introduce scenarios where it is important to be >> able to exactly determine how many diffs were present. >> >> Helped-by: Eric Sunshine >> Signed-off-by: Pranit Bauva >> >> Previous version of this patch: >> - [v10]: $gmane/288820 >> >> Changes this version wrt previous one: >> I decided to include no of diffs in every test and rewrote the commit >> message so as to sell this idea. This was given as an option to me by >> Eric and the other option being to drop unnecessary testing of lines >> where it isn't required. Also this patch uses a suggestion given by Eric >> to make the "editor" look more clean as compared to the editor in my >> previous version. >> --- > > OK, by always exiting 0 from the editor, you do not interfere with > the "git commit" that invoked it, and you inspect the editor's > finding after "git commit" returns. The approach taken by this > patch looks a lot more sensible than the previous one. > > You'd need the three-dash right before "Previous version of..." > line, though. That's silly of me to forget this. Will do it. >> t/t7507-commit-verbose.sh | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >> index 2ddf28c..0f28a86 100755 >> --- a/t/t7507-commit-verbose.sh >> +++ b/t/t7507-commit-verbose.sh >> @@ -3,11 +3,10 @@ >> test_description='verbose commit template' >> . ./test-lib.sh >> >> -cat >check-for-diff <> -#!$SHELL_PATH >> -exec grep '^diff --git' "\$1" >> +write_script "check-for-diff" <<\EOF && >> +grep '^diff --git' "$1" >out >> +exit 0 >> EOF >> -chmod +x check-for-diff >> test_set_editor "$PWD/check-for-diff" >> >> cat >message <<'EOF' >> @@ -23,7 +22,8 @@ test_expect_success 'setup' ' >> ' >> >> test_expect_success 'initial commit shows verbose diff' ' >> - git commit --amend -v >> + git commit --amend -v && >> + test_line_count = 1 out >> ' >> >> test_expect_success 'second commit' ' >> @@ -39,13 +39,15 @@ check_message() { >> >> test_expect_success 'verbose diff is stripped out' ' >> git commit --amend -v && >> - check_message message >> + check_message message && >> + test_line_count = 1 out >> ' >> >> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' >> git config diff.mnemonicprefix true && >> git commit --amend -v && >> - check_message message >> + check_message message && >> + test_line_count = 1 out >> ' >> >> cat >diff <<'EOF' >> >> -- >> https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-03-31 19:34 Subject: Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAFZEwPO4CJ-Q%3D52Es14CdD0mjG-qgy4%3DX2kU71npi%3D8x7FsSQw%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> This change will not affect existing users of COUNTUP at all, as long as >> they use the initial value of 0 (or more). >> >> Force uses the "unspecified" value in conjunction with OPT__FORCE in >> builtin/clean.c in a different way to handle its config which >> munges the "-1" into 0 before we get to parse_options. > > These two paragraphs leave the readers wondering what the conclusion > is. "it is OK as long as..." makes us wonder "so are there users > that do not satisfy that condition?" "in a different way" makes us > wonder "Does this change break builtin/clean.c because COUNTUP is > used in a different way?". > > This change does not affect existing users of COUNTUP, > because they all use the initial value of 0 (or more). > > Note that builtin/clean.c initializes the variable used with > OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, > but it is set to either 0 or 1 by reading the configuration > before the code calls parse_options(), i.e. as far as > parse_options() is concerned, the initial value of the > variable is not negative. > > perhaps? Thanks again for the help with the comit message. I sometimes fail to see how first time readers will infer from the message. >> `OPT_COUNTUP(short, long, &int_var, description)`:: >> Introduce a count-up option. >> - `int_var` is incremented on each use of `--option`, and >> - reset to zero with `--no-option`. >> + Each use of `--option` increments `int_var`, starting from zero >> + (even if initially negative), and `--no-option` resets it to >> + zero. To determine if `--option` or `--no-option` was set at >> + all, set `int_var` to a negative value, and if it is still >> + negative after parse_options(), then neither `--option` nor >> + `--no-option` was seen. >> >> `OPT_BIT(short, long, &int_var, description, mask)`:: >> Introduce a boolean option. >> diff --git a/parse-options.c b/parse-options.c >> index 47a9192..86d30bd 100644 >> --- a/parse-options.c >> +++ b/parse-options.c >> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, >> return 0; >> >> case OPTION_COUNTUP: >> - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; >> + if (unset) >> + *(int *)opt->value = 0; >> + else { >> + if (*(int *)opt->value < 0) >> + *(int *)opt->value = 0; >> + *(int *)opt->value += 1; >> + } >> return 0; >> >> case OPTION_SET_INT: > > The new behaviour given by the patch looks very sensible, but I > wonder if this is easier to reason about: > > case OPTION_COUNTUP: > + if (*(int *)opt->value < 0) > + *(int *)opt->value = 0; > *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > > That is, upon hitting this case arm, we know that an explicit option > was given from the command line, so the "unspecified" initial value, > if it is still there, gets reset to 0, and after doing that, we just > do the usual thing. This does look cleaner. Nice thinking that there is no need to actually specify when it gets 0. It gets 0 no matter what as long as OPTION_COUNTUP is speficied in any format (with or without --no) and variable is "unspecified". ``` ## Junio C Hamano, 2016-03-31 20:06 Subject: Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/xmqqegaqmmyr.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote: >> >> case OPTION_COUNTUP: >> + if (*(int *)opt->value < 0) >> + *(int *)opt->value = 0; >> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; >> >> That is, upon hitting this case arm, we know that an explicit option >> was given from the command line, so the "unspecified" initial value, >> if it is still there, gets reset to 0, and after doing that, we just >> do the usual thing. > > This does look cleaner. Nice thinking that there is no need to > actually specify when it gets 0. It gets 0 no matter what as long as > OPTION_COUNTUP is speficied in any format (with or without --no) and > variable is "unspecified". I do not think there is any planned users of such an enhancement, but the above points us into a future possibility to be able to do this: case OPTION_COUNTUP: + if (*(int *)opt->value < 0) + *(int *)opt->value = -*(int *)opt->value - 1; *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; That is, by using "-2" as the "unspecified" value, you can start counting up from 2 (i.e. the presence of the option resets the variable to 1 and then the option being not "unset" increments it) if your application wants to. ``` ## Pranit Bauva, 2016-03-31 20:41 Subject: Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAFZEwPN_%3DmLXf%2B8JfMhwf_5fviG-rjHp07b%2B41HXs2NLO6BnxA%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2016 at 1:36 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote: >>> >>> case OPTION_COUNTUP: >>> + if (*(int *)opt->value < 0) >>> + *(int *)opt->value = 0; >>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; >>> >>> That is, upon hitting this case arm, we know that an explicit option >>> was given from the command line, so the "unspecified" initial value, >>> if it is still there, gets reset to 0, and after doing that, we just >>> do the usual thing. >> >> This does look cleaner. Nice thinking that there is no need to >> actually specify when it gets 0. It gets 0 no matter what as long as >> OPTION_COUNTUP is speficied in any format (with or without --no) and >> variable is "unspecified". > > I do not think there is any planned users of such an enhancement, > but the above points us into a future possibility to be able to do > this: > > case OPTION_COUNTUP: > + if (*(int *)opt->value < 0) > + *(int *)opt->value = -*(int *)opt->value - 1; > *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > > That is, by using "-2" as the "unspecified" value, you can start > counting up from 2 (i.e. the presence of the option resets the > variable to 1 and then the option being not "unset" increments it) > if your application wants to. I am not very familiar with Git community but I think including a new feature to use our existing library (who's functionality isn't required as for now) can trigger some creative thoughts in minds of other developers which will make them think "How could this be used?". This could lead to some exciting ideas on improving the UI of git. Having something actually in hand gives you a confidence, rather than knowing that you could grab it whenever it is needed. Also, when such an idea for new feature comes up, it is better to implement it because let's say some developer is stuck in future and this new feature could help him but he doesn't have a clue that such a discussion happened some time ago. Thus saving him time and further effort. Anyways, How is the convention in git for these type of futuristic ideas? ``` ## Junio C Hamano, 2016-03-31 20:45 Subject: Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/xmqqshz6l6kt.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > Also, when such an idea for new feature comes up, it is better to > implement it because let's say some developer is stuck in future and > this new feature could help him but he doesn't have a clue that such a > discussion happened some time ago. Thus saving him time and further > effort. > > Anyways, How is the convention in git for these type of futuristic ideas? Just keep it in mind without complicating the code, unless the idea truly makes sense and has immediate use. For example, I do not know how it would be useful to be able to count up starting from 2 with an option --more that is COUNTUP, if your --no-more would reset it to 0. git cmd --more ;# sets more=2 git cmd --more --more ;# sets more=3 git cmd --no-more --more ;# sets more=1, not more=2 ```