[RFC/PATCH 0/2] Introduce "log.showSignature" config variable

[RFC/PATCH 0/2] Introduce "log.showSignature" config variable

[Mehul Jain]

Add a new configuratation variable "log.showSignature" for git-log and
git-show. "log.showSignature=true" will enable user to see GPG signature
by default while using git-log and git-show.

[Patch 1/2] introduce the config variable along with some tests.
[Patch 2/2] tackles the problem: what if user wants to disable the
	    setting of "log.showSignature=true" using a command line
	    switch.

Previous discussion on this: http://thread.gmane.org/gmane.comp.version-control.git/295405

Mehul Jain (2):
  log: add "log.showsignature" configuration variable
  log: add "--no-show-signature" command line option

 Documentation/git-log.txt |  4 ++++
 builtin/log.c             |  6 ++++++
 revision.c                |  2 ++
 t/t4202-log.sh            | 26 ++++++++++++++++++++++++++
 t/t7510-signed-commit.sh  |  7 +++++++
 5 files changed, 45 insertions(+)

-- 
2.9.0.rc0.dirty

[RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Mehul Jain]

People may want to always use "--show-signature" while using "git log"
or "git show".

When log.showsignature set true, "git log" and "git show" will behave
as "--show-signature" was given to them.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 Documentation/git-log.txt |  4 ++++
 builtin/log.c             |  6 ++++++
 t/t4202-log.sh            | 19 +++++++++++++++++++
 t/t7510-signed-commit.sh  |  7 +++++++
 4 files changed, 36 insertions(+)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 03f9580..f39f800 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -196,6 +196,10 @@ log.showRoot::
 	`git log -p` output would be shown without a diff attached.
 	The default is `true`.
 
+log.showSignature::
+	If `true`, `git log` and `git show` will act as if `--show-signature`
+	option was passed to them.
+
 mailmap.*::
 	See linkgit:git-shortlog[1].
 
diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..7103217 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -33,6 +33,7 @@ static const char *default_date_mode = NULL;
 static int default_abbrev_commit;
 static int default_show_root = 1;
 static int default_follow;
+static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;
@@ -119,6 +120,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->show_signature = default_show_signature;
 	DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
 
 	if (default_date_mode)
@@ -409,6 +411,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		use_mailmap_config = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "log.showsignature")) {
+		default_show_signature = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (grep_config(var, value, cb) < 0)
 		return -1;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 128ba93..36be9a1 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
+	git checkout -b test_sign master &&
+	echo foo >foo &&
+	git add foo &&
+	git commit -S -m signed_commit &&
+	test_config log.showsignature true &&
+	git log -1 signed >actual &&
+	test_i18ngrep "gpg: Signature made" actual &&
+	test_i18ngrep "gpg: Good signature" actual
+'
+
+test_expect_success GPG '--show-signature overrides log.showsignature=false' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git config log.showsignature false &&
+	git log -1 --show-signature signed >actual &&
+	test_i18ngrep "gpg: Signature made" actual &&
+	test_i18ngrep "gpg: Good signature" actual
+'
+
 test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 4177a86..326dcc8 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -210,4 +210,11 @@ test_expect_success GPG 'show lack of signature with custom format' '
 	test_cmp expect actual
 '
 
+test_expect_success GPG 'log.showsignature behaves like --show-signature' '
+	git config log.showsignature true &&
+	git show initial > actual &&
+	test_i18ngrep "gpg: Signature made" actual &&
+	test_i18ngrep "gpg: Good signature" actual
+'
+
 test_done
-- 
2.9.0.rc0.dirty

[RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Mehul Jain]

If "log.showsignature=true", then there is no way to override it using
command line switch.

Teach "git log" and "git show" about "--no-show-signature" command line
option.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 revision.c     | 2 ++
 t/t4202-log.sh | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/revision.c b/revision.c
index d30d1c4..3546ff9 100644
--- a/revision.c
+++ b/revision.c
@@ -1871,6 +1871,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->notes_opt.use_default_notes = 1;
 	} else if (!strcmp(arg, "--show-signature")) {
 		revs->show_signature = 1;
+	} else if (!strcmp(arg, "--no-show-signature")) {
+		revs->show_signature = 0;
 	} else if (!strcmp(arg, "--show-linear-break") ||
 		   starts_with(arg, "--show-linear-break=")) {
 		if (starts_with(arg, "--show-linear-break="))
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 36be9a1..ea24259 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
 	test_i18ngrep "gpg: Good signature" actual
 '
 
+test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
+	git config log.showsignature true &&
+	git log -1 --no-show-signature signed >actual &&
+	test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
+	test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
+'
+
 test_expect_success GPG '--show-signature overrides log.showsignature=false' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git config log.showsignature false &&
-- 
2.9.0.rc0.dirty

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Remi Galan Alfonso]

