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

[Mehul Jain]

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

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

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

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
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Remi Galan Alfonso]

Mehul Jain  writes:
> Hi Remi,
> 
> Thanks for your input.
> 
> On Thu, May 26, 2016 at 7:12 PM, Remi Galan Alfonso
>  wrote:
> > Hi Mehul,
> >
> > Mehul Jain  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
 wrote:
> Hi Mehul,
>
> Mehul Jain  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Remi Galan Alfonso]

Hi Mehul,

Mehul Jain  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 
> ---
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html