I lost my commit signature

I lost my commit signature

[ZhenTian]

Hi git developers,

I commit with -S argument, and I got some output like this:

You need a passphrase to unlock the secret key for
user: "Tian Zhen <tianzhen@honovation.com>"
4096-bit RSA key, ID 2EF2AD6E, created 2016-05-21

[master d107770] feat: mobile support free freight hint, closed #1417
 8 files changed, 58 insertions(+), 29 deletions(-)
 rewrite static/css/mobile.min.css (64%)


but when I check git log with --show-signature, I can't find my sign.

my git is 2.4.8, and OS is Ubuntu 14.04.4

Sincerely,
dawncold

Re: I lost my commit signature

[Jeff King]

On Tue, Jun 14, 2016 at 03:50:43PM +0800, ZhenTian wrote:

> I commit with -S argument, and I got some output like this:
> 
> You need a passphrase to unlock the secret key for
> user: "Tian Zhen <tianzhen@honovation.com>"
> 4096-bit RSA key, ID 2EF2AD6E, created 2016-05-21
> 
> [master d107770] feat: mobile support free freight hint, closed #1417
>  8 files changed, 58 insertions(+), 29 deletions(-)
>  rewrite static/css/mobile.min.css (64%)
>
> but when I check git log with --show-signature, I can't find my sign.
> 
> my git is 2.4.8, and OS is Ubuntu 14.04.4

Here's a reproduction which should work (and does for me):

  $ git init
  $ echo content >file
  $ git add file
  $ git commit -m foo -S
  You need a passphrase to unlock the secret key for
  user: "Jeff King <peff@peff.net>"
  4096-bit RSA key, ID F9430ED9, created 2016-02-03 (main key ID D7B337A8)

  [master (root-commit) 6b0b230] foo
   1 file changed, 1 insertion(+)
   create mode 100644 file

  $ git log --show-signature
  commit 6b0b230c79f8912bf8b21afc0d12d2cbf54cc74d (HEAD -> master)
  gpg: Signature made Tue 14 Jun 2016 03:55:11 AM EDT using RSA key ID F9430ED9
  gpg: Good signature from "Jeff King <peff@peff.net>"
  gpg:                 aka "Jeff King <peff@github.com>"
  Author: Jeff King <peff@peff.net>
  Date:   Tue Jun 14 03:55:11 2016 -0400

      foo

Does something similar work for you? If so, then we need to figure out
what happened in your original case. Can you show the exact commands you
ran, and what they did output?

If the simple case above doesn't work, then we need to figure out
whether the commit doesn't get a signature, or whether "log
--show-signature" is not working on your system. For the former, I'd try
"git cat-file commit HEAD", which should show the encoded signature
block. If it's there, then presumably something is not working in
calling gpg.

-Peff

Re: I lost my commit signature

[ZhenTian]

Hi Peff,

I commit via this command: gcs -nm "feat: mobile support free freight
hint, closed #1417"

gcs is an alias in zsh, which is: git commit -S

I have tested sign my work in another project, it works fine, I have
committed five times, all commits are signed.

I can't find encoded signature block in the output of "git cat-file
commit HEAD", only these:
```
tree 17a572e349ce2fda47470951b5011b9c2f6533b7
parent 2c35701725d34325520acb9b45daf42f64adc536
author TianZhen <tianzhen@honovation.com> 1465887785 +0800
committer TianZhen <tianzhen@honovation.com> 1465887791 +0800

feat: mobile support free freight hint, closed #1417
```

Some of my commits are signed, for example I have committed four times
today, only first commit is signed. Is it possible some issue with
gpg-agent? I can't find it via `ps -Af | grep gpg`.

-Dawncold
Sincerely,
田震


On Tue, Jun 14, 2016 at 3:58 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 14, 2016 at 03:50:43PM +0800, ZhenTian wrote:
>
>> I commit with -S argument, and I got some output like this:
>>
>> You need a passphrase to unlock the secret key for
>> user: "Tian Zhen <tianzhen@honovation.com>"
>> 4096-bit RSA key, ID 2EF2AD6E, created 2016-05-21
>>
>> [master d107770] feat: mobile support free freight hint, closed #1417
>>  8 files changed, 58 insertions(+), 29 deletions(-)
>>  rewrite static/css/mobile.min.css (64%)
>>
>> but when I check git log with --show-signature, I can't find my sign.
>>
>> my git is 2.4.8, and OS is Ubuntu 14.04.4
>
> Here's a reproduction which should work (and does for me):
>
>   $ git init
>   $ echo content >file
>   $ git add file
>   $ git commit -m foo -S
>   You need a passphrase to unlock the secret key for
>   user: "Jeff King <peff@peff.net>"
>   4096-bit RSA key, ID F9430ED9, created 2016-02-03 (main key ID D7B337A8)
>
>   [master (root-commit) 6b0b230] foo
>    1 file changed, 1 insertion(+)
>    create mode 100644 file
>
>   $ git log --show-signature
>   commit 6b0b230c79f8912bf8b21afc0d12d2cbf54cc74d (HEAD -> master)
>   gpg: Signature made Tue 14 Jun 2016 03:55:11 AM EDT using RSA key ID F9430ED9
>   gpg: Good signature from "Jeff King <peff@peff.net>"
>   gpg:                 aka "Jeff King <peff@github.com>"
>   Author: Jeff King <peff@peff.net>
>   Date:   Tue Jun 14 03:55:11 2016 -0400
>
>       foo
>
> Does something similar work for you? If so, then we need to figure out
> what happened in your original case. Can you show the exact commands you
> ran, and what they did output?
>
> If the simple case above doesn't work, then we need to figure out
> whether the commit doesn't get a signature, or whether "log
> --show-signature" is not working on your system. For the former, I'd try
> "git cat-file commit HEAD", which should show the encoded signature
> block. If it's there, then presumably something is not working in
> calling gpg.
>
> -Peff

Re: I lost my commit signature

[Jeff King]

On Tue, Jun 14, 2016 at 04:09:52PM +0800, ZhenTian wrote:

> I have tested sign my work in another project, it works fine, I have
> committed five times, all commits are signed.
> 
> I can't find encoded signature block in the output of "git cat-file
> commit HEAD", only these:
> ```
> tree 17a572e349ce2fda47470951b5011b9c2f6533b7
> parent 2c35701725d34325520acb9b45daf42f64adc536
> author TianZhen <tianzhen@honovation.com> 1465887785 +0800
> committer TianZhen <tianzhen@honovation.com> 1465887791 +0800
> 
> feat: mobile support free freight hint, closed #1417
> ```
> 
> Some of my commits are signed, for example I have committed four times
> today, only first commit is signed. Is it possible some issue with
> gpg-agent? I can't find it via `ps -Af | grep gpg`.

Possibly. If you set gpg.program "gpg -v", does it help? You could also
try setting it to "gpg | /tmp/log" to see what gpg is passing back to
git.

-Peff

Re: I lost my commit signature

[ZhenTian]

Hi Peff,

I want to set gpg -v to pgp.program, but if I set it, it can't call gpg:
```
error: cannot run gpg -v: No such file or directory
error: could not run gpg.
fatal: failed to write commit object
```

I have tried set gpg.program value to `gpg|/tmp/log`, `/usr/bin/gpg
-v`, `gpg -v`, `"/usr/bin/gpg -v"`

only after I set to `gpg` or `/usr/bin/gpg` without any argument, it will work.
Sincerely,
田震


On Tue, Jun 14, 2016 at 4:18 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 14, 2016 at 04:09:52PM +0800, ZhenTian wrote:
>
>> I have tested sign my work in another project, it works fine, I have
>> committed five times, all commits are signed.
>>
>> I can't find encoded signature block in the output of "git cat-file
>> commit HEAD", only these:
>> ```
>> tree 17a572e349ce2fda47470951b5011b9c2f6533b7
>> parent 2c35701725d34325520acb9b45daf42f64adc536
>> author TianZhen <tianzhen@honovation.com> 1465887785 +0800
>> committer TianZhen <tianzhen@honovation.com> 1465887791 +0800
>>
>> feat: mobile support free freight hint, closed #1417
>> ```
>>
>> Some of my commits are signed, for example I have committed four times
>> today, only first commit is signed. Is it possible some issue with
>> gpg-agent? I can't find it via `ps -Af | grep gpg`.
>
> Possibly. If you set gpg.program "gpg -v", does it help? You could also
> try setting it to "gpg | /tmp/log" to see what gpg is passing back to
> git.
>
> -Peff

Re: I lost my commit signature

[Jeff King]

On Tue, Jun 14, 2016 at 04:39:38PM +0800, ZhenTian wrote:

> I want to set gpg -v to pgp.program, but if I set it, it can't call gpg:
> ```
> error: cannot run gpg -v: No such file or directory
> error: could not run gpg.
> fatal: failed to write commit object
> ```
> 
> I have tried set gpg.program value to `gpg|/tmp/log`, `/usr/bin/gpg
> -v`, `gpg -v`, `"/usr/bin/gpg -v"`
> 
> only after I set to `gpg` or `/usr/bin/gpg` without any argument, it will work.

Ah, right. Most of the time we run such programs as shell commands, but
it looks like we do not. So you'd have to do something like:

	cat >/tmp/fake-gpg <<-\EOF
	#!/bin/sh
	gpg -v "$@"
	EOF
	chmod +x /tmp/fake-gpg
	git config gpg.program /tmp/fake-gpg

-Peff

Re: I lost my commit signature

[ZhenTian]

Thanks Peff, I have setup to gpg.program, if any progress I will reply.

Over.
Sincerely,
田震


On Tue, Jun 14, 2016 at 5:41 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 14, 2016 at 04:39:38PM +0800, ZhenTian wrote:
>
>> I want to set gpg -v to pgp.program, but if I set it, it can't call gpg:
>> ```
>> error: cannot run gpg -v: No such file or directory
>> error: could not run gpg.
>> fatal: failed to write commit object
>> ```
>>
>> I have tried set gpg.program value to `gpg|/tmp/log`, `/usr/bin/gpg
>> -v`, `gpg -v`, `"/usr/bin/gpg -v"`
>>
>> only after I set to `gpg` or `/usr/bin/gpg` without any argument, it will work.
>
> Ah, right. Most of the time we run such programs as shell commands, but
> it looks like we do not. So you'd have to do something like:
>
>         cat >/tmp/fake-gpg <<-\EOF
>         #!/bin/sh
>         gpg -v "$@"
>         EOF
>         chmod +x /tmp/fake-gpg
>         git config gpg.program /tmp/fake-gpg
>
> -Peff

Re: I lost my commit signature

[Michael J Gruber]

Jeff King venit, vidit, dixit 14.06.2016 11:41:
> On Tue, Jun 14, 2016 at 04:39:38PM +0800, ZhenTian wrote:
> 
>> I want to set gpg -v to pgp.program, but if I set it, it can't call gpg:
>> ```
>> error: cannot run gpg -v: No such file or directory
>> error: could not run gpg.
>> fatal: failed to write commit object
>> ```
>>
>> I have tried set gpg.program value to `gpg|/tmp/log`, `/usr/bin/gpg
>> -v`, `gpg -v`, `"/usr/bin/gpg -v"`
>>
>> only after I set to `gpg` or `/usr/bin/gpg` without any argument, it will work.
> 
> Ah, right. Most of the time we run such programs as shell commands, but
> it looks like we do not. So you'd have to do something like:
> 
> 	cat >/tmp/fake-gpg <<-\EOF
> 	#!/bin/sh
> 	gpg -v "$@"
> 	EOF
> 	chmod +x /tmp/fake-gpg
> 	git config gpg.program /tmp/fake-gpg
> 
> -Peff
> 

The content of "gpg.program" is used as argv[0] when we build up various
commands to be run; we expect it to heed standard gpg options.

On the other hand:

git -c gpg.program=echo commit -S

'successfully' creates a commit that has

gpgsig -bsau Michael J Gruber <mickey@mouse.dis>