Hi Mehul,

Mehul Jain <mehul.jain2029@gmail.com> writes:
> People may want to always use "--show-signature" while using "git log"
> or "git show".
> 
> When log.showsignature set true, "git log" and "git show" will behave

'When log.showsignature is set to true' ?

> as "--show-signature" was given to them.

s/as/as if

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  Documentation/git-log.txt |  4 ++++
>  builtin/log.c             |  6 ++++++
>  t/t4202-log.sh            | 19 +++++++++++++++++++
>  t/t7510-signed-commit.sh  |  7 +++++++
>  4 files changed, 36 insertions(+)
> [...]
> [...]
> +test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
> +        git checkout -b test_sign master &&
> +        echo foo >foo &&
> +        git add foo &&
> +        git commit -S -m signed_commit &&
> +        test_config log.showsignature true &&
> +        git log -1 signed >actual &&
> +        test_i18ngrep "gpg: Signature made" actual &&
> +        test_i18ngrep "gpg: Good signature" actual
> +'
> +
> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
> +        test_when_finished "git reset --hard && git checkout master" &&
> +        git config log.showsignature false &&

Any specific reason as to why you don't use test_config like in the
first test?

> +        git log -1 --show-signature signed >actual &&
> +        test_i18ngrep "gpg: Signature made" actual &&
> +        test_i18ngrep "gpg: Good signature" actual
> +'
> +
>  test_expect_success 'log --graph --no-walk is forbidden' '
>          test_must_fail git log --graph --no-walk
>  '
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 4177a86..326dcc8 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -210,4 +210,11 @@ test_expect_success GPG 'show lack of signature with custom format' '
>          test_cmp expect actual
>  '
>  
> +test_expect_success GPG 'log.showsignature behaves like --show-signature' '
> +        git config log.showsignature true &&

Same here.

> +        git show initial > actual &&

Style: no space after redirection.

Thanks,
Rémi

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Mehul Jain]

Hi Remi,

Thanks for your input.

On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Hi Mehul,
>
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>> When log.showsignature set true, "git log" and "git show" will behave
>
> 'When log.showsignature is set to true' ?

Pardon me, but I don't understand your question.
I think you are suggesting me to write
"When log.showsignature is set to true"
instead of
"When log.showsignature set true".

>> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
>> +        test_when_finished "git reset --hard && git checkout master" &&
>> +        git config log.showsignature false &&
>
> Any specific reason as to why you don't use test_config like in the
> first test?

None, actually. It was just that I forgot to use test_config while
writing the tests. I will make changes  accordingly as test_config
automatically unset the config variable, which is necessary.

Thanks,
Mehul

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Remi Galan Alfonso]

Mehul Jain <mehul.jain2029@gmail.com> writes:
> Hi Remi,
> 
> Thanks for your input.
> 
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
> <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> > Hi Mehul,
> >
> > Mehul Jain <mehul.jain2029@gmail.com> writes:
> >> When log.showsignature set true, "git log" and "git show" will behave
> >
> > 'When log.showsignature is set to true' ?
> 
> Pardon me, but I don't understand your question.
> I think you are suggesting me to write
> "When log.showsignature is set to true"
> instead of
> "When log.showsignature set true".

Sorry, I should have made explicit what went through my mind.
"When log.showsignature set true" doesn't sound right to me, while
"When log.showsignature is set to true" sounds better, however not
being a native english speaker maybe it's just me being wrong.

