threads / patch / 42689

v3, 3 partsIntroduce log.showSignature config variable

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

## tl;dr

10 messages between Jun 22, 2016 and Jun 24, 2016. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Mehul Jain· Jun 22, 2016, 16:51 UTC · lore

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
Mehul Jain· Jun 22, 2016, 16:51 UTC · re: Mehul Jain · lore

[PATCH v3 1/3] t4202: refactor test

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(-)
Show changes to t/t4202-log.sh +5 −2
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
Mehul Jain· Jun 24, 2016, 14:12 UTC · re: Mehul Jain · lore

[PATCH v3 1/3] t4202: refactor test

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(-)
Show changes to t/t4202-log.sh +5 −2
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
Mehul Jain· Jun 22, 2016, 16:51 UTC · re: Mehul Jain · lore

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

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(+)
Show changes to 2 files +7 −0

revision.c, t/t4202-log.sh

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
Mehul Jain· Jun 22, 2016, 16:51 UTC · re: Mehul Jain · lore

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

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(+)
Show changes to 4 files +37 −0

Documentation/git-log.txt, builtin/log.c, t/t4202-log.sh, t/t7510-signed-commit.sh

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
Junio C Hamano· Jun 22, 2016, 20:31 UTC · re: Mehul Jain · lore

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

Mehul Jain <mehul.jain2029@gmail.com> writes:
Show 7 quoted lines
> 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.
Show 7 quoted lines
> 	* 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].
Show 14 quoted lines
>
> [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(-)
Mehul Jain· Jun 23, 2016, 05:44 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 23, 2016 at 2:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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

Junio C Hamano· Jun 23, 2016, 06:32 UTC · re: Mehul Jain · lore

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

Mehul Jain <mehul.jain2029@gmail.com> writes:
Show 9 quoted lines
> 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.
Mehul Jain· Jun 24, 2016, 09:21 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 23, 2016 at 12:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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

Eric Sunshine· Jun 24, 2016, 12:25 UTC · re: Mehul Jain · lore

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

On Fri, Jun 24, 2016 at 5:21 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
Show 18 quoted lines
> 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---

← back to recent threads