threads / patch / 41743

v8, 2 partscommit: add a commit.verbose config variable

Subject: [PATCH v8 2/2] commit: add a commit.verbose config variable

## tl;dr

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

replies: 10people: 3as markdown or json

Pranit Bauva· Mar 18, 2016, 21:19 UTC · lore

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

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

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

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027

Previous version of the patch: [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061

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

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

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 5f0757d..a908d8a 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -144,8 +144,10 @@ There are some macros to easily define options:
 
 `OPT_COUNTUP(short, long, &int_var, description)`::
 	Introduce a count-up option.
-	`int_var` is incremented on each use of `--option`, and
-	reset to zero with `--no-option`.
+	If the `int_var` is negative and `--option` is absent,
+	then it will retain its value. Otherwise it will first set
+	`int_var` to 0 and then increment it on each use of `--option`,
+	and reset to zero with `--no-option`.
 
 `OPT_BIT(short, long, &int_var, description, mask)`::
 	Introduce a boolean option.
diff --git a/parse-options.c b/parse-options.c
index 47a9192..86d30bd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_COUNTUP:
-		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
+		if (unset)
+			*(int *)opt->value = 0;
+		else {
+			if (*(int *)opt->value < 0)
+				*(int *)opt->value = 0;
+			*(int *)opt->value += 1;
+		}
 		return 0;
 
 	case OPTION_SET_INT:

--
https://github.com/git/git/pull/213
Pranit Bauva· Mar 18, 2016, 21:19 UTC · re: Pranit Bauva · lore

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

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

--- The previous version of the patch are:

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

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..1d0ec2e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1110,6 +1110,10 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.verbose::
+	A boolean or int to specify the level of verbose with `git commit`.
+	See linkgit:git-commit[1].
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9ec6b3c..d474226 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].
 	what changes the commit has.
 	Note that this diff output doesn't have its
 	lines prefixed with '#'. This diff will not be a part
-	of the commit message.
+	of the commit message. See the `commit.verbose` configuration
+	variable in linkgit:git-config[1].
 +
 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged
diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..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
Eric Sunshine· Mar 20, 2016, 03:56 UTC · re: Pranit Bauva · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 13 quoted lines
> Add commit.verbose configuration variable as a convenience for those
> who always prefer --verbose.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> 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.
Show 7 quoted lines
> 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".
Show 47 quoted lines
> +       ) &&
> +       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· Mar 20, 2016, 11:05 UTC · re: Eric Sunshine · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 39 quoted lines
> On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Add commit.verbose configuration variable as a convenience for those
>> who always prefer --verbose.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> 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.
Show 14 quoted lines
>
>> 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.
Show 8 quoted lines
> 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.

Show 49 quoted 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· Mar 20, 2016, 17:34 UTC · re: Pranit Bauva · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 12 quoted lines
> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@sunshineco.com> 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 <out) = 1

And, another "by the way": You can use the write_script() function to simplify creation of the new "editor(s)".

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

Pranit Bauva· Mar 20, 2016, 18:02 UTC · re: Eric Sunshine · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 39 quoted lines
> On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine <sunshine@sunshineco.com> 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 <out) = 1
>
> 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.
Show 17 quoted lines
> 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· Mar 23, 2016, 19:19 UTC · re: Pranit Bauva · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 8 quoted lines
> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine <sunshine@sunshineco.com> 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· Mar 23, 2016, 19:23 UTC · re: Junio C Hamano · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine <sunshine@sunshineco.com> 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· Mar 23, 2016, 20:43 UTC · re: Pranit Bauva · lore

Re: [PATCH v8 2/2] commit: add a commit.verbose config variable

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 21 quoted lines
> On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>
>>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine <sunshine@sunshineco.com> 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.
Junio C Hamano· Mar 18, 2016, 22:59 UTC · re: Pranit Bauva · lore

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

Pranit Bauva <pranit.bauva@gmail.com> writes:
Show 10 quoted lines
> The reason to make it consider negative values or more specifically
> "unspecified" values is to give the ability to differentiate between
> once, multiple time or with --no-option.
>
> Eg. :
> initialize verbose = -1
> `git commit` => verbose = -1
> `git commit -v` => verbose = 1
> `git commit -v -v` => verbose = 2
> `git commit --no-verbose` => verbose = 0
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.

Show 57 quoted lines
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> The discussion about this patch:
> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027
>
> Previous version of the patch:
> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061
>
> Changes introduced:
> Use a different language in commit message to make the change and its
> utility more clear.
> ---
>  Documentation/technical/api-parse-options.txt | 6 ++++--
>  parse-options.c                               | 8 +++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 5f0757d..a908d8a 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -144,8 +144,10 @@ There are some macros to easily define options:
>  
>  `OPT_COUNTUP(short, long, &int_var, description)`::
>  	Introduce a count-up option.
> -	`int_var` is incremented on each use of `--option`, and
> -	reset to zero with `--no-option`.
> +	If the `int_var` is negative and `--option` is absent,
> +	then it will retain its value. Otherwise it will first set
> +	`int_var` to 0 and then increment it on each use of `--option`,
> +	and reset to zero with `--no-option`.
>  
>  `OPT_BIT(short, long, &int_var, description, mask)`::
>  	Introduce a boolean option.
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192..86d30bd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>  		return 0;
>  
>  	case OPTION_COUNTUP:
> -		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> +		if (unset)
> +			*(int *)opt->value = 0;
> +		else {
> +			if (*(int *)opt->value < 0)
> +				*(int *)opt->value = 0;
> +			*(int *)opt->value += 1;
> +		}
>  		return 0;
>  
>  	case OPTION_SET_INT:
>
> --
> https://github.com/git/git/pull/213
Pranit Bauva· Mar 19, 2016, 04:55 UTC · re: Junio C Hamano · lore

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

On Sat, Mar 19, 2016 at 4:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Pranit Bauva <pranit.bauva@gmail.com> 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

← back to recent threads