Thanks,
Rémi

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Mehul Jain]

On Thu, May 26, 2016 at 9:13 PM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Sorry, I should have made explicit what went through my mind.
> "When log.showsignature set true" doesn't sound right to me, while
> "When log.showsignature is set to true" sounds better, however not
> being a native english speaker maybe it's just me being wrong.

I think "When log.showsignature is set to true" is better.
The one that I phrased does sound bit strange. I also
not being a native English speaker, have a history of making
grammatical mistakes. :)

Thanks,
Mehul

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Jeff King]

On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:

> If "log.showsignature=true", then there is no way to override it using
> command line switch.
> 
> Teach "git log" and "git show" about "--no-show-signature" command line
> option.

I think this is teaching all of the revision machinery about it (which
is a good thing).

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 36be9a1..ea24259 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
>  	test_i18ngrep "gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
> +	git config log.showsignature true &&
> +	git log -1 --no-show-signature signed >actual &&
> +	test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
> +	test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
> +'

Perhaps it would be more robust to simply grep for "gpg:". We should not
be seeing any gpg-related lines in the output. It probably isn't that
big a deal in practice, though. If the output from gpg changes, this
test could report a false success, but all of the other nearby tests
would show a breakage, so somebody would probably notice.

-Peff

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Mehul Jain]

Hi,

Thanks for your input.

On Thu, May 26, 2016 at 10:02 PM, Jeff King <peff@peff.net> wrote:
> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 36be9a1..ea24259 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
>>       test_i18ngrep "gpg: Good signature" actual
>>  '
>>
>> +test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
>> +     git config log.showsignature true &&
>> +     git log -1 --no-show-signature signed >actual &&
>> +     test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
>> +     test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
>> +'
>
> Perhaps it would be more robust to simply grep for "gpg:". We should not
> be seeing any gpg-related lines in the output. It probably isn't that
> big a deal in practice, though. If the output from gpg changes, this
> test could report a false success, but all of the other nearby tests
> would show a breakage, so somebody would probably notice.

That's a very good point. I will make the changes accordingly.

Thanks,
Mehul

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Jeff King]

On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 03f9580..f39f800 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -196,6 +196,10 @@ log.showRoot::
>  	`git log -p` output would be shown without a diff attached.
>  	The default is `true`.
>  
> +log.showSignature::
> +	If `true`, `git log` and `git show` will act as if `--show-signature`
> +	option was passed to them.

This should be:

  ...if the `--show-signature` option was...

or:

  ...if `--show-signature` was...

Either is correct; you just need an article when not referring directly
to the option by its name.

The documentation here mentions "log" and "show". But I think this will
affect other programs, too, including "whatchanged" and "reflog". Those
ones are probably good, but the documentation is a little misleading (I
think other options just say "git-log and related commands" or
something).

I thought at first it would affect format-patch, too, which would be
weird. But in that command we _do_ parse the variable and end up setting
default_show_signature, but we never call cmd_log_init_defaults(), which
is what copies that value into the rev_info struct. That's kind of a
weird way to split it, but it's certainly not something you introduced
here.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 128ba93..36be9a1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>  	grep "^| | gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
> +	git checkout -b test_sign master &&
> +	echo foo >foo &&
> +	git add foo &&
> +	git commit -S -m signed_commit &&
> +	test_config log.showsignature true &&
> +	git log -1 signed >actual &&
> +	test_i18ngrep "gpg: Signature made" actual &&
> +	test_i18ngrep "gpg: Good signature" actual
> +'

You can see in the context that we do not use test_i18ngrep for finding
gpg output in existing tests. I'm not sure if the new tests should be
consistent, or if they should be changed to use test_i18ngrep. I don't
think it's actually doing anything here, though. It's used with a
git-specific GETTEXT_POISON flag that tweaks the output generated by
git, but not by sub-programs like gpg.

> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
> +	test_when_finished "git reset --hard && git checkout master" &&
> +	git config log.showsignature false &&

Should this be test_config?

> +test_expect_success GPG 'log.showsignature behaves like --show-signature' '
> +	git config log.showsignature true &&

Ditto here.

