[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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Remi Galan Alfonso]

Hi Mehul,

Mehul Jain <[hidden email]> 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 <[hidden email]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Mehul Jain]

Hi Remi,

Thanks for your input.

On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
<[hidden email]> wrote:
> Hi Mehul,
>
> Mehul Jain <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Remi Galan Alfonso]

Mehul Jain <[hidden email]> writes:

> Hi Remi,
>
> Thanks for your input.
>
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
> <[hidden email]> wrote:
> > Hi Mehul,
> >
> > Mehul Jain <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Mehul Jain]

On Thu, May 26, 2016 at 9:13 PM, Remi Galan Alfonso
<[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Mehul Jain]

Hi,

Thanks for your input.

On Thu, May 26, 2016 at 10:02 PM, Jeff King <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Junio C Hamano]

Jeff King <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html