[PATCH v3 0/3] Introduce log.showSignature config variable

[PATCH v3 0/3] Introduce log.showSignature config variable

[Mehul Jain]

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

Changes compared to v2:
	* A preparatory patch 1/3 has been introduced so that tests
	  in patches 2/3 and 3/3 can take advantage of it.
	* Mistake regarding branch in [patch v2 2/2] has been
	  corrected.
	* Tight coupling between the tests in [patch v2 2/2] has
	  been resovled.

I would like to thanks Eric Sunshine for his feedback on previous
series [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/297648 

Mehul Jain (3):
  t4202: refactoring of test
  log: add "--no-show-signature" command line option
  log: add log.showSignature configuration variable

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

-- 
2.9.0.rc0.dirty

[PATCH v3 1/3] t4202: refactor test

[Mehul Jain]

Separate the creation of 'signed' branch so that other tests can take
advantage of this branch.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t4202-log.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 128ba93..ab66ee0 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -860,12 +860,15 @@ test_expect_success 'dotdot is a parent directory' '
 	test_cmp expect actual
 '
 
-test_expect_success GPG 'log --graph --show-signature' '
+test_expect_success 'setup signed branch' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b signed master &&
 	echo foo >foo &&
 	git add foo &&
-	git commit -S -m signed_commit &&
+	git commit -S -m signed_commit
+'
+
+test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
 	grep "^| gpg: Good signature" actual
-- 
2.9.0.rc0.dirty

[PATCH v3 1/3] t4202: refactor test

[Mehul Jain]

Subsequent patches will want to reuse the 'signed' branch that the
'log --graph --show-signature' test creates and uses.

Split the set-up part into a test of its own, and make the existing
test into a separate one that only inspects the history on the 'signed'
branch. This way, it becomes clearer that tests added by subsequent
patches reuse the 'signed' branch in the same way.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t4202-log.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 128ba93..ab66ee0 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -860,12 +860,15 @@ test_expect_success 'dotdot is a parent directory' '
 	test_cmp expect actual
 '
 
-test_expect_success GPG 'log --graph --show-signature' '
+test_expect_success GPG 'setup signed branch' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b signed master &&
 	echo foo >foo &&
 	git add foo &&
-	git commit -S -m signed_commit &&
+	git commit -S -m signed_commit
+'
+
+test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
 	grep "^| gpg: Good signature" actual
-- 
2.9.0.rc0.dirty

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

[Mehul Jain]

If an user creates an alias with "--show-signature" early in command
line, e.g.
	[alias] logss = log --show-signature

then there is no way to countermand it through command line.

Teach git-log and related commands about "--no-show-signature" command
line option. This will make "git logss --no-show-signature" run
without showing GPG signature.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 revision.c     | 2 ++
 t/t4202-log.sh | 5 +++++
 2 files changed, 7 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 ab66ee0..93a82e9 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -893,6 +893,11 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success GPG '--no-show-signature overrides --show-signature' '
+	git log -1 --show-signature --no-show-signature signed >actual &&
+	! grep "^gpg:" actual
+'
+
 test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
-- 
2.9.0.rc0.dirty

[PATCH v3 3/3] log: add log.showSignature configuration variable

[Mehul Jain]

Users may want to always use "--show-signature" while using git-log and
related commands.

When log.showSignature is set to true, git-log and related commands will
behave as if "--show-signature" was given to them.

Note that this config variable is meant to affect git-log, git-show,
git-whatchanged and git-reflog. Other commands like git-format-patch,
git-rev-list are not to be affected by this config variable.

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

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 03f9580..bbb5adc 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 related commands will act as if the
+	`--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 93a82e9..ecac186 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -898,6 +898,26 @@ test_expect_success GPG '--no-show-signature overrides --show-signature' '
 	! grep "^gpg:" actual
 '
 
+test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
+	test_config log.showsignature true &&
+	git log -1 signed >actual &&
+	grep "gpg: Signature made" actual &&
+	grep "gpg: Good signature" actual
+'
+
+test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
+	test_config log.showsignature true &&
+	git log -1 --no-show-signature signed >actual &&
+	! grep "^gpg:" actual
+'
+
+test_expect_success GPG '--show-signature overrides log.showsignature=false' '
+	test_config log.showsignature false &&
+	git log -1 --show-signature signed >actual &&
+	grep "gpg: Signature made" actual &&
+	grep "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..6e839f5 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' '
+	test_config log.showsignature true &&
+	git show initial >actual &&
+	grep "gpg: Signature made" actual &&
+	grep "gpg: Good signature" actual
+'
+
 test_done
-- 
2.9.0.rc0.dirty

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Junio C Hamano]

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

> Add a new configuratation variable "log.showSignature" for git-log
> and related commands. "log.showSignature=true" will enable user to
> see GPG signature by default for git-log and related commands.
>
> Changes compared to v2:
> 	* A preparatory patch 1/3 has been introduced so that tests
> 	  in patches 2/3 and 3/3 can take advantage of it.

It is unclear how this change allows the remainder to "take
advanrage" to me.  Earlier, "signed" branch was created only when
the GPG prerequisite is met and with this change the branch is
always created, which is the only change as far as I can see.  But
the tests that are added in 2 and 3 are all protected with the GPG
prerequiste.

Besides, the invocation of "git commit -S" after this change is no
longer protected by the GPG prerequisite and it may even cause the
'setup' step to fail on a host without GPG.

What am I missing?  I do not quite see any reason to take 1/2; I
only see a possible downside without any upside.

The main two steps 2&3 looked good.

Thanks.

> 	* Mistake regarding branch in [patch v2 2/2] has been
> 	  corrected.
> 	* Tight coupling between the tests in [patch v2 2/2] has
> 	  been resovled.
>
> I would like to thanks Eric Sunshine for his feedback on previous
> series [1].



>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/297648 
>
> Mehul Jain (3):
>   t4202: refactoring of test
>   log: add "--no-show-signature" command line option
>   log: add log.showSignature configuration variable
>
>  Documentation/git-log.txt |  4 ++++
>  builtin/log.c             |  6 ++++++
>  revision.c                |  2 ++
>  t/t4202-log.sh            | 32 ++++++++++++++++++++++++++++++--
>  t/t7510-signed-commit.sh  |  7 +++++++
>  5 files changed, 49 insertions(+), 2 deletions(-)

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Mehul Jain]

On Thu, Jun 23, 2016 at 2:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> Add a new configuratation variable "log.showSignature" for git-log
>> and related commands. "log.showSignature=true" will enable user to
>> see GPG signature by default for git-log and related commands.
>>
>> Changes compared to v2:
>>       * A preparatory patch 1/3 has been introduced so that tests
>>         in patches 2/3 and 3/3 can take advantage of it.
>
> It is unclear how this change allows the remainder to "take
> advanrage" to me.  Earlier, "signed" branch was created only when
> the GPG prerequisite is met and with this change the branch is
> always created, which is the only change as far as I can see.  But
> the tests that are added in 2 and 3 are all protected with the GPG
> prerequiste.
>
> Besides, the invocation of "git commit -S" after this change is no
> longer protected by the GPG prerequisite and it may even cause the
> 'setup' step to fail on a host without GPG.

I overlooked the GPG prerequisite when I created the "setup signed
branch" test in patch 1/3. I will send a patch to  rectify it ones
everyone agree with the idea behind this patch.

In patch 2/3 and 3/3, there are many tests which requires a branch
similar to that of "signed" branch, i.e. a branch with a commit having
GPG signature. So previously in v2, I created two new branches,
"test_sign" and "no_sign", which are identical to that of "signed"
branch. And with these branches, I wrote the tests in patch 2/3
and 3/3.

As suggested by Eric [1], rather than creating new branches, I
can take advantage of "signed" branch which already exists.
So, I created a new test to separate the creation of "signed" branch
from existing test "log --graph --show-signature". This was done
because I do not want new tests to depend on this test. If in future
someone changes this test then it will affect new tests introduced
in 2/3 and 3/3.

Now the new tests and existing one ("log --graph ... ") are using a
single branch "signed" to do there work.

If changing an existing test is not well justified here, then I can create
setup test for new tests only, without affecting the existing test.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/297648

Thanks,
Mehul

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Junio C Hamano]

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

> In patch 2/3 and 3/3, there are many tests which requires a branch
> similar to that of "signed" branch, i.e. a branch with a commit having
> GPG signature. So previously in v2, I created two new branches,
> "test_sign" and "no_sign", which are identical to that of "signed"
> branch. And with these branches, I wrote the tests in patch 2/3
> and 3/3.
>
> As suggested by Eric [1], rather than creating new branches, I
> can take advantage of "signed" branch which already exists.

Yeah, I understand that part.  But you do not _need_ to do the split
you do in 1/3 in order to reuse "signed".

The first 'log --graph --signature' test may fail, but it is not
like that the test without 1/3 removes the "signed" branch when
"log" command or the signature tests of its output fail, so "I
didn't want the later tests to depend on the first test being
successful" does not quite justify the split.  If "commit -S" step,
which 1/3 splits into the earlier one of the two, fails, then you
won't have a signed commit to use in the later tests, even though
you have "signed" branch.  The split of the tests done by 1/3 does
not make the tests more robust.  If the commands in the original
in the part of the first test in the original that you keep in the
first half with 1/3 fail, subsequent tests will be affected with or
without 1/3.

If 1/3 justifies the change a bit differently, it would become a
good one.

    In 2/3 and 3/3, we will use the same 'signed' branch that the
    first test for 'log --graph --show-signature' uses.  This branch
    is currently created in that 'log --graph --show-signature' test
    itself.

    Split the set-up part into a test of its own, and make the
    existing first test into a separate one that only inspects the
    history on the 'signed' branch.  That way, it would become
    clearer that later tests added by 2/3 and 3/3 reuse the 'signed'
    branch in the same way this 'log --graph --show-signature' uses
    that same branch.

Of course, you would need to keep GPG prerequisite for both halves
1/3 creates.

Thanks.

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Mehul Jain]

On Thu, Jun 23, 2016 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> In patch 2/3 and 3/3, there are many tests which requires a branch
>> similar to that of "signed" branch, i.e. a branch with a commit having
>> GPG signature. So previously in v2, I created two new branches,
>> "test_sign" and "no_sign", which are identical to that of "signed"
>> branch. And with these branches, I wrote the tests in patch 2/3
>> and 3/3.
>>
>> As suggested by Eric [1], rather than creating new branches, I
>> can take advantage of "signed" branch which already exists.
>
> Yeah, I understand that part.  But you do not _need_ to do the split
> you do in 1/3 in order to reuse "signed".

If it's fine, then I think it would be OK to drop this 1/3. Without splitting
the 'log --graph --show-signature' in two test will also serve the
purpose for the new test to use the signed branch.

Should I send a new patch series with 1/3 dropped or you can do
it manually at your end?

Thanks,
Mehul

Re: [PATCH v3 0/3] Introduce log.showSignature config variable

[Eric Sunshine]

On Fri, Jun 24, 2016 at 5:21 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> On Thu, Jun 23, 2016 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>> In patch 2/3 and 3/3, there are many tests which requires a branch
>>> similar to that of "signed" branch, i.e. a branch with a commit having
>>> GPG signature. So previously in v2, I created two new branches,
>>> "test_sign" and "no_sign", which are identical to that of "signed"
>>> branch. And with these branches, I wrote the tests in patch 2/3
>>> and 3/3.
>>>
>>> As suggested by Eric [1], rather than creating new branches, I
>>> can take advantage of "signed" branch which already exists.
>>
>> Yeah, I understand that part.  But you do not _need_ to do the split
>> you do in 1/3 in order to reuse "signed".
>
> If it's fine, then I think it would be OK to drop this 1/3. Without splitting
> the 'log --graph --show-signature' in two test will also serve the
> purpose for the new test to use the signed branch.

My understanding of Junio's response is that the missing PGP
prerequisite along with a weak commit message make for poor
justification of patch 1/3, however, if you add the prerequisite and
use the commit message he proposed (reproduced below) then it becomes
sensible to retain 1/3.

    --->8---
    In 2/3 and 3/3, we will use the same 'signed' branch that the
    first test for 'log --graph --show-signature' uses.  This branch
    is currently created in that 'log --graph --show-signature' test
    itself.

    Split the set-up part into a test of its own, and make the
    existing first test into a separate one that only inspects the
    history on the 'signed' branch.  That way, it would become
    clearer that later tests added by 2/3 and 3/3 reuse the 'signed'
    branch in the same way this 'log --graph --show-signature' uses
    that same branch.
    --->8---