Re: [PATCH] Add .gitconfig variable commit.gpg-sign

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Junio C Hamano]

Joel Jacobson <joel@trustly.com> writes:

> Signed-off-by: Joel Jacobson <joel@trustly.com>
> ---
>  builtin/commit.c | 4 +++-

No docs?  No tests?

As to the design, any regular configuration variable settings must
be overridable from the command line for a single invocation. Please
design an escape hatch in, for somebody who has this configuration
variable set, but does not want to sign this commit he is about to
make.

Also do we generally use dash in the configuration variable names?
I thought the norm was section.CamelCase.

> +       if (!strcmp(k, "commit.gpg-sign"))
> +               return git_config_string(&sign_commit, k, v);
>         if (!strcmp(k, "commit.cleanup"))
>                 return git_config_string(&cleanup_arg, k, v);
>
> --
> --to=gitster@pobox.com

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Joel Jacobson]

On Tue, Apr 23, 2013 at 12:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> No docs?  No tests?

Maybe simply adding this text to git-commit.txt,

The default can be changed by the 'commit.gpg-sign' configuration
variable (see linkgit:git-config[1]).

after,

-S[<keyid>]::
--gpg-sign[=<keyid>]::
GPG-sign commit.

would be sufficient?

Not sure what the proper way to test this,
could you please suggest any other unit test I could look at for inspiration?

> As to the design, any regular configuration variable settings must
> be overridable from the command line for a single invocation. Please
> design an escape hatch in, for somebody who has this configuration
> variable set, but does not want to sign this commit he is about to
> make.

Something like --no-gpg-sign?

> Also do we generally use dash in the configuration variable names?
> I thought the norm was section.CamelCase.

Since the command line long option is "gpg-sign", I thought it was best
to use exactly the same term in the configuration variable name to
avoid confusion. Is there any problem with dashes in variable names?

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Michael J Gruber]

Joel Jacobson venit, vidit, dixit 23.04.2013 02:00:
> On Tue, Apr 23, 2013 at 12:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> No docs?  No tests?
> 
> Maybe simply adding this text to git-commit.txt,
> 
> The default can be changed by the 'commit.gpg-sign' configuration
> variable (see linkgit:git-config[1]).
> 
> after,
> 
> -S[<keyid>]::
> --gpg-sign[=<keyid>]::
> GPG-sign commit.
> 
> would be sufficient?
> 
> Not sure what the proper way to test this,
> could you please suggest any other unit test I could look at for inspiration?
> 
>> As to the design, any regular configuration variable settings must
>> be overridable from the command line for a single invocation. Please
>> design an escape hatch in, for somebody who has this configuration
>> variable set, but does not want to sign this commit he is about to
>> make.
> 
> Something like --no-gpg-sign?
> 
>> Also do we generally use dash in the configuration variable names?
>> I thought the norm was section.CamelCase.
> 
> Since the command line long option is "gpg-sign", I thought it was best
> to use exactly the same term in the configuration variable name to
> avoid confusion. Is there any problem with dashes in variable names?
> 

Not really a problem, but as Junio writes, we don't use dashes in the
config.

As for the command line override:

Don't we have "git -c commit.gpgsign=false" and such these days, so that
we don't have to inflate options any more?

Michael

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Joel Jacobson <joel@trustly.com> writes:
>
>> Signed-off-by: Joel Jacobson <joel@trustly.com>
>> ---
>>  builtin/commit.c | 4 +++-
>
> No docs?  No tests?
>
> As to the design, any regular configuration variable settings must
> be overridable from the command line for a single invocation. Please
> design an escape hatch in, for somebody who has this configuration
> variable set, but does not want to sign this commit he is about to
> make.
>
> Also do we generally use dash in the configuration variable names?
> I thought the norm was section.CamelCase.

Another design issue you would need to consider before deciding to
add such a configuration is how it interacts with "git rebase -i".

The command internally runs "git commit --amend", etc. and I do not
know if _all_ users who set this configuration want to unconditionally
always re-sign all these commits.

There may be other commands that internally use "git commit" that
may be affected with such a configuration variable.

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> As for the command line override:
>
> Don't we have "git -c commit.gpgsign=false" and such these days, so that
> we don't have to inflate options any more?

I would consider such use of "git -c key=val" a last-resort escape
hatch to work around broken commands that do not implement a proper
escape hatch designed in to help users, unless the "key" is for
something very obscure and not meant for every-day use (read: not
deserving a proper command line override).

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Joel Jacobson]

