From aba91192ae39cd1a2f79e7ed91e966df3cfe10b7 Mon Sep 17 00:00:00 2001 From: Carlos Rica Date: Sun, 9 Sep 2007 02:39:29 +0200 Subject: git-tag -s must fail if gpg cannot sign the tag. Most of this patch code and message was written by Shawn O. Pearce. I made some tests to know what the problem was, and then I changed the code related with the SIGPIPE signal. If the user has misconfigured `user.signingkey` in their .git/config or just doesn't have any secret keys on their keyring and they ask for a signed tag with `git tag -s` we better make sure the resulting tag was actually signed by gpg. Prior versions of builtin git-tag allowed this failure to slip by without error as they were not checking the return value of the finish_command() so they did not notice when gpg exited with an error exit status. They also did not fail if gpg produced an empty output or if read_in_full received an error from the read system call while trying to read the pipe back from gpg. Finally, we did not actually honor any return value from the do_sign function as it returns ssize_t but was being stored into an unsigned long. This caused the compiler to optimize out the die condition, allowing git-tag to continue along and create the tag object. However, when gpg gets a wrong username, it exits before any read was done and then the writing process receives SIGPIPE and program is terminated. By ignoring this signal, anyway, the function write_or_die gets EPIPE from write_in_full and exits returning 0 to the system without a message. Here we better call to write_in_full directly so we can fail printing a message and return safely to the caller. With these issues fixed `git-tag -s` will now fail to create the tag and will report a non-zero exit status to its caller, thereby allowing automated helper scripts to detect (and recover from) failure if gpg is not working properly. Proposed-by: Shawn O. Pearce Signed-off-by: Carlos Rica Signed-off-by: Junio C Hamano --- builtin-tag.c | 18 ++++++++++++++---- t/t7004-tag.sh | 7 +++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/builtin-tag.c b/builtin-tag.c index 348919cff8..3a9d2eea71 100644 --- a/builtin-tag.c +++ b/builtin-tag.c @@ -200,6 +200,10 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max) bracket[1] = '\0'; } + /* When the username signingkey is bad, program could be terminated + * because gpg exits without reading and then write gets SIGPIPE. */ + signal(SIGPIPE, SIG_IGN); + memset(&gpg, 0, sizeof(gpg)); gpg.argv = args; gpg.in = -1; @@ -212,12 +216,17 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max) if (start_command(&gpg)) return error("could not run gpg."); - write_or_die(gpg.in, buffer, size); + if (write_in_full(gpg.in, buffer, size) != size) { + close(gpg.in); + finish_command(&gpg); + return error("gpg did not accept the tag data"); + } close(gpg.in); gpg.close_in = 0; len = read_in_full(gpg.out, buffer + size, max - size); - finish_command(&gpg); + if (finish_command(&gpg) || !len || len < 0) + return error("gpg failed to sign the tag"); if (len == max - size) return error("could not read the entire signature from gpg."); @@ -310,9 +319,10 @@ static void create_tag(const unsigned char *object, const char *tag, size += header_len; if (sign) { - size = do_sign(buffer, size, max_size); - if (size < 0) + ssize_t r = do_sign(buffer, size, max_size); + if (r < 0) die("unable to sign the tag"); + size = r; } if (write_sha1_file(buffer, size, tag_type, result) < 0) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 606d4f2a2c..0d07bc39c7 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -990,6 +990,13 @@ test_expect_success \ git diff expect actual ' +# try to sign with bad user.signingkey +git config user.signingkey BobTheMouse +test_expect_failure \ + 'git-tag -s fails if gpg is misconfigured' \ + 'git tag -s -m tail tag-gpg-failure' +git config --unset user.signingkey + # try to verify without gpg: rm -rf gpghome -- cgit v1.2.1 From 05cc2ffc572f05e8aeec495a9ab9bc9609863491 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Mon, 10 Sep 2007 00:15:29 -0400 Subject: fix doc for --compression argument to pack-objects Remove obsolete details (core.legacyheaders is always true now). Signed-off-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- Documentation/git-pack-objects.txt | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt index 6f17cff24a..f8a0be3511 100644 --- a/Documentation/git-pack-objects.txt +++ b/Documentation/git-pack-objects.txt @@ -155,12 +155,8 @@ base-name:: generated pack. If not specified, pack compression level is determined first by pack.compression, then by core.compression, and defaults to -1, the zlib default, if neither is set. - Data copied from loose objects will be recompressed - if core.legacyheaders was true when they were created or if - the loose compression level (see core.loosecompression and - core.compression) is now a different value than the pack - compression level. Add --no-reuse-object if you want to force - a uniform compression level on all data no matter the source. + Add \--no-reuse-object if you want to force a uniform compression + level on all data no matter the source. --delta-base-offset:: A packed archive can express base object of a delta as -- cgit v1.2.1 From a4503a15af661ee865ed10102df15a6d3b43e60a Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sun, 9 Sep 2007 19:38:11 -0400 Subject: Make --no-thin the default in git-push to save server resources 1) pushes happen less often than fetches, so the bandwidth saving is much less visible in that case overall. 2) thin packs have to be complemented with missing delta bases to be valid, so many received thin packs will take more disk space. 3) the bother of repacking should be distributed amongst "clients" i.e. fetchers and pushers as much as possible, and not the server being fetched or pushed, to keep disk and CPU usage low on the server. This is why a fetch should get thin packs but a push should not. Both Nico and I have been assuming that --no-thin was the default behavior of git-push ever since Nico introduced --fix-thin into the index-pack process, which allowed fetch and receive-pack to avoid exploding packfiles received during transfer. This patch finally makes it so. Signed-off-by: Shawn O. Pearce Signed-off-by: Junio C Hamano --- builtin-push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin-push.c b/builtin-push.c index 2612f07f74..88c5024da7 100644 --- a/builtin-push.c +++ b/builtin-push.c @@ -9,7 +9,7 @@ static const char push_usage[] = "git-push [--all] [--tags] [--receive-pack=] [--repo=all] [-f | --force] [-v] [ ...]"; -static int all, force, thin = 1, verbose; +static int all, force, thin, verbose; static const char *receivepack; static const char **refspec; -- cgit v1.2.1