as the last header line. gpg.program=true fails (as does cat, unhappy
with the options), so apparently we do some error checking but not enough.

Michael

[PATCH] gpg-interface: check gpg signature for correct header

[Michael J Gruber]

When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.

Check for the correct header to catch these cases better.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
This catches at least my echo example.

We could do a full blown gpg signature check, of course.

 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..664796f 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 
 	sigchain_pop(SIGPIPE);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (finish_command(&gpg) || !len || len < 0 || strncmp(signature->buf, PGP_SIGNATURE, strlen(PGP_SIGNATURE)))
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
-- 
2.9.0.382.g87fd384

Re: [PATCH] gpg-interface: check gpg signature for correct header

[Jeff King]

On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote:

> When we create a signature, it may happen that gpg returns with
> "success" but not with an actual detached signature on stdout.
> 
> Check for the correct header to catch these cases better.

Seems like a reasonable idea.

I do worry that checking for PGP_SIGNATURE is a little fragile, though.
We currently let you sign with gpgsm, for example, and I think this
would break it (the verification side is not great because we don't
recognize gpgsm headers, but this feels like a step backwards).

That wouldn't be too hard to work around with a "is this a signature"
function that checks both types.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index c4b1e8c..664796f 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>  
>  	sigchain_pop(SIGPIPE);
>  
> -	if (finish_command(&gpg) || !len || len < 0)
> +	if (finish_command(&gpg) || !len || len < 0 || strncmp(signature->buf, PGP_SIGNATURE, strlen(PGP_SIGNATURE)))
>  		return error(_("gpg failed to sign the data"));

I think your strncmp is better spelled:

  starts_with(signature->buf, PGP_SIGNATURE);

The check for "!len" is redundant now. I think you could drop "len < 0"
as well (and in fact, drop the "len" variable entirely), as in the error
case we'd simply have an empty signature->len.

Your patch effectively swaps out "did we get any data" for "did we get
the data we expect", which is what those "len" checks were doing.

-Peff

Re: [PATCH] gpg-interface: check gpg signature for correct header

[Michael J Gruber]

Jeff King venit, vidit, dixit 14.06.2016 13:20:
> On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote:
> 
>> When we create a signature, it may happen that gpg returns with
>> "success" but not with an actual detached signature on stdout.
>>
>> Check for the correct header to catch these cases better.
> 
> Seems like a reasonable idea.
> 
> I do worry that checking for PGP_SIGNATURE is a little fragile, though.
> We currently let you sign with gpgsm, for example, and I think this
> would break it (the verification side is not great because we don't
> recognize gpgsm headers, but this feels like a step backwards).
> 
> That wouldn't be too hard to work around with a "is this a signature"
> function that checks both types.
> 
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> index c4b1e8c..664796f 100644
>> --- a/gpg-interface.c
>> +++ b/gpg-interface.c
>> @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>>  
>>  	sigchain_pop(SIGPIPE);
>>  
>> -	if (finish_command(&gpg) || !len || len < 0)
>> +	if (finish_command(&gpg) || !len || len < 0 || strncmp(signature->buf, PGP_SIGNATURE, strlen(PGP_SIGNATURE)))
>>  		return error(_("gpg failed to sign the data"));
> 
> I think your strncmp is better spelled:
> 
>   starts_with(signature->buf, PGP_SIGNATURE);
> 
> The check for "!len" is redundant now. I think you could drop "len < 0"
> as well (and in fact, drop the "len" variable entirely), as in the error
> case we'd simply have an empty signature->len.
> 
> Your patch effectively swaps out "did we get any data" for "did we get
> the data we expect", which is what those "len" checks were doing.
> 
> -Peff
> 

My patch actually makes several tests fail, sorry. (I did check before
that I can still create signatures...) Maybe my offset in buf is wrong.

starts_with, yes.

Can't check any further now, sorry. But we do check for the
PGP_SIGNATURE in our signed objects anyways. So I feel that we can either

- tighthen the check for valid gpg signatures

or

- make our signature interface completely pluggable.

We can't have it both ways, but at least things are localised in
gpg-interface.c now.

The proposed patch is just some consistency check that does not rely on
gpg.program itself(!), or else we could simply call verify.

Michael

Re: [PATCH] gpg-interface: check gpg signature for correct header

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 14.06.2016 13:34:
> Jeff King venit, vidit, dixit 14.06.2016 13:20:
>> On Tue, Jun 14, 2016 at 01:11:19PM +0200, Michael J Gruber wrote:
>>
>>> When we create a signature, it may happen that gpg returns with
>>> "success" but not with an actual detached signature on stdout.
>>>
>>> Check for the correct header to catch these cases better.
>>
>> Seems like a reasonable idea.
>>
>> I do worry that checking for PGP_SIGNATURE is a little fragile, though.
>> We currently let you sign with gpgsm, for example, and I think this
>> would break it (the verification side is not great because we don't
>> recognize gpgsm headers, but this feels like a step backwards).
>>
>> That wouldn't be too hard to work around with a "is this a signature"
>> function that checks both types.
>>
>>> diff --git a/gpg-interface.c b/gpg-interface.c
>>> index c4b1e8c..664796f 100644
>>> --- a/gpg-interface.c
>>> +++ b/gpg-interface.c
>>> @@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
>>>  
>>>  	sigchain_pop(SIGPIPE);
>>>  
>>> -	if (finish_command(&gpg) || !len || len < 0)
>>> +	if (finish_command(&gpg) || !len || len < 0 || strncmp(signature->buf, PGP_SIGNATURE, strlen(PGP_SIGNATURE)))
>>>  		return error(_("gpg failed to sign the data"));
>>
>> I think your strncmp is better spelled:
>>
>>   starts_with(signature->buf, PGP_SIGNATURE);
>>
>> The check for "!len" is redundant now. I think you could drop "len < 0"
>> as well (and in fact, drop the "len" variable entirely), as in the error
>> case we'd simply have an empty signature->len.
>>
>> Your patch effectively swaps out "did we get any data" for "did we get
>> the data we expect", which is what those "len" checks were doing.
>>
>> -Peff
>>
> 
> My patch actually makes several tests fail, sorry. (I did check before
> that I can still create signatures...) Maybe my offset in buf is wrong.
> 
> starts_with, yes.
> 
> Can't check any further now, sorry. But we do check for the
> PGP_SIGNATURE in our signed objects anyways. So I feel that we can either
> 
> - tighthen the check for valid gpg signatures
> 
> or
> 
> - make our signature interface completely pluggable.
> 
> We can't have it both ways, but at least things are localised in
> gpg-interface.c now.
> 
> The proposed patch is just some consistency check that does not rely on
> gpg.program itself(!), or else we could simply call verify.
> 
> Michael