On Tue, Apr 23, 2013 at 6:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I would consider such use of "git -c key=val" a last-resort escape
> hatch to work around broken commands that do not implement a proper
> escape hatch designed in to help users, unless the "key" is for
> something very obscure and not meant for every-day use (read: not
> deserving a proper command line override).

Agreed.

We already have --gpg-sign[=<keyid>], so I suggest --no-gpg-sign to
override commit.gpgsign.

Sounds good?

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Junio C Hamano]

Joel Jacobson <joel@trustly.com> writes:

> On Tue, Apr 23, 2013 at 6:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I would consider such use of "git -c key=val" a last-resort escape
>> hatch to work around broken commands that do not implement a proper
>> escape hatch designed in to help users, unless the "key" is for
>> something very obscure and not meant for every-day use (read: not
>> deserving a proper command line override).
>
> Agreed.
>
> We already have --gpg-sign[=<keyid>], so I suggest --no-gpg-sign to
> override commit.gpgsign.
>
> Sounds good?

Yup.

And then we would need to add the same option to existing callers of
"git commit" (such as "git rebase") to pass it down the callchain.

But stepping back a bit, I have a suspicion that your upstream
project _only_ cares about what you feed them (either by pushing
your work yourself to them, or telling them to pull from your
repository).  There is no reason for you to be constantly signing
your commits you make during your exploratory development that you
may throw-away in the end.

It _might_ be a better option to just teach "-S" option to "git
rebase" that tells it to replay all the commits with "commit -S",
instead of adding commit.gpgSign configuration.

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Joel Jacobson]

On Tue, Apr 23, 2013 at 8:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Yup.
>
> And then we would need to add the same option to existing callers of
> "git commit" (such as "git rebase") to pass it down the callchain.

Got it.

> But stepping back a bit, I have a suspicion that your upstream
> project _only_ cares about what you feed them (either by pushing
> your work yourself to them, or telling them to pull from your
> repository).  There is no reason for you to be constantly signing
> your commits you make during your exploratory development that you
> may throw-away in the end.

Your suspicions are correct.
But I'm a bit paranoid, so it feels better to sign even local commits.

> It _might_ be a better option to just teach "-S" option to "git
> rebase" that tells it to replay all the commits with "commit -S",
> instead of adding commit.gpgSign configuration.

In my case, I don't do that much exploratory development locally,
so I usually just commit, pull and push.

Always signing everything can't really hurt, can it? Takes a few clock
cycles more, and a few more bytes, but apart from that I don't see any
problems?

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Sebastian Götte]

On 04/23/2013 09:56 PM, Joel Jacobson wrote:
>> But stepping back a bit, I have a suspicion that your upstream
>> project _only_ cares about what you feed them (either by pushing
>> your work yourself to them, or telling them to pull from your
>> repository).  There is no reason for you to be constantly signing
>> your commits you make during your exploratory development that you
>> may throw-away in the end.
> 
> Your suspicions are correct.
> But I'm a bit paranoid, so it feels better to sign even local commits.
> 
>> It _might_ be a better option to just teach "-S" option to "git
>> rebase" that tells it to replay all the commits with "commit -S",
>> instead of adding commit.gpgSign configuration.
> 
> In my case, I don't do that much exploratory development locally,
> so I usually just commit, pull and push.
> 
> Always signing everything can't really hurt, can it? Takes a few clock
> cycles more, and a few more bytes, but apart from that I don't see any
> problems?
I have my GPG-keys password-protected, and I would be severely annoyed by
GnuPG password prompts popping up on every commit. I think the -S option
to rebase would be the more elegant way. What could be nice would be a
config option that makes "git push" warn/abort in case I try to push an
unsigned head commit to a repo where I want to have signed commits:
> remote.<name>.abortUnsigned
This of course needs an command line override switch.

Something to be considered is whether "git rebase -S" should sign *every*
commit in the series or only the *head* commit.

Re: [PATCH] Add .gitconfig variable commit.gpg-sign

[Michael J Gruber]