-Peff

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Jeff King]

On Thu, May 26, 2016 at 10:12:30PM +0530, Mehul Jain wrote:

> On Thu, May 26, 2016 at 10:02 PM, Jeff King <peff@peff.net> wrote:
> > On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
> >> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> >> index 36be9a1..ea24259 100755
> >> --- a/t/t4202-log.sh
> >> +++ b/t/t4202-log.sh
> >> @@ -901,6 +901,13 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
> >>       test_i18ngrep "gpg: Good signature" actual
> >>  '
> >>
> >> +test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
> >> +     git config log.showsignature true &&
> >> +     git log -1 --no-show-signature signed >actual &&
> >> +     test "$(test_i18ngrep "gpg: Signature made" actual)" = "" &&
> >> +     test "$(test_i18ngrep "gpg: Good signature" actual)" = ""
> >> +'
> >
> > Perhaps it would be more robust to simply grep for "gpg:". We should not
> > be seeing any gpg-related lines in the output. It probably isn't that
> > big a deal in practice, though. If the output from gpg changes, this
> > test could report a false success, but all of the other nearby tests
> > would show a breakage, so somebody would probably notice.
> 
> That's a very good point. I will make the changes accordingly.

While you are here, note that test_i18ngrep can already do the
"negative" grep, like:

  test_i18ngrep ! "^gpg:" actual

Though see my comments in the other part of the thread; I'm not sure
it's worth using i18ngrep at all.

-Peff

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>
>> If "log.showsignature=true", then there is no way to override it using
>> command line switch.
>> 
>> Teach "git log" and "git show" about "--no-show-signature" command line
>> option.
>
> I think this is teaching all of the revision machinery about it (which
> is a good thing).

I agree that the proposed commit log message should be updated to
say so.

Because we do not want .showsignature configuration to affect
rev-list nor format-patch, and we will not make "--show-sig" the
default for them either.  From that point of view, there is no
reason for them to know about the "--no-show-signature" option.

The only reason why teaching the "--no-show-signature" option to
these commands is a good idea is because it would help people who
create an alias with "--show-sig" in early part of the command line,
e.g.

	[alias] fp = format-patch --show-signature

by allowing them to countermand with --no-show-signature, i.e.

	$ git fp --no-show-signature ...

If we are updating the log message in the final submission of this
patch, we'd want it to be clear that the presence of this option is
not an excuse to introduce .showsignature that affects rev-list
later to make sure we do not have to waste our time rejecting such a
patch in the future.

Re: [RFC/PATCH 0/2] Introduce "log.showSignature" config variable

[Austin English]

On Thu, May 26, 2016 at 8:06 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> Add a new configuratation variable "log.showSignature" for git-log and
> git-show. "log.showSignature=true" will enable user to see GPG signature
> by default while using git-log and git-show.
>
> [Patch 1/2] introduce the config variable along with some tests.
> [Patch 2/2] tackles the problem: what if user wants to disable the
>             setting of "log.showSignature=true" using a command line
>             switch.
>
> Previous discussion on this: http://thread.gmane.org/gmane.comp.version-control.git/295405
>
> Mehul Jain (2):
>   log: add "log.showsignature" configuration variable
>   log: add "--no-show-signature" command line option
>
>  Documentation/git-log.txt |  4 ++++
>  builtin/log.c             |  6 ++++++
>  revision.c                |  2 ++
>  t/t4202-log.sh            | 26 ++++++++++++++++++++++++++
>  t/t7510-signed-commit.sh  |  7 +++++++
>  5 files changed, 45 insertions(+)
>
> --
> 2.9.0.rc0.dirty
>

Hi Mehul,

Thanks for working on this. With your patch and git config
log.showsignature = true, git log and git show now display signatures
for me:

austin@debian-laptop:~/src/winetricks$ ~/src/git/git config
log.showsignature true
austin@debian-laptop:~/src/winetricks$ ~/src/git/git log -n 1
commit 3399c38411259bf171fc32a3e145bc49fee2291e
gpg: Signature made Tue 10 May 2016 01:04:14 AM CDT using RSA key ID A041937B
gpg: Good signature from "Austin English (Austin English personal
gmail key) <austinenglish@gmail.com>"
Author: Austin English <austinenglish@gmail.com>
Date:   Tue May 10 01:04:14 2016 -0500

    release.sh: allow overridding version