So, with

!starts_with(signature->buf+bottom, PGP_SIGNATURE)

everything is fine except our tests for RFC1991 signatures.
Sigh...

I'll resend a patch that uses parse_signature so that all gpg specifics
are localised there.

Michael

[PATCHv2] gpg-interface: check gpg signature for correct header

[Michael J Gruber]

When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.

Check for the correct header to catch these cases better. We use the
same parse_signature function for that that we use otherwise, so that
gpg specifics are localised there.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
So, this is the real thing.

Between you and me: parse_signature in fact is more lenient, but hey - it's
exactly as lenient as we are otherwise, bar running gpg --verify.

 gpg-interface.c |  2 +-
 t/t7004-tag.sh  | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..784953c 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -185,7 +185,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 
 	sigchain_pop(SIGPIPE);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (finish_command(&gpg) || !len || len < 0 || parse_signature(signature->buf, signature->len) == signature->len)
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f9b7d79..467e968 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,10 +1202,18 @@ test_expect_success GPG,RFC1991 \
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \
-	'git tag -s fails if gpg is misconfigured' \
+	'git tag -s fails if gpg is misconfigured (bad key)' \
 	'test_must_fail git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
+# try to produce invalid signature
+git config gpg.program echo
+test_expect_success GPG \
+	'git tag -s fails if gpg is misconfigured (bad signature format)' \
+	'test_must_fail git tag -s -m tail tag-gpg-failure'
+git config --unset gpg.program
+
+
 # try to verify without gpg:
 
 rm -rf gpghome
-- 
2.9.0.382.g87fd384

[PATCHv3] gpg-interface: check gpg signature creation status

[Michael J Gruber]

When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.

Check for the correct signature creation status to catch these cases
better. Really, --status-fd parsing is the only way to check gpg status
reliably. We do the same for verify already.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
That must be the real real thing now...

 gpg-interface.c | 22 +++++++++++++++-------
 t/t7004-tag.sh  | 10 +++++++++-
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..850dc81 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -150,17 +150,19 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	const char *args[4];
-	ssize_t len;
+	const char *args[5];
 	size_t i, j, bottom;
+	struct strbuf err = STRBUF_INIT;
 
 	gpg.argv = args;
 	gpg.in = -1;
 	gpg.out = -1;
+	gpg.err = -1;
 	args[0] = gpg_program;
-	args[1] = "-bsau";
-	args[2] = signing_key;
-	args[3] = NULL;
+	args[1] = "--status-fd=2";
+	args[2] = "-bsau";
+	args[3] = signing_key;
+	args[4] = NULL;
 
 	if (start_command(&gpg))
 		return error(_("could not run gpg."));
@@ -174,19 +176,25 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
 		close(gpg.in);
 		close(gpg.out);
+		close(gpg.err);
 		finish_command(&gpg);
 		return error(_("gpg did not accept the data"));
 	}
 	close(gpg.in);
 
 	bottom = signature->len;
-	len = strbuf_read(signature, gpg.out, 1024);
+	strbuf_read(signature, gpg.out, 1024);
+	strbuf_read(&err, gpg.err, 0);
 	close(gpg.out);
+	close(gpg.err);
 
 	sigchain_pop(SIGPIPE);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (finish_command(&gpg) || !strstr(err.buf, "\n[GNUPG:] SIG_CREATED ")) {
+		strbuf_release(&err);
 		return error(_("gpg failed to sign the data"));
+	}
+	strbuf_release(&err);
 
 	/* Strip CR from the line endings, in case we are on Windows. */
 	for (i = j = bottom; i < signature->len; i++)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f9b7d79..467e968 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,10 +1202,18 @@ test_expect_success GPG,RFC1991 \
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \
-	'git tag -s fails if gpg is misconfigured' \
+	'git tag -s fails if gpg is misconfigured (bad key)' \
 	'test_must_fail git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
+# try to produce invalid signature
+git config gpg.program echo
+test_expect_success GPG \
+	'git tag -s fails if gpg is misconfigured (bad signature format)' \
+	'test_must_fail git tag -s -m tail tag-gpg-failure'
+git config --unset gpg.program
+
+
 # try to verify without gpg:
 
 rm -rf gpghome
-- 
2.9.0.382.g87fd384

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Junio C Hamano]

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

>  	bottom = signature->len;
> -	len = strbuf_read(signature, gpg.out, 1024);
> +	strbuf_read(signature, gpg.out, 1024);
> +	strbuf_read(&err, gpg.err, 0);

Hmmmm, isn't this asking for a deadlock?  When GPG spews more than
what would fit in a pipe buffer to its standard error (hence gets
blocked), its standard output may not complete, and the we would get
stuck by attempting to read from gpg.out, failing to reach the other
strbuf_read() that would unblock GPG by reading from gpg.err?

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King]

On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> >  	bottom = signature->len;
> > -	len = strbuf_read(signature, gpg.out, 1024);
> > +	strbuf_read(signature, gpg.out, 1024);
> > +	strbuf_read(&err, gpg.err, 0);
> 
> Hmmmm, isn't this asking for a deadlock?  When GPG spews more than
> what would fit in a pipe buffer to its standard error (hence gets
> blocked), its standard output may not complete, and the we would get
> stuck by attempting to read from gpg.out, failing to reach the other
> strbuf_read() that would unblock GPG by reading from gpg.err?

Yeah, it definitely is a deadlock. I think we'd need a select loop to
read into multiple strbufs at once (we can't use "struct async" because
that might happen in another process).