Sebastian Götte venit, vidit, dixit 24.04.2013 10:53:
> On 04/23/2013 09:56 PM, Joel Jacobson wrote:
>>> But stepping back a bit, I have a suspicion that your upstream
>>> project _only_ cares about what you feed them (either by pushing
>>> your work yourself to them, or telling them to pull from your
>>> repository).  There is no reason for you to be constantly signing
>>> your commits you make during your exploratory development that you
>>> may throw-away in the end.
>>
>> Your suspicions are correct.
>> But I'm a bit paranoid, so it feels better to sign even local commits.
>>
>>> It _might_ be a better option to just teach "-S" option to "git
>>> rebase" that tells it to replay all the commits with "commit -S",
>>> instead of adding commit.gpgSign configuration.
>>
>> In my case, I don't do that much exploratory development locally,
>> so I usually just commit, pull and push.
>>
>> Always signing everything can't really hurt, can it? Takes a few clock
>> cycles more, and a few more bytes, but apart from that I don't see any
>> problems?
> I have my GPG-keys password-protected, and I would be severely annoyed by
> GnuPG password prompts popping up on every commit. I think the -S option
> to rebase would be the more elegant way. What could be nice would be a
> config option that makes "git push" warn/abort in case I try to push an
> unsigned head commit to a repo where I want to have signed commits:
>> remote.<name>.abortUnsigned
> This of course needs an command line override switch.

This appears to be more suited for a server side hook (update), or a new
pre-push hook.

> Something to be considered is whether "git rebase -S" should sign *every*
> commit in the series or only the *head* commit.

The idea is probably to sign a commit that used to signed?

Otherwise, "git commit --amend -S" is your friend, either during rebase
(for individual commits) or after (for the head commit).

Michael

[PATCH 1/1] templates: pre-push hook: check for missing GPG signatures (was: Re: [PATCH] Add .gitconfig variable commit.gpg-sign)

[Sebastian Götte]

On 04/24/2013 11:51 AM, Michael J Gruber wrote:
> Sebastian Götte venit, vidit, dixit 24.04.2013 10:53:
>>                                          What could be nice would be a
>> config option that makes "git push" warn/abort in case I try to push an
>> unsigned head commit to a repo where I want to have signed commits:
>>> remote.<name>.abortUnsigned
>> This of course needs a command line override switch.
> 
> This appears to be more suited for a server side hook (update), or a new
> pre-push hook.
Ok, here it is ;)
I replaced the previous sample hook code because it did only check for commits
containing "WIP" in their messages which I think is not terribly useful (and
can easily be added to this script. I also added a missing colon that caused my
shell to complain about an empty if.
This patch applies to the current master as it requires the new GPG %G? pretty
placeholder output.

Signed-off-by: Sebastian Götte <jaseg@physik-pool.tu-berlin.de>
---
 templates/hooks--pre-push.sample | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
old mode 100644
new mode 100755
index 15ab6d8..08a72df
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -16,20 +16,19 @@
 #
 #   <local ref> <local sha1> <remote ref> <remote sha1>
 #
-# This sample shows how to prevent push of commits where the log message starts
-# with "WIP" (work in progress).
+# This sample shows how to prevent pushing commits without good GPG signatures
 
 remote="$1"
 url="$2"
 
 z40=0000000000000000000000000000000000000000
+ec=0
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
 	then
-		# Handle delete
+		: # Handle delete
 	else
 		if [ "$remote_sha" = $z40 ]
 		then
@@ -40,14 +39,13 @@ do
 			range="$remote_sha..$local_sha"
 		fi
 
-		# Check for WIP commit
-		commit=`git rev-list -n 1 --grep '^WIP' "$range"`
-		if [ -n "$commit" ]
-		then
-			echo "Found WIP commit in $local_ref, not pushing"
-			exit 1
-		fi
+		commits=`git log --format="%G? %h" "$range" | grep -v '^G' | cut -d\  -f2`
+		for commit in $commits
+		do
+			echo "Commit $commit does not have a good GPG signature"
+			ec=1
+		done
 	fi
 done
 
-exit 0
+exit $ec
-- 
1.8.2

Re: [PATCH 1/1] templates: pre-push hook: check for missing GPG signatures

[Junio C Hamano]

Sebastian Götte <jaseg@physik.tu-berlin.de> writes:

> On 04/24/2013 11:51 AM, Michael J Gruber wrote:
>> Sebastian Götte venit, vidit, dixit 24.04.2013 10:53:
>>>                                          What could be nice would be a
>>> config option that makes "git push" warn/abort in case I try to push an
>>> unsigned head commit to a repo where I want to have signed commits:
>>>> remote.<name>.abortUnsigned
>>> This of course needs a command line override switch.
>> 
>> This appears to be more suited for a server side hook (update), or a new
>> pre-push hook.
> Ok, here it is ;)
> I replaced the previous sample hook code because it did only check for commits
> containing "WIP" in their messages which I think is not terribly useful (and
> can easily be added to this script. I also added a missing colon that caused my
> shell to complain about an empty if.
> This patch applies to the current master as it requires the new GPG %G? pretty
> placeholder output.

None of the above is part of a proper commit log message, is it?

>
> Signed-off-by: Sebastian Götte <jaseg@physik-pool.tu-berlin.de>
> ---
>  templates/hooks--pre-push.sample | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
> old mode 100644
> new mode 100755

Why?

> index 15ab6d8..08a72df
> --- a/templates/hooks--pre-push.sample
> +++ b/templates/hooks--pre-push.sample
> @@ -16,20 +16,19 @@
>  #
>  #   <local ref> <local sha1> <remote ref> <remote sha1>
>  #
> -# This sample shows how to prevent push of commits where the log message starts
> -# with "WIP" (work in progress).
> +# This sample shows how to prevent pushing commits without good GPG signatures

What justifies to remove existing demonstration?  It is far easier
for the end users to remove parts that do not apply to their needs,
than coming up with a solution to add themselves without help from
an example.

>  remote="$1"
>  url="$2"
>  
>  z40=0000000000000000000000000000000000000000
> +ec=0

I think it is more customary to call this kind of variable "ret" or
"retval", not an abbreviation for "european commission" ;-).

