git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v5 0/5] Verify GPG signatures when merging and extend %G? pretty string

From
SGSebastian Götte <jaseg@physik.tu-berlin.de>
Date
Mar 30, 2013, 00:13 UTC
Message-ID
<51562E3E.6060301@physik.tu-berlin.de>
In-Reply-To
<7vy5d7qhmm.fsf@alter.siamese.dyndns.org>
I hope I did not introduce more problems than I fixed in this revision ;)
On 03/28/2013 11:33 PM, Junio C Hamano wrote:
Show 7 quoted lines
> It would be much easier to read if it were "unless we are not
> looking at the very first byte, the previous byte must be LF", i.e.
> 
> 	if (found != buf && found[-1] != '\n')
> 
> Is that continue correct?  Don't you want to retry from the end of
> the line that contains the mistaken hit?
Actually it is not. Sorry.
> The "\n" at the beginning anchors the expected string for quicker
> multi-line scan done with strstr().  If you really want to lose that
> LF and still write this function correctly and clearly, I think you
> would need to iterate over the buffer line by line.
The function now does that and calls prefixcmp on each line.
On 03/28/2013 11:33 PM, Junio C Hamano wrote:> Sebastian Götte <jaseg@physik.tu-berlin.de> writes:
Show 11 quoted lines
>> +	if (verify_signatures) {
>> +		/* Verify the commit signatures */
>> +		for (p = remoteheads; p; p = p->next) {
>> +			struct commit *commit = p->item;
>> +			struct signature signature;
>> +			signature.check_result = 0;
> 
> [...] 
> By the way, I think this variable and type should be called more
> like "signature_check" or something with "check" in its name, not
> "signature".  After all it is _not_ a signature itself.

I renamed it "signature_check". I put that into the commit moving the code to commit.c. I also renamed the array/struct containing the GPG status output strings since that was originally called "signature_check".

>> +	grep "does not have a GPG signature" mergeerror
> 
> Do we plan to make this message localized?  If so I think you would
> need to do this with test_i18ngrep.
Yes, this message should be localized since it is "normal" status output. I
fixed the test case, however I noticed a possible problem with the "git log
 --show-signature" test case (t/t7510-signed-commit.sh): Here, grep is used on
git output, but this git output is actually just passed-through GPG output, and
GPG localizes that output. Are the tests alwasy run with LANG=en_US.utf-8,
LANG=C etc.?
Show 15 quoted lines
>> +test_expect_success GPG 'merge commit with bad signature with verification' '
>> +	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
>> +	grep "has a bad GPG signature" mergeerror
>> +'
>> +
>> +test_expect_success GPG 'merge signed commit with verification' '
>> +	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
>> +	grep "has a good GPG signature" mergeoutput
>> +'
> 
> Hmph.
> 
> So the caller needs to check both the standard output and the
> standard error to get the whole picture?  Is there a reason why we
> are not sending everything to the standard output consistently?
If --verify-signatures is given, everything but a good signature results in
"die(_("foo")), with _("foo") being printed on stderr. I clarified that point
in merge-options.txt. If additionally --verbose is given on the command line,
git will print _("Commit %s has a good GPG signature by %s (key fingerprint
%s)\n") on stdout for each "good" commit.  I think it is ok that way because
the caller only needs to check the exit code to get a picture. The "good GPG
signature"-message is only meant to convey the signer's name and key
fingerprint in case the caller is interested. If the caller does not want to
abort the merge in case of GPG trouble, git merge may be called without
 --verify-signatures followed by a git log --show-signature on the commits
that have been merged.
Sebastian Götte (5):
  Move commit GPG signature verification to commit.c
  commit.c/GPG signature verification: Also look at the first GPG status
    line
  merge/pull: verify GPG signatures of commits being merged
  merge/pull Check for untrusted good GPG signatures
  pretty printing: extend %G? to include 'N' and 'U'
 Documentation/merge-options.txt    |   5 ++
 Documentation/pretty-formats.txt   |   3 +-
 builtin/merge.c                    |  35 +++++++++++++-
 commit.c                           |  67 ++++++++++++++++++++++++++
 commit.h                           |  10 ++++
 git-pull.sh                        |  10 +++-
 gpg-interface.h                    |   8 ++++
 pretty.c                           |  93 ++++++-------------------------------
 t/lib-gpg/pubring.gpg              | Bin 1164 -> 2359 bytes
 t/lib-gpg/random_seed              | Bin 600 -> 600 bytes
 t/lib-gpg/secring.gpg              | Bin 1237 -> 3734 bytes
 t/lib-gpg/trustdb.gpg              | Bin 1280 -> 1360 bytes
 t/t7612-merge-verify-signatures.sh |  61 ++++++++++++++++++++++++
 13 files changed, 210 insertions(+), 82 deletions(-)
 create mode 100755 t/t7612-merge-verify-signatures.sh
-- 
1.8.1.5
Previous: Junio C HamanoNext: Sebastian Götte
Message 18 of 62 in “Move commit GPG signature verification to commit.c”
  1. 1/4 Move commit GPG signature verification to commit.cSebastian Götte, Mar 23, 2013
  2. Junio C HamanoMar 25, 2013
  3. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 25, 2013
  4. Junio C HamanoMar 26, 2013
  5. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 26, 2013
  6. Junio C HamanoMar 26, 2013
  7. Sebastian GötteMar 26, 2013
  8. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 25, 2013
  9. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 25, 2013
  10. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 25, 2013
  11. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 25, 2013
  12. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 25, 2013
  13. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 26, 2013
  14. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 26, 2013
  15. Junio C HamanoMar 28, 2013
  16. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 26, 2013
  17. Junio C HamanoMar 28, 2013
  18. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 30, 2013
  19. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 26, 2013
  20. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 26, 2013
  21. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 30, 2013
  22. Junio C HamanoMar 30, 2013
  23. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 30, 2013
  24. Junio C HamanoMar 30, 2013
  25. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 30, 2013
  26. Junio C HamanoMar 30, 2013
  27. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 30, 2013
  28. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 30, 2013
  29. Thomas RastMar 31, 2013
  30. Sebastian GötteMar 31, 2013
  31. Thomas RastMar 31, 2013
  32. Sebastian GötteMar 31, 2013
  33. Thomas RastMar 31, 2013
  34. Sebastian GötteMar 31, 2013
  35. John KeepingMar 31, 2013
  36. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 31, 2013
  37. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 30, 2013
  38. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 30, 2013
  39. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 30, 2013
  40. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 30, 2013
  41. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 30, 2013
  42. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 30, 2013
  43. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 31, 2013
  44. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 31, 2013
  45. John KeepingMar 31, 2013
  46. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 31, 2013
  47. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 31, 2013
  48. John KeepingMar 31, 2013
  49. Thomas RastMar 31, 2013
  50. Sebastian GötteMar 31, 2013
  51. Thomas RastMar 31, 2013
  52. John KeepingMar 31, 2013
  53. 0/5 Verify GPG signatures when merging and extend %G? pretty stringSebastian Götte, Mar 31, 2013
  54. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 31, 2013
  55. 1/5 Move commit GPG signature verification to commit.cSebastian Götte, Mar 31, 2013
  56. 2/5 commit.c/GPG signature verification: Also look at the first GPG status lineSebastian Götte, Mar 31, 2013
  57. 3/5 merge/pull: verify GPG signatures of commits being mergedSebastian Götte, Mar 31, 2013
  58. Junio C HamanoApr 1, 2013
  59. Sebastian GötteApr 1, 2013
  60. Junio C HamanoApr 1, 2013
  61. 4/5 merge/pull Check for untrusted good GPG signaturesSebastian Götte, Mar 31, 2013
  62. 5/5 pretty printing: extend %G? to include 'N' and 'U'Sebastian Götte, Mar 31, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.