austin@debian-laptop:~/src/winetricks$ ~/src/git/git show
commit 3399c38411259bf171fc32a3e145bc49fee2291e
gpg: Signature made Tue 10 May 2016 01:04:14 AM CDT using RSA key ID A041937B
gpg: Good signature from "Austin English (Austin English personal
gmail key) <austinenglish@gmail.com>"
Author: Austin English <austinenglish@gmail.com>
Date:   Tue May 10 01:04:14 2016 -0500

    release.sh: allow overridding version

diff --git a/src/release.sh b/src/release.sh
index 442df33..03a9462 100755
--- a/src/release.sh
+++ b/src/release.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Trivial release helper for winetricks
 #
+# Usage: $0 optional_version_name
+#
 # Copyright (C) 2016 Austin English
 #
 # This software comes with ABSOLUTELY NO WARRANTY.
@@ -19,7 +21,7 @@ if [ ! -f Makefile ] ; then
     exit 1
 fi

-version="$(date +%Y%m%d)"
+version="${1:-$(date +%Y%m%d)}"

 if git tag | grep ${version} ; then
     echo "A tag for ${version} already exists!"


Please CC me on future patches if you'd like me to test them. Thanks
again for your help!

-- 
-Austin

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Pranit Bauva]

Hey Mehul,

On Thu, May 26, 2016 at 8:34 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> Hi Remi,
>
> Thanks for your input.
>
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
> <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
>> Hi Mehul,
>>
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
>>> +        test_when_finished "git reset --hard && git checkout master" &&
>>> +        git config log.showsignature false &&
>>
>> Any specific reason as to why you don't use test_config like in the
>> first test?
>
> None, actually. It was just that I forgot to use test_config while
> writing the tests. I will make changes  accordingly as test_config
> automatically unset the config variable, which is necessary.

Or you could probably use 'git -c' which makes it all the more compact.

Regards,
Pranit Bauva

Re: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Mehul Jain]

On Thu, May 26, 2016 at 10:29 PM, Jeff King <peff@peff.net> wrote:
> On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:
> The documentation here mentions "log" and "show". But I think this will
> affect other programs, too, including "whatchanged" and "reflog". Those
> ones are probably good, but the documentation is a little misleading (I
> think other options just say "git-log and related commands" or
> something).

Yes, the documentation is misleading. As you have mentioned, this
config variable will affect git-log, git-show, git-whatchanged and git-reflog.
I will mention them in the documentation.

>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 128ba93..36be9a1 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>>       grep "^| | gpg: Good signature" actual
>>  '
>>
>> +test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
>> +     git checkout -b test_sign master &&
>> +     echo foo >foo &&
>> +     git add foo &&
>> +     git commit -S -m signed_commit &&
>> +     test_config log.showsignature true &&
>> +     git log -1 signed >actual &&
>> +     test_i18ngrep "gpg: Signature made" actual &&
>> +     test_i18ngrep "gpg: Good signature" actual
>> +'
>
> You can see in the context that we do not use test_i18ngrep for finding
> gpg output in existing tests. I'm not sure if the new tests should be
> consistent, or if they should be changed to use test_i18ngrep. I don't
> think it's actually doing anything here, though. It's used with a
> git-specific GETTEXT_POISON flag that tweaks the output generated by
> git, but not by sub-programs like gpg.

There was no real motivation behind usage of test_i18ngrep. Certainly,
usage of grep will fit in the context.

>> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
>> +     test_when_finished "git reset --hard && git checkout master" &&
>> +     git config log.showsignature false &&
>
> Should this be test_config?

Noted.

Thanks,
Mehul

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Mehul Jain]