> -IFS=' '

Why?

>  while read local_ref local_sha remote_ref remote_sha
>  do
>  	if [ "$local_sha" = $z40 ]
>  	then
> -		# Handle delete
> +		: # Handle delete
>  	else
>  		if [ "$remote_sha" = $z40 ]
>  		then
> @@ -40,14 +39,13 @@ do
>  			range="$remote_sha..$local_sha"
>  		fi
>  
> -		# Check for WIP commit
> -		commit=`git rev-list -n 1 --grep '^WIP' "$range"`
> -		if [ -n "$commit" ]
> -		then
> -			echo "Found WIP commit in $local_ref, not pushing"
> -			exit 1
> -		fi
> +		commits=`git log --format="%G? %h" "$range" | grep -v '^G' | cut -d\  -f2`

Useless use of cut.  You could do

	git log ... |
        while read sign commit
        do
        	test "$sign" = G && continue
                echo "Found commit that is not properly signed: $commit"
		...

[PATCH v2 0/1] templates: pre-push hook: check for missing GPG signatures

[Sebastian Götte]

On 04/24/2013 09:54 PM, Junio C Hamano wrote:
> None of the above is part of a proper commit log message, is it?
Fixed (I hope)

>> Signed-off-by: Sebastian Götte <jaseg@physik-pool.tu-berlin.de>
>> diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
>> old mode 100644
>> new mode 100755
> Why?
According to man githooks(5):
>It is also a requirement for a given hook to be executable. However - in a
>freshly initialized repository - the .sample files are executable by default.
This file is the only one in templates/ that was not executable, so I assume
this was a mistake.

>> index 15ab6d8..08a72df
>> --- a/templates/hooks--pre-push.sample
>> +++ b/templates/hooks--pre-push.sample
>> -# This sample shows how to prevent push of commits where the log message starts
>> -# with "WIP" (work in progress).
>> +# This sample shows how to prevent pushing commits without good GPG signatures
> What justifies to remove existing demonstration?  It is far easier
> for the end users to remove parts that do not apply to their needs,
> than coming up with a solution to add themselves without help from
> an example.
re-added it.

>> +ec=0
> I think it is more customary to call this kind of variable "ret" or
> "retval", not an abbreviation for "european commission" ;-).
renamed it to "exitcode".

>> -IFS=' '
> Why?
Otherwise in the for-loop below the output of the pipe chain is not correctly
split by newlines. Also AFAIK, this is not needed: I think the default
'<space><tab><newline>' is just fine here.
 