-Peff

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King]

On Tue, Jun 14, 2016 at 05:50:19PM -0400, Jeff King wrote:

> On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote:
> 
> > Michael J Gruber <git@drmicha.warpmail.net> writes:
> > 
> > >  	bottom = signature->len;
> > > -	len = strbuf_read(signature, gpg.out, 1024);
> > > +	strbuf_read(signature, gpg.out, 1024);
> > > +	strbuf_read(&err, gpg.err, 0);
> > 
> > Hmmmm, isn't this asking for a deadlock?  When GPG spews more than
> > what would fit in a pipe buffer to its standard error (hence gets
> > blocked), its standard output may not complete, and the we would get
> > stuck by attempting to read from gpg.out, failing to reach the other
> > strbuf_read() that would unblock GPG by reading from gpg.err?
> 
> Yeah, it definitely is a deadlock. I think we'd need a select loop to
> read into multiple strbufs at once (we can't use "struct async" because
> that might happen in another process).

Something like this on top of Michael's patch (only lightly tested).

I'm still undecided on whether it is a better approach than making sure
the stdout we got looks sane. In particular I'd worry that it would make
things harder for somebody trying to plug in something gpg-like (e.g.,
if you wanted to do something exotic like call a program which fetched
the signature from a remote device or something). But it's probably not
_that_ hard for such a script to emulate --status-fd.

---
diff --git a/gpg-interface.c b/gpg-interface.c
index 850dc81..576e462 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,6 +153,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	const char *args[5];
 	size_t i, j, bottom;
 	struct strbuf err = STRBUF_INIT;
+	struct strbuf_reader readers[2];
 
 	gpg.argv = args;
 	gpg.in = -1;
@@ -183,8 +184,15 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	close(gpg.in);
 
 	bottom = signature->len;
-	strbuf_read(signature, gpg.out, 1024);
-	strbuf_read(&err, gpg.err, 0);
+
+	readers[0].buf = signature;
+	readers[0].fd = gpg.out;
+	readers[0].hint = 1024;
+	readers[1].buf = &err;
+	readers[1].fd = gpg.err;
+	readers[1].hint = 1024;
+	strbuf_read_parallel(readers, 2);
+
 	close(gpg.out);
 	close(gpg.err);
 