On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>>
>>> If "log.showsignature=true", then there is no way to override it using
>>> command line switch.
>>>
>>> Teach "git log" and "git show" about "--no-show-signature" command line
>>> option.
>>
>> I think this is teaching all of the revision machinery about it (which
>> is a good thing).
>
> I agree that the proposed commit log message should be updated to
> say so.
>
> Because we do not want .showsignature configuration to affect
> rev-list nor format-patch, and we will not make "--show-sig" the
> default for them either.  From that point of view, there is no
> reason for them to know about the "--no-show-signature" option.
>
> The only reason why teaching the "--no-show-signature" option to
> these commands is a good idea is because it would help people who
> create an alias with "--show-sig" in early part of the command line,
> e.g.
>
>         [alias] fp = format-patch --show-signature
>
> by allowing them to countermand with --no-show-signature, i.e.
>
>         $ git fp --no-show-signature ...
>
> If we are updating the log message in the final submission of this
> patch, we'd want it to be clear that the presence of this option is
> not an excuse to introduce .showsignature that affects rev-list
> later to make sure we do not have to waste our time rejecting such a
> patch in the future.

Currently, with the [patch 1/2], only git-show, git-log, git-whatchanged
and git-reflog are able to learn about log.showsignature config variable.
But commands which will learn about "--no-show-signature" with
[patch 2/2] are notably a super-set of above mentioned commands.
Introduction of this option should not give an impression that we might
need log.showSignature for commands like git-format-patch etc, and
it will definitely be a wise decision to convey the same in the commit
message of this patch. I will do the necessary change.

Just out of curiosity, I was thinking that we might be able to teach
"--no-show-signature" option only to git-show, git-log, git-whatchanged
and git-reflog. To do this we can introduce a new member
"no_show_signature" in struct rev_info, and use this variable further
to modify the value of value of "rev.show_signature" after init_revision()
is called. This way we can selectively decide which commands should
learn about "--no-show-signature". This may be a bad idea because
we will have two variables in rev_info, for option --[no]-show-signature.
Any thoughts?

Thanks,
Mehul

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Junio C Hamano]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> The only reason why teaching the "--no-show-signature" option to
>> these commands is a good idea is because it would help people who
>> create an alias with "--show-sig" in early part of the command line,
>> e.g.
>>
>>         [alias] fp = format-patch --show-signature
>>
>> by allowing them to countermand with --no-show-signature, i.e.
>>
>>         $ git fp --no-show-signature ...
>> ...
>
> Just out of curiosity, I was thinking that we might be able to teach
> "--no-show-signature" option only to git-show, git-log, git-whatchanged
> and git-reflog.

Yeah, I know it is possible with extra code, but I do not think of a
good reason why it is necessary.

Re: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Jeff King]

On Fri, May 27, 2016 at 10:37:16AM -0700, Junio C Hamano wrote:

> Mehul Jain <mehul.jain2029@gmail.com> writes:
> 
> > On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> >> The only reason why teaching the "--no-show-signature" option to
> >> these commands is a good idea is because it would help people who
> >> create an alias with "--show-sig" in early part of the command line,
> >> e.g.
> >>
> >>         [alias] fp = format-patch --show-signature
> >>
> >> by allowing them to countermand with --no-show-signature, i.e.
> >>
> >>         $ git fp --no-show-signature ...
> >> ...
> >
> > Just out of curiosity, I was thinking that we might be able to teach
> > "--no-show-signature" option only to git-show, git-log, git-whatchanged
> > and git-reflog.
> 
> Yeah, I know it is possible with extra code, but I do not think of a
> good reason why it is necessary.

Not only "not necessary" but "actively worse" in my opinion. We have
--show-signature in revision.c, and that is reason enough to have
--no-show-signature, in case anybody would want to countermand an
earlier request (whether from config that is soon to exist, or from a
previous --show-signature on the command line), or just because somebody
feels like making sure git is doing what they want without bothering to
check the defaults.

We add the "--no-" form by default for all of our bools parsed by
parse-options. The only reason it is not already here is that this
option parsing predates our use of parse-options, and nobody had
bothered to go back and add it. But doing so is a win simply for
consistency if nothing else, IMHO.

I actually think it would be nice to convert all of handle_revision_opt
to parse-options, but that's a non-trivial task. And I certainly
wouldn't want it to hold up this otherwise simple topic.

-Peff