>> +		commits=`git log --format="%G? %h" "$range" | grep -v '^G' | cut -d\  -f2`
> Useless use of cut.  You could do
I just tried this, but I really want the script to output a list of *all*
offending commits (instead of exiting on the first problem). For this I need
the exitcode variable, but since at least bash executes the while loop in a
subshell due to the preceding pipe, I have some issues getting that out of the
subshell. This is what the code looked like without grep/cut:
># Check for missing good GPG signatures
>git log --format="%G? %h" "$range" |
>(
>                exitcode=0
>                while read sign commit
>                do
>                                test "$sign" = G && continue
>                                echo "Commit $commit does not have a good GPG signature"
>                                exitcode=1
>                done
>                exit $exitcode
>)
>let exitcode=exitcode\|$?
This is less readable and only spawns one process less.

Sebastian Götte (1):
  templates: pre-push hook: check for missing GPG signatures

 templates/hooks--pre-push.sample | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 templates/hooks--pre-push.sample

-- 
1.8.2

[PATCH v2 1/1] templates: pre-push hook: check for missing GPG signatures

[Sebastian Götte]

Also added a missing colon that caused my bash 4.2.45(2)-release to
complain about an empty if.

Signed-off-by: Sebastian Götte <jaseg@physik-pool.tu-berlin.de>
---
 templates/hooks--pre-push.sample | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
old mode 100644
new mode 100755
index 15ab6d8..a16283c
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-# An example hook script to verify what is about to be pushed.  Called by "git
+# An example hook script to verify what is about to be pushed.	Called by "git
 # push" after it has checked the remote status, but before anything has been
 # pushed.  If this script exits with a non-zero status nothing will be pushed.
 #
@@ -14,22 +14,22 @@
 # Information about the commits which are being pushed is supplied as lines to
 # the standard input in the form:
 #
-#   <local ref> <local sha1> <remote ref> <remote sha1>
+#	<local ref> <local sha1> <remote ref> <remote sha1>
 #
-# This sample shows how to prevent push of commits where the log message starts
-# with "WIP" (work in progress).
+# This sample shows how to prevent pushing commits without good GPG signatures
+# or where the log message starts with "WIP" (work in progress).
 
 remote="$1"
 url="$2"
 
 z40=0000000000000000000000000000000000000000
+exitcode=0
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
 	then
-		# Handle delete
+		: # Handle delete
 	else
 		if [ "$remote_sha" = $z40 ]
 		then
@@ -45,9 +45,16 @@ do
 		if [ -n "$commit" ]
 		then
 			echo "Found WIP commit in $local_ref, not pushing"
-			exit 1
+			exitcode=1
 		fi
+
+		# Check for missing good GPG signatures
+		for commit in `git log --format="%G? %h" "$range" | grep -v '^G' | cut -d\  -f2`
+		do
+			echo "Commit $commit does not have a good GPG signature"
+			exitcode=1
+		done
 	fi
 done
 
-exit 0
+exit $exitcode
-- 
1.8.2

Re: [PATCH v2 0/1] templates: pre-push hook: check for missing GPG signatures

[Junio C Hamano]

Sebastian Götte <jaseg@physik.tu-berlin.de> writes:

> On 04/24/2013 09:54 PM, Junio C Hamano wrote:
>> None of the above is part of a proper commit log message, is it?
> Fixed (I hope)

Don't hope, instead do.  ;-)

The questions I asked were not requests to explain them to _me_ in a
response like this.  They were the examples of what the proposed
commit log message should have explained what the patch attempts to
do.

>>> -IFS=' '
>> Why?
> Otherwise in the for-loop below the output of the pipe chain is not correctly
> split by newlines. Also AFAIK, this is not needed: I think the default
> '<space><tab><newline>' is just fine here.

It is not enough to make sure that IFS has SP so that existing code
works correctly; we also need to see if the existing code needs to
avoid cutting the tokens at HT or LF.  I think in this case using
the default IFS is safe, as input to pre-push are SP separated refs
and object names, none of which can have SP, HT or LF in it.

>># Check for missing good GPG signatures
>>git log --format="%G? %h" "$range" |
>>(
>>                exitcode=0
>>                while read sign commit
>>                do
>>                                test "$sign" = G && continue
>>                                echo "Commit $commit does not have a good GPG signature"
>>                                exitcode=1
>>                done
>>                exit $exitcode
>>)
>>let exitcode=exitcode\|$?

Don't use bash-ism "let".

The above loop is a perfectly fine and readable way to write the
logic, by the way

Except that we tend to prefer $ret over $exitcode, but I've already
said that.

Thanks.