# [PATCH v8 2/2] commit: add a commit.verbose config variable 11 messages from 2016-03-18 to 2016-03-23. Participants: Pranit Bauva, Junio C Hamano, Eric Sunshine. Thread: https://gitlist.dev/t/41743 ## Pranit Bauva, 2016-03-18 21:19 Subject: [PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values Message-ID: <010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000%40eu-west-1.amazonses.com In-Reply-To: <010201537710be08-f31428b3-5df3-4694-8c4a-0e8f058131b3-000000@eu-west-1.amazonses.com> ``` The reason to make it consider negative values or more specifically "unspecified" values is to give the ability to differentiate between once, multiple time or with --no-option. Eg. : initialize verbose = -1 `git commit` => verbose = -1 `git commit -v` => verbose = 1 `git commit -v -v` => verbose = 2 `git commit --no-verbose` => verbose = 0 Signed-off-by: Pranit Bauva --- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 Previous version of the patch: [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061 Changes introduced: Use a different language in commit message to make the change and its utility more clear. --- Documentation/technical/api-parse-options.txt | 6 ++++-- parse-options.c | 8 +++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 5f0757d..a908d8a 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -144,8 +144,10 @@ There are some macros to easily define options: `OPT_COUNTUP(short, long, &int_var, description)`:: Introduce a count-up option. - `int_var` is incremented on each use of `--option`, and - reset to zero with `--no-option`. + If the `int_var` is negative and `--option` is absent, + then it will retain its value. Otherwise it will first set + `int_var` to 0 and then increment it on each use of `--option`, + and reset to zero with `--no-option`. `OPT_BIT(short, long, &int_var, description, mask)`:: Introduce a boolean option. diff --git a/parse-options.c b/parse-options.c index 47a9192..86d30bd 100644 --- a/parse-options.c +++ b/parse-options.c @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, return 0; case OPTION_COUNTUP: - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; + if (unset) + *(int *)opt->value = 0; + else { + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; + *(int *)opt->value += 1; + } return 0; case OPTION_SET_INT: -- https://github.com/git/git/pull/213 ``` ## Pranit Bauva, 2016-03-18 21:19 Subject: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: <010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000%40eu-west-1.amazonses.com In-Reply-To: <010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-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: - [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: - Consider multiple verbosity as mentioned by SZEDER. - To implement this, change the way in which OPTION_COUNTUP() works. Approach used: (Suggested by Junio Hamano and Eric Sunshine) - initialize verbose to "unspecified" - initialize config_verbose to "unspecified" - let git_config() update config_verbose - let parse_options() update verbose Then: - If verbose is still "unspecified", then overwrite it with config_verbose. - If verbose is still "unspecified" after that, then neither the command line nor the configuration gave you verbosity. - Otherwise verbose at this point has the verbosity level to use so set it to 0. --- Documentation/config.txt | 4 ++++ Documentation/git-commit.txt | 3 ++- builtin/commit.c | 17 +++++++++++++- t/t7507-commit-verbose.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 77 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..ad85be0 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -113,7 +113,9 @@ static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; +static int config_verbose = -1; /* unspecified */ +static int verbose = -1; /* unspecified */ +static int quiet, no_verify, allow_empty, dry_run, renew_authorship; static int no_post_rewrite, allow_empty_message; static char *untracked_files_arg, *force_date, *ignore_submodule_arg; static char *sign_commit; @@ -1505,6 +1507,11 @@ static int git_commit_config(const char *k, const char *v, void *cb) sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; } + if (!strcmp(k, "commit.verbose")) { + int is_bool; + config_verbose = git_config_bool_or_int(k, v, &is_bool); + return 0; + } status = git_gpg_config(k, v, NULL); if (status) @@ -1654,6 +1661,14 @@ 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 < 0) { + if (config_verbose > -1) + verbose = config_verbose; + else + verbose = 0; + } + 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 2ddf28c..dda674e 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -96,4 +96,59 @@ 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' ' + git -c commit.verbose=true commit --amend +' + +test_expect_success 'commit.verbose true and --verbose' ' + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + git -c commit.verbose=true commit --amend --verbose + ) && + grep "^diff --git" .git/COMMIT_EDITMSG >out && + wc -l out | grep "1" +' + +test_expect_success 'commit.verbose true and -v -v' ' + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + git -c commit.verbose=true commit --amend -v -v + ) && + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && + wc -l out | grep "2" +' + +test_expect_success 'commit.verbose true and --no-verbose' ' + test_must_fail git -c commit.verbose=true commit --amend --no-verbose +' + +test_expect_success 'commit.verbose false and --verbose' ' + git -c commit.verbose=false commit --amend --verbose +' + +test_expect_success 'commit.verbose false and -v -v' ' + ( + GIT_EDITOR=cat && + export GIT_EDITOR && + git -c commit.verbose=false commit --amend -v -v + ) && + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && + wc -l out | grep "2" +' + +test_expect_success 'commit.verbose false and --verbose omitted' ' + test_must_fail git -c commit.verbose=false commit --amend +' + +test_expect_success 'commit.verbose false and --no-verbose' ' + test_must_fail git -c commit.verbose=false commit --amend --no-verbose +' + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/213 ``` ## Junio C Hamano, 2016-03-18 22:59 Subject: Re: [PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values Message-ID: URL: https://gitlist.dev/e/xmqq4mc3pf3y.fsf%40gitster.mtv.corp.google.com In-Reply-To: <010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000@eu-west-1.amazonses.com> ``` Pranit Bauva writes: > The reason to make it consider negative values or more specifically > "unspecified" values is to give the ability to differentiate between > once, multiple time or with --no-option. > > Eg. : > initialize verbose = -1 > `git commit` => verbose = -1 > `git commit -v` => verbose = 1 > `git commit -v -v` => verbose = 2 > `git commit --no-verbose` => verbose = 0 A few more things I noticed about this are: - Many uses of COUNTUP has 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". So it seems a safe and sensible change. Even though I suspect that the justification can be written more clearly, I am not sure if it worth the extra effort. > > 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. > --- > Documentation/technical/api-parse-options.txt | 6 ++++-- > parse-options.c | 8 +++++++- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt > index 5f0757d..a908d8a 100644 > --- a/Documentation/technical/api-parse-options.txt > +++ b/Documentation/technical/api-parse-options.txt > @@ -144,8 +144,10 @@ There are some macros to easily define options: > > `OPT_COUNTUP(short, long, &int_var, description)`:: > Introduce a count-up option. > - `int_var` is incremented on each use of `--option`, and > - reset to zero with `--no-option`. > + If the `int_var` is negative and `--option` is absent, > + then it will retain its value. Otherwise it will first set > + `int_var` to 0 and then increment it on each use of `--option`, > + and reset to zero with `--no-option`. > > `OPT_BIT(short, long, &int_var, description, mask)`:: > Introduce a boolean option. > diff --git a/parse-options.c b/parse-options.c > index 47a9192..86d30bd 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p, > return 0; > > case OPTION_COUNTUP: > - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > + if (unset) > + *(int *)opt->value = 0; > + else { > + if (*(int *)opt->value < 0) > + *(int *)opt->value = 0; > + *(int *)opt->value += 1; > + } > return 0; > > case OPTION_SET_INT: > > -- > https://github.com/git/git/pull/213 ``` ## Pranit Bauva, 2016-03-19 04:55 Subject: Re: [PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values Message-ID: URL: https://gitlist.dev/e/CAFZEwPNkWYC4nj6bjQd%2BHYxSVjm_UQwGF5UQwOxziA8K6iuszg%40mail.gmail.com In-Reply-To: ``` On Sat, Mar 19, 2016 at 4:29 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> The reason to make it consider negative values or more specifically >> "unspecified" values is to give the ability to differentiate between >> once, multiple time or with --no-option. >> >> Eg. : >> initialize verbose = -1 >> `git commit` => verbose = -1 >> `git commit -v` => verbose = 1 >> `git commit -v -v` => verbose = 2 >> `git commit --no-verbose` => verbose = 0 > > A few more things I noticed about this are: > > - Many uses of COUNTUP has 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". > > So it seems a safe and sensible change. Even though I suspect that > the justification can be written more clearly, I am not sure if it > worth the extra effort. I can include this paragraph. Some people might have a bit difficulty in knowing the importance as they might not have followed my earlier patches. To bridge this gap, I can include your paragraph in the commit message. Any other particular things you want me to justify? I will be glad to send another re-roll of this. Regards, Pranit Bauva ``` ## Eric Sunshine, 2016-03-20 03:56 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcShT2aPkoRK0pXPzbRbQqCU%2BO5uEqEr4oMkZ8LTwREaAA%40mail.gmail.com In-Reply-To: <010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com> ``` On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva wrote: > Add commit.verbose configuration variable as a convenience for those > who always prefer --verbose. > > Signed-off-by: Pranit Bauva > --- > diff --git a/builtin/commit.c b/builtin/commit.c > @@ -1654,6 +1661,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > + if (verbose < 0) { > + if (config_verbose > -1) > + verbose = config_verbose; > + else > + verbose = 0; > + } I think it's more common in this codebase to compare against -1 directly rather than <0, so: if (verbose == -1) { if (config_verbose != -1) verbose = config_verbose; else verbose = 0; } Or, this might be easier to read: if (verbose == -1) verbose = config_verbose; if (verbose == -1) verbose = 0; But, this likely isn't better: if (verbose == -1) verbose = config_verbose == -1 ? 0 : config_verbose; Anyhow, probably not worth a re-roll. > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > @@ -96,4 +96,59 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' > +test_expect_success 'commit.verbose true and --verbose' ' > + ( > + GIT_EDITOR=cat && > + export GIT_EDITOR && > + git -c commit.verbose=true commit --amend --verbose Easier would be to write this as: GIT_EDITOR=cat git -c commit.verbose=true commit --amend --verbose and then you wouldn't need the subhsell. However, more intuitive would probably be to create another "editor" similar to the 'check-for-diff' editor this script already uses. (The 'check-for-diff' editor is an obvious example about how to go about such an undertaking.) You would need to invoke 'test_set_editor' in a subshell for this particular test in order to avoid clobbering the global editor used by this script. Or, have a preparatory patch which ditches the global setting of the editor and has each test invoke 'test_set_editor' as needed (and only if needed). Same comments apply to the other new tests which use a custom "editor". > + ) && > + grep "^diff --git" .git/COMMIT_EDITMSG >out && > + wc -l out | grep "1" > +' > + > +test_expect_success 'commit.verbose true and -v -v' ' > + ( > + GIT_EDITOR=cat && > + export GIT_EDITOR && > + git -c commit.verbose=true commit --amend -v -v > + ) && > + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && > + wc -l out | grep "2" > +' > + > +test_expect_success 'commit.verbose true and --no-verbose' ' > + test_must_fail git -c commit.verbose=true commit --amend --no-verbose > +' > + > +test_expect_success 'commit.verbose false and --verbose' ' > + git -c commit.verbose=false commit --amend --verbose > +' > + > +test_expect_success 'commit.verbose false and -v -v' ' > + ( > + GIT_EDITOR=cat && > + export GIT_EDITOR && > + git -c commit.verbose=false commit --amend -v -v > + ) && > + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && > + wc -l out | grep "2" > +' > + > +test_expect_success 'commit.verbose false and --verbose omitted' ' > + test_must_fail git -c commit.verbose=false commit --amend > +' > + > +test_expect_success 'commit.verbose false and --no-verbose' ' > + test_must_fail git -c commit.verbose=false commit --amend --no-verbose > +' > + > +test_expect_success 'status ignores commit.verbose=true' ' > + git -c commit.verbose=true status >actual && > + ! grep "^diff --git" actual > +' > + > test_done ``` ## Pranit Bauva, 2016-03-20 11:05 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPMZk9jDpi4F9WiYhv4aTqFqn3127i1AUKtNJXwG-11tyw%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote: > On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva wrote: >> Add commit.verbose configuration variable as a convenience for those >> who always prefer --verbose. >> >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/builtin/commit.c b/builtin/commit.c >> @@ -1654,6 +1661,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> + if (verbose < 0) { >> + if (config_verbose > -1) >> + verbose = config_verbose; >> + else >> + verbose = 0; >> + } > > I think it's more common in this codebase to compare against -1 > directly rather than <0, so: > > if (verbose == -1) { > if (config_verbose != -1) > verbose = config_verbose; > else > verbose = 0; > } > > Or, this might be easier to read: > > if (verbose == -1) > verbose = config_verbose; > > if (verbose == -1) > verbose = 0; > > But, this likely isn't better: > > if (verbose == -1) > verbose = config_verbose == -1 ? 0 : config_verbose; > > Anyhow, probably not worth a re-roll. I will note this for future patches. > >> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >> @@ -96,4 +96,59 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' >> +test_expect_success 'commit.verbose true and --verbose' ' >> + ( >> + GIT_EDITOR=cat && >> + export GIT_EDITOR && >> + git -c commit.verbose=true commit --amend --verbose > > Easier would be to write this as: > > GIT_EDITOR=cat git -c commit.verbose=true commit --amend --verbose > > and then you wouldn't need the subhsell. True. I will update this. > However, more intuitive would probably be to create another "editor" > similar to the 'check-for-diff' editor this script already uses. (The > 'check-for-diff' editor is an obvious example about how to go about > such an undertaking.) You would need to invoke 'test_set_editor' in a > subshell for this particular test in order to avoid clobbering the > global editor used by this script. Or, have a preparatory patch which > ditches the global setting of the editor and has each test invoke > 'test_set_editor' as needed (and only if needed). I guess it would complicate things as sometimes I need to check whether it has 1 line and sometimes 2 lines. > Same comments apply to the other new tests which use a custom "editor". > >> + ) && >> + grep "^diff --git" .git/COMMIT_EDITMSG >out && >> + wc -l out | grep "1" >> +' >> + >> +test_expect_success 'commit.verbose true and -v -v' ' >> + ( >> + GIT_EDITOR=cat && >> + export GIT_EDITOR && >> + git -c commit.verbose=true commit --amend -v -v >> + ) && >> + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && >> + wc -l out | grep "2" >> +' >> + >> +test_expect_success 'commit.verbose true and --no-verbose' ' >> + test_must_fail git -c commit.verbose=true commit --amend --no-verbose >> +' >> + >> +test_expect_success 'commit.verbose false and --verbose' ' >> + git -c commit.verbose=false commit --amend --verbose >> +' >> + >> +test_expect_success 'commit.verbose false and -v -v' ' >> + ( >> + GIT_EDITOR=cat && >> + export GIT_EDITOR && >> + git -c commit.verbose=false commit --amend -v -v >> + ) && >> + grep "# Changes not staged for commit" .git/COMMIT_EDITMSG >out && >> + wc -l out | grep "2" >> +' >> + >> +test_expect_success 'commit.verbose false and --verbose omitted' ' >> + test_must_fail git -c commit.verbose=false commit --amend >> +' >> + >> +test_expect_success 'commit.verbose false and --no-verbose' ' >> + test_must_fail git -c commit.verbose=false commit --amend --no-verbose >> +' >> + >> +test_expect_success 'status ignores commit.verbose=true' ' >> + git -c commit.verbose=true status >actual && >> + ! grep "^diff --git" actual >> +' >> + >> test_done ``` ## Eric Sunshine, 2016-03-20 17:34 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSCrdxYb%3D0WOiAARrrPK07PeyTKsopPfwDOLAO4m3scnQ%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva wrote: > On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote: >> However, more intuitive would probably be to create another "editor" >> similar to the 'check-for-diff' editor this script already uses. (The >> 'check-for-diff' editor is an obvious example about how to go about >> such an undertaking.) You would need to invoke 'test_set_editor' in a >> subshell for this particular test in order to avoid clobbering the >> global editor used by this script. Or, have a preparatory patch which >> ditches the global setting of the editor and has each test invoke >> 'test_set_editor' as needed (and only if needed). > > I guess it would complicate things as sometimes I need to check > whether it has 1 line and sometimes 2 lines. It's not really a big complication. If I'm reading the patch correctly, you should be able to re-use the existing check-for-diff "editor" for the first of the new tests for which you're currently setting a custom GIT_EDITOR. To do so, you will need to modify check-for-diff to also count lines and assert that only one was found, which should work for the new test and continue working for the existing tests. Then, you just need to create one more "editor" for the two tests where you set custom GIT_EDITOR and expect two lines. By the way, I forgot to mention in the review that, rather than: wc -l out | grep "1" for counting lines in a test script, you'd use: test_line_count = 1 out However, if you're doing it in an "editor" (which I recommend), then you'd use: test $(wc -l URL: https://gitlist.dev/e/CAFZEwPNKozf%2BowtJtaE27MqQb3koL0L%3DHDnErVoWrTnQpieGgw%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote: > On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva wrote: >> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote: >>> However, more intuitive would probably be to create another "editor" >>> similar to the 'check-for-diff' editor this script already uses. (The >>> 'check-for-diff' editor is an obvious example about how to go about >>> such an undertaking.) You would need to invoke 'test_set_editor' in a >>> subshell for this particular test in order to avoid clobbering the >>> global editor used by this script. Or, have a preparatory patch which >>> ditches the global setting of the editor and has each test invoke >>> 'test_set_editor' as needed (and only if needed). >> >> I guess it would complicate things as sometimes I need to check >> whether it has 1 line and sometimes 2 lines. > > It's not really a big complication. If I'm reading the patch > correctly, you should be able to re-use the existing check-for-diff > "editor" for the first of the new tests for which you're currently > setting a custom GIT_EDITOR. To do so, you will need to modify > check-for-diff to also count lines and assert that only one was found, > which should work for the new test and continue working for the > existing tests. > > Then, you just need to create one more "editor" for the two tests > where you set custom GIT_EDITOR and expect two lines. > > By the way, I forgot to mention in the review that, rather than: > > wc -l out | grep "1" > > for counting lines in a test script, you'd use: > > test_line_count = 1 out > > However, if you're doing it in an "editor" (which I recommend), then you'd use: > > test $(wc -l > And, another "by the way": You can use the write_script() function to > simplify creation of the new "editor(s)". Thanks for a detailed explanation. > In fact, it would be nice to convert creation of the check-for-diff > "editor" to use write_script, as well. So, basically, I'm suggesting > splitting the current patch into three, where the first two are > preparatory cleanups: > > 1. use write_script() to create the check-for-diff "editor" rather > than creating it manually > > 2. drop the global test_set_editor() and instead have each test invoke > it as needed (and only if needed) > > 3. the current patch which adds new tests along with a new "editor" > > Alternatively, combine #1 and #2 into a single patch which drops the > global test_set_editor() and, as an aside, also does "while here, > let's use write_script() to create 'check-for'diff' rather than doing > so manually". These changes seem nice. I will update and send the patch. ``` ## Junio C Hamano, 2016-03-23 19:19 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqq7fgtc88i.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote: > ... >> Alternatively, combine #1 and #2 into a single patch which drops the >> global test_set_editor() and, as an aside, also does "while here, >> let's use write_script() to create 'check-for'diff' rather than doing >> so manually". > > These changes seem nice. I will update and send the patch. So, has anything happened to this topic or has it been abandoned? I am not in a hurry, just wanted to see if I need to keep the old one in my tree as a reminder to myself. ``` ## Pranit Bauva, 2016-03-23 19:23 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPM1tukRCewKCgvYMQNDRMWh4MXcrXAbdn4S3Hg8c1R%2BhA%40mail.gmail.com In-Reply-To: ``` On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote: >> ... >>> Alternatively, combine #1 and #2 into a single patch which drops the >>> global test_set_editor() and, as an aside, also does "while here, >>> let's use write_script() to create 'check-for'diff' rather than doing >>> so manually". >> >> These changes seem nice. I will update and send the patch. > > So, has anything happened to this topic or has it been abandoned? > > I am not in a hurry, just wanted to see if I need to keep the old > one in my tree as a reminder to myself. Sorry for that! Actually I am bit caught up with writing my proposal for GSoC 2016. I would be able to complete that in around an hour. Then will work on this. Then on the shell function -> C function porting patch. Please bear with me for a little while. ``` ## Junio C Hamano, 2016-03-23 20:43 Subject: Re: [PATCH v8 2/2] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqq37rhc4cs.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano wrote: >> Pranit Bauva writes: >> >>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote: >>> ... >>>> Alternatively, combine #1 and #2 into a single patch which drops the >>>> global test_set_editor() and, as an aside, also does "while here, >>>> let's use write_script() to create 'check-for'diff' rather than doing >>>> so manually". >>> >>> These changes seem nice. I will update and send the patch. >> >> So, has anything happened to this topic or has it been abandoned? >> >> I am not in a hurry, just wanted to see if I need to keep the old >> one in my tree as a reminder to myself. > > Sorry for that! Actually I am bit caught up with writing my proposal > for GSoC 2016. I would be able to complete that in around an hour. > Then will work on this. Then on the shell function -> C function > porting patch. Please bear with me for a little while. Oh, no worries. Naturally the application/proposal should take higher priority. Thanks. ```