diff --git a/strbuf.c b/strbuf.c
index 1ba600b..f674b23 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -395,6 +395,58 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 	return cnt;
 }
 
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr)
+{
+	int i;
+	struct pollfd *pfd;
+
+	ALLOC_ARRAY(pfd, nr);
+	for (i = 0; i < nr; i++)
+		readers[i].error = 0;
+
+	while (1) {
+		int pollsize = 0;
+		int ret;
+
+		for (i = 0; i < nr; i++) {
+			if (readers[i].fd < 0)
+				continue;
+			pfd[pollsize].fd = readers[i].fd;
+			pfd[pollsize].events = POLLIN;
+			readers[i].pfd = &pfd[pollsize];
+			pollsize++;
+		}
+
+		if (!pollsize)
+			break;
+
+		ret = poll(pfd, pollsize, -1);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			/* should never happen? */
+			die_errno("poll failed");
+		}
+
+		for (i = 0; i < nr; i++) {
+			if (readers[i].fd < 0)
+				continue;
+			if (readers[i].pfd->revents &
+			    (POLLIN|POLLHUP|POLLERR|POLLNVAL)) {
+				ret = strbuf_read_once(readers[i].buf,
+						       readers[i].fd,
+						       readers[i].hint);
+				if (ret < 0)
+					readers[i].error = errno;
+				if (ret <= 0)
+					readers[i].fd = -1;
+			}
+		}
+	}
+
+	free(pfd);
+}
+
 ssize_t strbuf_write(struct strbuf *sb, FILE *f)
 {
 	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
diff --git a/strbuf.h b/strbuf.h
index 7987405..b93822e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -374,6 +374,25 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
  */
 extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
 
+/**
+ * Read from several descriptors in parallel, each into its own strbuf.
+ * This can be used, for example, to capture stdout and stderr from a
+ * sub-process without worrying about deadlocks.
+ */
+struct strbuf_reader {
+	/* Initialized by caller */
+	struct strbuf *buf;
+	int fd;
+	size_t hint;
+
+	/* Returned by strbuf_read_parallel */
+	int error; /* 0 for success, otherwise errno */
+
+	/* Internal use */
+	struct pollfd *pfd;
+};
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr);
+
 /**
  * Read the contents of a file, specified by its path. The third argument
  * can be used to give a hint about the file size, to avoid reallocs.

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm still undecided on whether it is a better approach than making
> sure the stdout we got looks sane. In particular I'd worry that it
> would make things harder for somebody trying to plug in something
> gpg-like (e.g., if you wanted to do something exotic like call a
> program which fetched the signature from a remote device or
> something).  But it's probably not _that_ hard for such a script
> to emulate --status-fd.

I share the same thinking, but at the same time, it already is a
requirement to give --status-fd output that is close enough on the
signature verification side, isn't it?

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King]

On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm still undecided on whether it is a better approach than making
> > sure the stdout we got looks sane. In particular I'd worry that it
> > would make things harder for somebody trying to plug in something
> > gpg-like (e.g., if you wanted to do something exotic like call a
> > program which fetched the signature from a remote device or
> > something).  But it's probably not _that_ hard for such a script
> > to emulate --status-fd.
> 
> I share the same thinking, but at the same time, it already is a
> requirement to give --status-fd output that is close enough on the
> signature verification side, isn't it?

Yeah, though I could see somebody wanting to sit amidst the signing side
but not verification (e.g., if your keys are elsewhere from the machine
running git). Of course such a case could probably ferry --status-fd
from the other side anyway.

I admit I don't know of such a case in practice, though, and
implementing a rudimentary --status-fd to say "SIG OK" or whatever on
the signing side is not _that_ big a deal. So if we think this approach
is a more robust solution in the normal case, let's not hold it up over
what-ifs.

-Peff

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King]

On Tue, Jun 14, 2016 at 06:26:33PM -0400, Jeff King wrote:

> > > >  	bottom = signature->len;
> > > > -	len = strbuf_read(signature, gpg.out, 1024);
> > > > +	strbuf_read(signature, gpg.out, 1024);
> > > > +	strbuf_read(&err, gpg.err, 0);
> > > 
> > > Hmmmm, isn't this asking for a deadlock?  When GPG spews more than
> > > what would fit in a pipe buffer to its standard error (hence gets
> > > blocked), its standard output may not complete, and the we would get
> > > stuck by attempting to read from gpg.out, failing to reach the other
> > > strbuf_read() that would unblock GPG by reading from gpg.err?
> > 
> > Yeah, it definitely is a deadlock. I think we'd need a select loop to
> > read into multiple strbufs at once (we can't use "struct async" because
> > that might happen in another process).
> 
> Something like this on top of Michael's patch (only lightly tested).

I wondered if this is something we might run into in other places, but
just haven't in practice. After grepping existing calls to strbuf_read(),
I think the only other case is the one in verify_signed_buffer(), which
reads all of stderr before trying to read stdout. I suspect that _could_
deadlock but doesn't tend to in practice.

Interestingly, it also writes in full to gpg's stdin before reading
anything. If gpg buffers all of its input before writing, that's fine,
but in theory it does not have to. I have no idea if it's possible for
it to be a problem in practice.

That can be solved in the same select loop, though obviously it's not
just strbuf_read_parallel() at that point, but rather a kind of a
feed_and_capture_command().

-Peff

Re: I lost my commit signature

[ZhenTian]

Hi Michael and Peff,

I got two more lines from gpg -v during commit with -S:
```
gpg: writing to stdout
gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen <tianzhen@honovation.com>"
```

after I commit, I push it to remote, but someone had pushed before to
master branch, so I pull on master branch(`git pull --rebase`), then I
check my commit via `git log --show-signature`, there is no signature
in it, so I commit it with --ament and -S again, the signature is come
back.

I haven't check signature before push, because I have checked four
commits before, every commit is fine.

I don't know whether the `git pull` influenced signature or not.

My signature is just like Schrodinger's cat, when I check it, it lost :)
-Schrödinger


On Tue, Jun 14, 2016 at 6:57 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Jeff King venit, vidit, dixit 14.06.2016 11:41:
>> On Tue, Jun 14, 2016 at 04:39:38PM +0800, ZhenTian wrote:
>>
>>> I want to set gpg -v to pgp.program, but if I set it, it can't call gpg:
>>> ```
>>> error: cannot run gpg -v: No such file or directory
>>> error: could not run gpg.
>>> fatal: failed to write commit object
>>> ```
>>>
>>> I have tried set gpg.program value to `gpg|/tmp/log`, `/usr/bin/gpg
>>> -v`, `gpg -v`, `"/usr/bin/gpg -v"`
>>>
>>> only after I set to `gpg` or `/usr/bin/gpg` without any argument, it will work.
>>
>> Ah, right. Most of the time we run such programs as shell commands, but
>> it looks like we do not. So you'd have to do something like:
>>
>>       cat >/tmp/fake-gpg <<-\EOF
>>       #!/bin/sh
>>       gpg -v "$@"
>>       EOF
>>       chmod +x /tmp/fake-gpg
>>       git config gpg.program /tmp/fake-gpg
>>
>> -Peff
>>
>
> The content of "gpg.program" is used as argv[0] when we build up various
> commands to be run; we expect it to heed standard gpg options.
>
> On the other hand:
>
> git -c gpg.program=echo commit -S
>
> 'successfully' creates a commit that has
>
> gpgsig -bsau Michael J Gruber <mickey@mouse.dis>
>
> as the last header line. gpg.program=true fails (as does cat, unhappy
> with the options), so apparently we do some error checking but not enough.
>
> Michael

Re: I lost my commit signature

[Jeff King]

On Wed, Jun 15, 2016 at 12:27:15PM +0800, ZhenTian wrote:

> I got two more lines from gpg -v during commit with -S:
> ```
> gpg: writing to stdout
> gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen <tianzhen@honovation.com>"
> ```
> 
> after I commit, I push it to remote, but someone had pushed before to
> master branch, so I pull on master branch(`git pull --rebase`), then I
> check my commit via `git log --show-signature`, there is no signature
> in it, so I commit it with --ament and -S again, the signature is come
> back.
> 
> I haven't check signature before push, because I have checked four
> commits before, every commit is fine.
> 
> I don't know whether the `git pull` influenced signature or not.

Ah, so the problem is probably that you had a signature _initially_, but
that it did not survive the rebase. Which makes sense, as rebase would
need to re-sign.  It does not by default, but you can tell it to do so
with "-S". Or you can set `commit.gpgsign`, which should sign in both
cases.

-Peff

Re: I lost my commit signature

[Michael J Gruber]

Jeff King venit, vidit, dixit 15.06.2016 06:34:
> On Wed, Jun 15, 2016 at 12:27:15PM +0800, ZhenTian wrote:
> 
>> I got two more lines from gpg -v during commit with -S:
>> ```
>> gpg: writing to stdout
>> gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen <tianzhen@honovation.com>"
>> ```
>>
>> after I commit, I push it to remote, but someone had pushed before to
>> master branch, so I pull on master branch(`git pull --rebase`), then I
>> check my commit via `git log --show-signature`, there is no signature
>> in it, so I commit it with --ament and -S again, the signature is come
>> back.
>>
>> I haven't check signature before push, because I have checked four
>> commits before, every commit is fine.
>>
>> I don't know whether the `git pull` influenced signature or not.
> 
> Ah, so the problem is probably that you had a signature _initially_, but
> that it did not survive the rebase. Which makes sense, as rebase would
> need to re-sign.  It does not by default, but you can tell it to do so
> with "-S". Or you can set `commit.gpgsign`, which should sign in both
> cases.

While it's clear that a rebase invalidates the signature, we could try
to be more helpful here, especially given the fact that (with our model)
you can't sign a commit afterwards any more.

commit.gpgsign signs everything, but there should be a mode for
re-signing signed commits, or at least a warning that rebase dropped a
signature so that you can --amend -S the last commit.

Michael

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Michael J Gruber]

Jeff King venit, vidit, dixit 15.06.2016 02:56:
> On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>> I'm still undecided on whether it is a better approach than making
>>> sure the stdout we got looks sane. In particular I'd worry that it
>>> would make things harder for somebody trying to plug in something
>>> gpg-like (e.g., if you wanted to do something exotic like call a
>>> program which fetched the signature from a remote device or
>>> something).  But it's probably not _that_ hard for such a script
>>> to emulate --status-fd.
>>
>> I share the same thinking, but at the same time, it already is a
>> requirement to give --status-fd output that is close enough on the
>> signature verification side, isn't it?
> 
> Yeah, though I could see somebody wanting to sit amidst the signing side
> but not verification (e.g., if your keys are elsewhere from the machine
> running git). Of course such a case could probably ferry --status-fd
> from the other side anyway.
> 
> I admit I don't know of such a case in practice, though, and
> implementing a rudimentary --status-fd to say "SIG OK" or whatever on
> the signing side is not _that_ big a deal. So if we think this approach
> is a more robust solution in the normal case, let's not hold it up over
> what-ifs.
> 
> -Peff

As for the flexibility:
We do code specifically for gpg, which happens to work for gpg2 also.
The patch doesn't add any gpg ui requirements that we don't require
elsewhere already.
More flexibility requires a completely pluggable approach - gpgsm
already fails to meet the gpg command line ui.

In any case, "status-fd" is *the* way to interface with gpg reliably
just like plumbing commands are *the* way to interface with git reliably.

As for the read locking:
I'm sorry I have no idea about that area at all. I thought that I'm
doing the same that we do for verify, but apparently not. My
strbuf_read-fu is not that strong either (read: copy&paste). I trust
your assessment completely, though.

As for the original problem:
That had a different cause, as we know now (rebase dropping signatures
without hint). I still think we should check gpg status codes properly.

Michael

Re: I lost my commit signature

[ZhenTian]

Thank you all very much!
-Schrödinger


On Wed, Jun 15, 2016 at 3:07 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Jeff King venit, vidit, dixit 15.06.2016 06:34:
>> On Wed, Jun 15, 2016 at 12:27:15PM +0800, ZhenTian wrote:
>>
>>> I got two more lines from gpg -v during commit with -S:
>>> ```
>>> gpg: writing to stdout
>>> gpg: RSA/SHA1 signature from: "2EF2AD6E Tian Zhen <tianzhen@honovation.com>"
>>> ```
>>>
>>> after I commit, I push it to remote, but someone had pushed before to
>>> master branch, so I pull on master branch(`git pull --rebase`), then I
>>> check my commit via `git log --show-signature`, there is no signature
>>> in it, so I commit it with --ament and -S again, the signature is come
>>> back.
>>>
>>> I haven't check signature before push, because I have checked four
>>> commits before, every commit is fine.
>>>
>>> I don't know whether the `git pull` influenced signature or not.
>>
>> Ah, so the problem is probably that you had a signature _initially_, but
>> that it did not survive the rebase. Which makes sense, as rebase would
>> need to re-sign.  It does not by default, but you can tell it to do so
>> with "-S". Or you can set `commit.gpgsign`, which should sign in both
>> cases.
>
> While it's clear that a rebase invalidates the signature, we could try
> to be more helpful here, especially given the fact that (with our model)
> you can't sign a commit afterwards any more.
>
> commit.gpgsign signs everything, but there should be a mode for
> re-signing signed commits, or at least a warning that rebase dropped a
> signature so that you can --amend -S the last commit.
>
> Michael

Re: I lost my commit signature

[Jeff King]

On Wed, Jun 15, 2016 at 09:07:16AM +0200, Michael J Gruber wrote:

> > Ah, so the problem is probably that you had a signature _initially_, but
> > that it did not survive the rebase. Which makes sense, as rebase would
> > need to re-sign.  It does not by default, but you can tell it to do so
> > with "-S". Or you can set `commit.gpgsign`, which should sign in both
> > cases.
> 
> While it's clear that a rebase invalidates the signature, we could try
> to be more helpful here, especially given the fact that (with our model)
> you can't sign a commit afterwards any more.
> 
> commit.gpgsign signs everything, but there should be a mode for
> re-signing signed commits, or at least a warning that rebase dropped a
> signature so that you can --amend -S the last commit.

I had a similar thought, though I'm not sure how useful a "re-sign
signed commits" mode would be in practice. Mostly because I'm not sure
why signing would be more important for one commit versus another.

That is, I can see why somebody would set "commit.gpgSign"; their
preference (or that of their project) is to sign commits, and they've
set up gpg, etc, to make it relatively painless.

But why does somebody run "commit -S" for a single commit, but not all
the time? Is it because that commit is special? Or is that particular
moment special? One implies that it's important for the signature to be
retained during a rebase, and one does not.

So I dunno. I would not be opposed to such a feature, but I'm having
trouble figuring out why it would be useful (though for the most part, I
do not see why anything but per-project commit.gpgSign config is
particularly useful. Maybe I just lack imagination).

-Peff

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King]

On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote:

> As for the flexibility:
> We do code specifically for gpg, which happens to work for gpg2 also.
> The patch doesn't add any gpg ui requirements that we don't require
> elsewhere already.
> More flexibility requires a completely pluggable approach - gpgsm
> already fails to meet the gpg command line ui.

Does it? I haven't run it in production, but I did some tests with gpgsm
a few months ago and found it to be a drop-in replacement, at least with
respect to git. Though I don't think that matters one way or the other
for the current discussion; it _does_ do --status-fd.

> In any case, "status-fd" is *the* way to interface with gpg reliably
> just like plumbing commands are *the* way to interface with git reliably.

Fair enough. I've generally found its exit code pretty reliable. It's
unclear to me if the problem you saw was because gpg was exiting 0 but
producing no signature, or if your debugging was masking its exit code.

Either way, I do not mind using --status-fd; it seems like it should be
more robust in general. It's the tip commit in the series I'm about to
post.

> As for the read locking:
> I'm sorry I have no idea about that area at all. I thought that I'm
> doing the same that we do for verify, but apparently not. My
> strbuf_read-fu is not that strong either (read: copy&paste). I trust
> your assessment completely, though.

Yeah, you're right that the deadlock thing is also a possibility on the
verification side. I'm not sure whether it's possible to trigger in
practice or not. See the analysis in the series.

> As for the original problem:
> That had a different cause, as we know now (rebase dropping signatures
> without hint). I still think we should check gpg status codes properly.

Yep, I agree.

-Peff

Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Michael J Gruber]

Jeff King venit, vidit, dixit 16.06.2016 11:25:
> On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote:
> 
>> As for the flexibility:
>> We do code specifically for gpg, which happens to work for gpg2 also.
>> The patch doesn't add any gpg ui requirements that we don't require
>> elsewhere already.
>> More flexibility requires a completely pluggable approach - gpgsm
>> already fails to meet the gpg command line ui.
> 
> Does it? I haven't run it in production, but I did some tests with gpgsm
> a few months ago and found it to be a drop-in replacement, at least with
> respect to git. Though I don't think that matters one way or the other
> for the current discussion; it _does_ do --status-fd.

You are right!

I solely trusted in the documentation rather than simply trying it out.
gpgsm even supports the undocumented short options.

Only the resulting header is different (plus the fact that I have to
turn of crl checks for whatever reason).

>> In any case, "status-fd" is *the* way to interface with gpg reliably
>> just like plumbing commands are *the* way to interface with git reliably.
> 
> Fair enough. I've generally found its exit code pretty reliable. It's
> unclear to me if the problem you saw was because gpg was exiting 0 but
> producing no signature, or if your debugging was masking its exit code.
> 
> Either way, I do not mind using --status-fd; it seems like it should be
> more robust in general. It's the tip commit in the series I'm about to
> post.
> 
>> As for the read locking:
>> I'm sorry I have no idea about that area at all. I thought that I'm
>> doing the same that we do for verify, but apparently not. My
>> strbuf_read-fu is not that strong either (read: copy&paste). I trust
>> your assessment completely, though.
> 
> Yeah, you're right that the deadlock thing is also a possibility on the
> verification side. I'm not sure whether it's possible to trigger in
> practice or not. See the analysis in the series.

With great admiration I've looked at your series which solves this
problem at the root. Way above my head (the solution, not the root,
fortunately).

>> As for the original problem:
>> That had a different cause, as we know now (rebase dropping signatures
>> without hint). I still think we should check gpg status codes properly.
> 
> Yep, I agree.
> 
> -Peff
> 

Re: I lost my commit signature

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But why does somebody run "commit -S" for a single commit, but not all
> the time? Is it because that commit is special? Or is that particular
> moment special? One implies that it's important for the signature to be
> retained during a rebase, and one does not.
>
> So I dunno. I would not be opposed to such a feature, but I'm having
> trouble figuring out why it would be useful (though for the most part, I
> do not see why anything but per-project commit.gpgSign config is
> particularly useful. Maybe I just lack imagination).

I am not so imaginative, either. One remotely plausible use case may
be a project that has two classes of paths (let's call these classes
sensitive and others), and requires its participants to sign commits
that touch sensitive paths.  The user needs something finter grained
than per-project commit.gpgSign there.

But even in such a case, the fact that an original commit is with a
signature should not be a good indication that the rewritten version
of that commit in the new history still touches the sensitive paths
that required the original to be signed (i.e. the history the user
is rebasing onto may already have the necessary changes to these
paths).

So, I dunno, either.

Re: I lost my commit signature

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 16.06.2016 19:06:
> Jeff King <peff@peff.net> writes:
> 
>> But why does somebody run "commit -S" for a single commit, but not all
>> the time? Is it because that commit is special? Or is that particular
>> moment special? One implies that it's important for the signature to be
>> retained during a rebase, and one does not.
>>
>> So I dunno. I would not be opposed to such a feature, but I'm having
>> trouble figuring out why it would be useful (though for the most part, I
>> do not see why anything but per-project commit.gpgSign config is
>> particularly useful. Maybe I just lack imagination).
> 
> I am not so imaginative, either. One remotely plausible use case may
> be a project that has two classes of paths (let's call these classes
> sensitive and others), and requires its participants to sign commits
> that touch sensitive paths.  The user needs something finter grained
> than per-project commit.gpgSign there.
> 
> But even in such a case, the fact that an original commit is with a
> signature should not be a good indication that the rewritten version
> of that commit in the new history still touches the sensitive paths
> that required the original to be signed (i.e. the history the user
> is rebasing onto may already have the necessary changes to these
> paths).
> 
> So, I dunno, either.
> 

While I follow both of your lines of argumentation, I tend to claim that
they imply: there is no reason to blindly sign any commit... We should
dump that config :)

Since it's not possible to sign commits after the fact without rebasing
(they are not "notes" attached to a commit but part of the commit) it is
very conceivable to me that you build up your work with fine-grained
commits and then, at some point where everything is ready and carefully
inspected, you sign it. There are various possible reasons why you may
not be able to rebase at that point. (I don't know why one wouldn't want
to use signed tags here, but I never understood the need for signed
commits in the first place.)

I guess users of signed commits with rebase should speak up so that we
can serve them well.

Michael

Re: I lost my commit signature

[Junio C Hamano]

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

> Junio C Hamano venit, vidit, dixit 16.06.2016 19:06:
>
>> I am not so imaginative, either. One remotely plausible use case may
>> be a project that has two classes of paths (let's call these classes
>> sensitive and others), and requires its participants to sign commits
>> that touch sensitive paths.  The user needs something finter grained
>> than per-project commit.gpgSign there.
>>  ...
>> So, I dunno, either.
>
> While I follow both of your lines of argumentation, I tend to claim that
> they imply: there is no reason to blindly sign any commit... We should
> dump that config :)

... no reason to blindly sign any commit IN SUCH A PROJECT that
wants you to selectively sign commits.

That does not lead to "we should dump that config" that all, does
it?