Re: [PATCH 9/9] clone: run check_everything_connected
On Thu, Mar 28, 2013 at 07:40:51AM +0700, Duy Nguyen wrote: Maybe we could do it in index-pack to save some (wall) time. I haven't tried but I think it might work. The problem is to make sure the pack contains objects for all sha1 references in the pack. By that description, we don't need to do standard DAG traversal. We could extract sha-1 references in index-pack as we uncompress objects and put all want sha-1 in a hash table. At the end of index-pack, we check if any sha-1 in the hash table still points to non-existing object. This way, at least we don't need to uncompress all objects again in rev-list. We could parse+hash in both phases in index-pack. The first phase (parse_pack_objects) is usually I/O bound, we could hide some cost there. The second phase is multithreaded, all the better. It looks like what I describe above is exactly what index-pack --strict does. Except that it holds the lock longer and has more abstraction layers to slow things down. On linux-2.6 with 3 threads: $ rev-list --all --objects --quiet (aka check_everything_connected) 34.26user 0.22system 0:34.56elapsed 99%CPU (0avgtext+0avgdata 2550528maxresident)k 0inputs+0outputs (0major+208569minor)pagefaults 0swaps $ index-pack --stdin 214.57user 8.38system 1:31.82elapsed 242%CPU (0avgtext+0avgdata 1357328maxresident)k 8inputs+1421016outputs (0major+1222537minor)pagefaults 0swaps $ index-pack --stdin --strict 297.36user 13.77system 2:11.82elapsed 236%CPU (0avgtext+0avgdata 1875040maxresident)k 0inputs+1421016outputs (0major+1308718minor)pagefaults 0swaps $ index-pack --stdin --connectivity 231.09user 7.42system 1:37.39elapsed 244%CPU (0avgtext+0avgdata 2080816maxresident)k 0inputs+1421016outputs (0major+540069minor)pagefaults 0swaps The last one does not hold locks by duplicating object hash table per thread. As you can see the consumed memory is much higher than --stdin. In return it only adds up 1/3 of rev-list time. Maybe you should check which one is cheaper for clone case, check_everything_connected() or index-pack --strict. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] tests: --valgrind=tool
From: Thomas Rast tr...@inf.ethz.ch I had a quick-and-dirty edit to t/valgrind/valgrind.sh in my trees while we did the index-pack investigation. This small series is the proper way of achieving the same. Thomas Rast (3): t/README: --valgrind already implies -v tests: parameterize --valgrind option tests --valgrind: provide a mode without --track-origins t/README | 21 +++-- t/test-lib.sh | 10 +- t/valgrind/valgrind.sh | 27 +-- 3 files changed, 41 insertions(+), 17 deletions(-) -- 1.8.2.467.gedf93a5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] t/README: --valgrind already implies -v
From: Thomas Rast tr...@inf.ethz.ch This was missed in 3da9365 (Tests: let --valgrind imply --verbose and --tee, 2009-02-04). Signed-off-by: Thomas Rast tr...@inf.ethz.ch --- t/README | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/README b/t/README index e4128e5..bc7253c5 100644 --- a/t/README +++ b/t/README @@ -95,8 +95,7 @@ appropriately before running make. --valgrind:: Execute all Git binaries with valgrind and exit with status 126 on errors (just like regular tests, this will only stop - the test script when running under -i). Valgrind errors - go to stderr, so you might want to pass the -v option, too. + the test script when running under -i). Since it makes no sense to run the tests with --valgrind and not see any output, this option implies --verbose. For -- 1.8.2.467.gedf93a5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] tests: parameterize --valgrind option
From: Thomas Rast tr...@inf.ethz.ch
Running tests under helgrind and DRD recently proved useful in
tracking down thread interaction issues. This can unfortunately not
be done through GIT_VALGRIND_OPTIONS because any tool other than
memcheck would complain about unknown options.
Let --valgrind take an optional parameter that describes the valgrind
tool to invoke. The default mode is to run memcheck as before.
Signed-off-by: Thomas Rast tr...@inf.ethz.ch
---
t/README | 15 ++-
t/test-lib.sh | 10 +-
t/valgrind/valgrind.sh | 25 +++--
3 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/t/README b/t/README
index bc7253c5..f5ee40f 100644
--- a/t/README
+++ b/t/README
@@ -92,16 +92,21 @@ appropriately before running make.
This causes additional long-running tests to be run (where
available), for more exhaustive testing.
---valgrind::
- Execute all Git binaries with valgrind and exit with status
- 126 on errors (just like regular tests, this will only stop
- the test script when running under -i).
+--valgrind=tool::
+ Execute all Git binaries under valgrind tool tool and exit
+ with status 126 on errors (just like regular tests, this will
+ only stop the test script when running under -i).
Since it makes no sense to run the tests with --valgrind and
not see any output, this option implies --verbose. For
convenience, it also implies --tee.
- Note that valgrind is run with the option --leak-check=no,
+ tool defaults to 'memcheck', just like valgrind itself.
+ Other particularly useful choices include 'helgrind' and
+ 'drd', but you may use any tool recognized by your valgrind
+ installation.
+
+ Note that memcheck is run with the option --leak-check=no,
as the git process is short-lived and some errors are not
interesting. In order to run a single command under the same
conditions manually, you should set GIT_VALGRIND to point to
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1f51025..da57a2f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -193,7 +193,11 @@ do
--no-color)
color=; shift ;;
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=t; verbose=t; shift ;;
+ valgrind=memcheck
+ shift ;;
+ --valgrind=*)
+ valgrind=$(expr z$1 : 'z[^=]*=\(.*\)')
+ shift ;;
--tee)
shift ;; # was handled already
--root=*)
@@ -204,6 +208,8 @@ do
esac
done
+test -n $valgrind verbose=t
+
if test -n $color
then
say_color () {
@@ -530,6 +536,8 @@ then
PATH=$GIT_VALGRIND/bin:$PATH
GIT_EXEC_PATH=$GIT_VALGRIND/bin
export GIT_VALGRIND
+ GIT_VALGRIND_MODE=$valgrind
+ export GIT_VALGRIND_MODE
elif test -n $GIT_TEST_INSTALLED
then
GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 582b4dc..472ac2d 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -2,20 +2,25 @@
base=$(basename $0)
-TRACK_ORIGINS=
+TOOL_OPTIONS='--leak-check=no'
-VALGRIND_VERSION=$(valgrind --version)
-VALGRIND_MAJOR=$(expr $VALGRIND_VERSION : '[^0-9]*\([0-9]*\)')
-VALGRIND_MINOR=$(expr $VALGRIND_VERSION : '[^0-9]*[0-9]*\.\([0-9]*\)')
-test 3 -gt $VALGRIND_MAJOR ||
-test 3 -eq $VALGRIND_MAJOR -a 4 -gt $VALGRIND_MINOR ||
-TRACK_ORIGINS=--track-origins=yes
+case $GIT_VALGRIND_MODE in
+memcheck)
+ VALGRIND_VERSION=$(valgrind --version)
+ VALGRIND_MAJOR=$(expr $VALGRIND_VERSION : '[^0-9]*\([0-9]*\)')
+ VALGRIND_MINOR=$(expr $VALGRIND_VERSION : '[^0-9]*[0-9]*\.\([0-9]*\)')
+ test 3 -gt $VALGRIND_MAJOR ||
+ test 3 -eq $VALGRIND_MAJOR -a 4 -gt $VALGRIND_MINOR ||
+ TOOL_OPTIONS=$TOOL_OPTIONS --track-origins=yes
+ ;;
+*)
+ TOOL_OPTIONS=--tool=$GIT_VALGRIND_MODE
+esac
exec valgrind -q --error-exitcode=126 \
- --leak-check=no \
- --suppressions=$GIT_VALGRIND/default.supp \
--gen-suppressions=all \
- $TRACK_ORIGINS \
+ --suppressions=$GIT_VALGRIND/default.supp \
+ $TOOL_OPTIONS \
--log-fd=4 \
--input-fd=4 \
$GIT_VALGRIND_OPTIONS \
--
1.8.2.467.gedf93a5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] tests --valgrind: provide a mode without --track-origins
From: Thomas Rast tr...@inf.ethz.ch With --valgrind=memcheck-fast, the tests run under memcheck but without the autodetected --track-origins. If you just run valgrind to see *if* there is any memory issue with your program, the extra information is not needed, and it comes at a roughly 30% hit in runtime. While it is possible to achieve the same through GIT_VALGRIND_OPTIONS, this should be more discoverable and hopefully encourage more users to run their tests with valgrind. Signed-off-by: Thomas Rast tr...@inf.ethz.ch --- t/README | 5 + t/valgrind/valgrind.sh | 2 ++ 2 files changed, 7 insertions(+) diff --git a/t/README b/t/README index f5ee40f..9b41fe7 100644 --- a/t/README +++ b/t/README @@ -106,6 +106,11 @@ appropriately before running make. 'drd', but you may use any tool recognized by your valgrind installation. + As a special case, tool can be 'memcheck-fast', which uses + memcheck but disables --track-origins. Use this if you are + running tests in bulk, to see if there are _any_ memory + issues. + Note that memcheck is run with the option --leak-check=no, as the git process is short-lived and some errors are not interesting. In order to run a single command under the same diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh index 472ac2d..6b87c91 100755 --- a/t/valgrind/valgrind.sh +++ b/t/valgrind/valgrind.sh @@ -5,6 +5,8 @@ base=$(basename $0) TOOL_OPTIONS='--leak-check=no' case $GIT_VALGRIND_MODE in +memcheck-fast) + ;; memcheck) VALGRIND_VERSION=$(valgrind --version) VALGRIND_MAJOR=$(expr $VALGRIND_VERSION : '[^0-9]*\([0-9]*\)') -- 1.8.2.467.gedf93a5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de writes:
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
[...]
+test_expect_success GPG 'merge commit with untrusted signature with
verification' '
^
`.
Nit: you have a pointless(?) double space here-´
+ test_must_fail git merge --ff-only --verify-signatures side-untrusted
2mergeerror
+ test_i18ngrep from an untrusted key mergeerror
+'
This test gives me the following:
==26527== Conditional jump or move depends on uninitialised value(s)
==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084)
==26527==by 0x4989CC: parse_signature_lines (commit.c:1074)
==26527==by 0x498B33: check_commit_signature (commit.c:1100)
==26527==by 0x453719: cmd_merge (merge.c:1246)
==26527==by 0x4057B6: run_builtin (git.c:282)
==26527==by 0x405949: handle_internal_command (git.c:444)
==26527==by 0x405A63: run_argv (git.c:490)
==26527==by 0x405BF2: main (git.c:565)
though I currently cannot see what's wrong, probably because I don't
know the format that parse_signature_lines gives. Can you look into it?
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] tests: notice valgrind error in test_must_fail
We tell valgrind to return 126 if it notices that something is wrong,
but we did not actually handle this in test_must_fail, leading to
false negatives. Catch and report it.
Signed-off-by: Thomas Rast tr...@inf.ethz.ch
---
Just noticed this issue when tracking down the failure in t7612. It
might still be a bit too fragile; when running the entire suite under
valgrind, I usually just
cd test-results
egrep '^==[0-9]+==' *.out| less -S
t/test-lib-functions.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fa62d01..6766553 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -536,6 +536,9 @@ test_must_fail () {
elif test $exit_code = 127; then
echo 2 test_must_fail: command not found: $*
return 1
+ elif test $exit_code = 126; then
+ echo 2 test_must_fail: valgrind error: $*
+ return 1
fi
return 0
}
--
1.8.2.467.gedf93a5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: More detailed error message for 403 forbidden.
Maybe. But I would worry somewhat about sites which provide a useless and verbose text/plain message. Ideally an x-git-error-message would be no more than few lines, suitable for the error message of a terminal program. I would not want a site-branded Your page cannot be found. Here's a complete navigation bar page to be spewed to the terminal. Those tend to be text/html, though, so we may be safe. It's just that we're gambling on what random servers do, and if we show useless spew even some of the time, that would be a regression. -Peff I completely agree with you. And should git client need to add x-git-error-message in Accept header and/or perhaps language preference in Accept-Language header? Accept: x-git-error-message, */*;q=0.8 Accept-Language: ko,en;q=0.8 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 10:32 AM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de writes: When --verify-signatures is specified, abort the merge in case a good GPG signature from an untrusted key is encountered. [...] +test_expect_success GPG 'merge commit with untrusted signature with verification' ' ^ `. Nit: you have a pointless(?) double space here-´ Will fix that in the next revision ;) +test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror +test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) though I currently cannot see what's wrong, probably because I don't know the format that parse_signature_lines gives. Can you look into it? Against what version/combination of the patches are you running the test? parse_signature_lines is called parse_gpg_output in v5. Perhaps just try again with v6 of the patch, the difference between v5 and v6 is parse_gpg_output (Junio did not like the v5 variant). Thanks Sebastian -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] check_everything_connected replacement
My investigation in lowering connectivity check cost in git-clone [1] led me to try 'index-pack --strict' code. Without calling fsck_object(), --strict seems to be cheaper than check_everything_connected() while accomplishing the same thing (imo). The first patch is a bug fix running git-clone --depth with fetch.fsckObjects on. The second is fix while i'm there. The third introduces check_everything_connected alternative. The fourth makes use of it. The last use of check_everything_connected after this series is fetch.c:quickfetch(), which I think is unnecessary. It can only catch errors if we have incomplete object islands in repo, which could happen before this series. After this series, incomplete object islands can't enter the repository, at least via git transport. So maybe we should remove that check_everything_connected too (maybe after a few years, enough time for the laziest user to run fsck/repack once). [1] http://article.gmane.org/gmane.comp.version-control.git/219602 Nguyễn Thái Ngọc Duy (4): fetch-pack: save shallow file before fetching the pack index-pack: remove dead code (it should never happen) index-pack, unpack-objects: add --not-so-strict for connectivity check Use --not-so-strict on all pack transfer for connectivity check builtin/fetch.c | 6 builtin/index-pack.c| 10 -- builtin/receive-pack.c | 22 +++-- builtin/unpack-objects.c| 9 -- fetch-pack.c| 72 +++-- t/t5500-fetch-pack.sh | 7 t/t5504-fetch-receive-strict.sh | 2 +- 7 files changed, 66 insertions(+), 62 deletions(-) -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] fetch-pack: save shallow file before fetching the pack
index-pack --strict looks up and follows parent commits. If shallow
information is not ready by the time index-pack is run, index-pack may
be lead to non-existent objects. Make fetch-pack save shallow file to
disk before invoking index-pack.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
fetch-pack.c | 70 ---
t/t5500-fetch-pack.sh | 7 ++
2 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index cef8fde..1f9c5ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -779,11 +779,44 @@ static int cmp_ref_by_name(const void *a_, const void *b_)
return strcmp(a-name, b-name);
}
+static void flush_shallow_to_disk(struct stat *st)
+{
+ static struct lock_file lock;
+ struct cache_time mtime;
+ struct strbuf sb = STRBUF_INIT;
+ char *shallow = git_path(shallow);
+ int fd;
+
+ mtime.sec = st-st_mtime;
+ mtime.nsec = ST_MTIME_NSEC(*st);
+ if (stat(shallow, st)) {
+ if (mtime.sec)
+ die(shallow file was removed during fetch);
+ } else if (st-st_mtime != mtime.sec
+#ifdef USE_NSEC
+ || ST_MTIME_NSEC(*st) != mtime.nsec
+#endif
+ )
+ die(shallow file was changed during fetch);
+
+ fd = hold_lock_file_for_update(lock, shallow,
+ LOCK_DIE_ON_ERROR);
+ if (!write_shallow_commits(sb, 0)
+ || write_in_full(fd, sb.buf, sb.len) != sb.len) {
+ unlink_or_warn(shallow);
+ rollback_lock_file(lock);
+ } else {
+ commit_lock_file(lock);
+ }
+ strbuf_release(sb);
+}
+
static struct ref *do_fetch_pack(struct fetch_pack_args *args,
int fd[2],
const struct ref *orig_ref,
struct ref **sought, int nr_sought,
-char **pack_lockfile)
+char **pack_lockfile,
+struct stat *shallow_st)
{
struct ref *ref = copy_ref_list(orig_ref);
unsigned char sha1[20];
@@ -858,6 +891,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
if (args-stateless_rpc)
packet_flush(fd[1]);
+ if (args-depth 0)
+ flush_shallow_to_disk(shallow_st);
if (get_pack(args, fd, pack_lockfile))
die(git fetch-pack: fetch failed.);
@@ -952,38 +987,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
packet_flush(fd[1]);
die(no matching remote head);
}
- ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
pack_lockfile);
- if (args-depth 0) {
- static struct lock_file lock;
- struct cache_time mtime;
- struct strbuf sb = STRBUF_INIT;
- char *shallow = git_path(shallow);
- int fd;
-
- mtime.sec = st.st_mtime;
- mtime.nsec = ST_MTIME_NSEC(st);
- if (stat(shallow, st)) {
- if (mtime.sec)
- die(shallow file was removed during fetch);
- } else if (st.st_mtime != mtime.sec
-#ifdef USE_NSEC
- || ST_MTIME_NSEC(st) != mtime.nsec
-#endif
- )
- die(shallow file was changed during fetch);
-
- fd = hold_lock_file_for_update(lock, shallow,
- LOCK_DIE_ON_ERROR);
- if (!write_shallow_commits(sb, 0)
-|| write_in_full(fd, sb.buf, sb.len) != sb.len) {
- unlink_or_warn(shallow);
- rollback_lock_file(lock);
- } else {
- commit_lock_file(lock);
- }
- strbuf_release(sb);
- }
+ ref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,
+ pack_lockfile, st);
reprepare_packed_git();
return ref_cpy;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d574085..557b073 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '
test `git --git-dir=shallow0/.git rev-list --count HEAD` = 1
'
+test_expect_success 'clone shallow depth 1 with fsck' '
+ git config --global fetch.fsckobjects true
+ git clone --no-single-branch --depth 1 file://$(pwd)/. shallow0fsck
+ test `git --git-dir=shallow0fsck/.git rev-list --count HEAD` = 1
+ git config --global --unset fetch.fsckobjects
+'
+
test_expect_success 'clone shallow' '
git clone --no-single-branch --depth 2 file://$(pwd)/. shallow
'
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line
[PATCH 2/4] index-pack: remove dead code (it should never happen)
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- builtin/index-pack.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index ef62124..fdac7c7 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -735,8 +735,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry, int eaten; void *buf = (void *) data; - if (!buf) - buf = new_data = get_data_from_pack(obj_entry); + assert(data data can only be NULL for large blobs); /* * we do not need to free the memory here, as the -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] index-pack, unpack-objects: add --not-so-strict for connectivity check
--not-so-strict only checks if all links from objects in the pack
point to real objects (either in current repo, or from the pack
itself). It's like check_everything_connected() except that:
- it does not follow DAG in order
- it can detect incomplete object islands
- it seems to be faster than rev-list --objects --all
On my box, rev-list --objects --all takes 34 seconds. index-pack takes
215.25user 8.42system 1:32.31elapsed 242%CPU (0avgtext+0avgdata
1357328maxresident)k
0inputs+1421016outputs (0major+1222987minor)pagefaults 0swaps
And index-pack --not-so-strict takes
pack96a4e3befa40bf38eddc2d7c99246a59af4ad55d
229.75user 11.31system 1:42.50elapsed 235%CPU (0avgtext+0avgdata
1876816maxresident)k
0inputs+1421016outputs (0major+1307989minor)pagefaults 0swaps
The overhead is about 10 seconds, just 1/3 of rev-list, which makes it
in a better position to replace check_everything_connected(). If this
holds true for general case, it could reduce fetch time by a little bit.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/index-pack.c | 7 ++-
builtin/unpack-objects.c | 9 +++--
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index fdac7c7..3cded32 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -77,6 +77,7 @@ static int nr_threads;
static int from_stdin;
static int strict;
+static int do_fsck_object;
static int verbose;
static struct progress *progress;
@@ -744,7 +745,8 @@ static void sha1_object(const void *data, struct
object_entry *obj_entry,
obj = parse_object_buffer(sha1, type, size, buf,
eaten);
if (!obj)
die(_(invalid %s), typename(type));
- if (fsck_object(obj, 1, fsck_error_function))
+ if (do_fsck_object
+ fsck_object(obj, 1, fsck_error_function))
die(_(Error in object));
if (fsck_walk(obj, mark_link, NULL))
die(_(Not all child objects of %s are
reachable), sha1_to_hex(obj-sha1));
@@ -1491,6 +1493,9 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
fix_thin_pack = 1;
} else if (!strcmp(arg, --strict)) {
strict = 1;
+ do_fsck_object = 1;
+ } else if (!strcmp(arg, --not-so-strict)) {
+ strict = 1;
} else if (!strcmp(arg, --verify)) {
verify = 1;
} else if (!strcmp(arg, --verify-stat)) {
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 2217d7b..dd0518b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -12,7 +12,7 @@
#include decorate.h
#include fsck.h
-static int dry_run, quiet, recover, has_errors, strict;
+static int dry_run, quiet, recover, has_errors, strict, do_fsck_object;
static const char unpack_usage[] = git unpack-objects [-n] [-q] [-r]
[--strict] pack-file;
/* We always read in 4kB chunks. */
@@ -198,7 +198,7 @@ static int check_object(struct object *obj, int type, void
*data)
return 0;
}
- if (fsck_object(obj, 1, fsck_error_function))
+ if (do_fsck_object fsck_object(obj, 1, fsck_error_function))
die(Error in object);
if (fsck_walk(obj, check_object, NULL))
die(Error on reachable objects of %s, sha1_to_hex(obj-sha1));
@@ -520,6 +520,11 @@ int cmd_unpack_objects(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --strict)) {
+ do_fsck_object = 1;
+ strict = 1;
+ continue;
+ }
+ if (!strcmp(arg, --not-so-strict)) {
strict = 1;
continue;
}
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] Use --not-so-strict on all pack transfer for connectivity check
This replaces check_everything_connected() with --not-so-strict, which
accomplishes the same thing and is generally cheaper.
This also forces connectivity check on git clone.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/fetch.c | 6 --
builtin/receive-pack.c | 22 --
fetch-pack.c| 2 ++
t/t5504-fetch-receive-strict.sh | 2 +-
4 files changed, 7 insertions(+), 25 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b6b1df..d9f970f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -400,12 +400,6 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
else
url = xstrdup(foreign);
- rm = ref_map;
- if (check_everything_connected(iterate_ref_map, 0, rm)) {
- rc = error(_(%s did not send all necessary objects\n), url);
- goto abort;
- }
-
/*
* The first pass writes objects to be merged and then the
* second pass writes the rest, in order to allow using
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 62ba6e7..07abb14 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -663,19 +663,6 @@ static int command_singleton_iterator(void *cb_data,
unsigned char sha1[20])
return 0;
}
-static void set_connectivity_errors(struct command *commands)
-{
- struct command *cmd;
-
- for (cmd = commands; cmd; cmd = cmd-next) {
- struct command *singleton = cmd;
- if (!check_everything_connected(command_singleton_iterator,
- 0, singleton))
- continue;
- cmd-error_string = missing necessary objects;
- }
-}
-
static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
{
struct command **cmd_list = cb_data;
@@ -718,11 +705,6 @@ static void execute_commands(struct command *commands,
const char *unpacker_erro
return;
}
- cmd = commands;
- if (check_everything_connected(iterate_receive_command_list,
- 0, cmd))
- set_connectivity_errors(commands);
-
reject_updates_to_hidden(commands);
if (run_receive_hook(commands, pre-receive, 0)) {
@@ -843,6 +825,8 @@ static const char *unpack(int err_fd)
unpacker[i++] = -q;
if (fsck_objects)
unpacker[i++] = --strict;
+ else
+ unpacker[i++] = --not-so-strict;
unpacker[i++] = hdr_arg;
unpacker[i++] = NULL;
memset(child, 0, sizeof(child));
@@ -868,6 +852,8 @@ static const char *unpack(int err_fd)
keeper[i++] = --stdin;
if (fsck_objects)
keeper[i++] = --strict;
+ else
+ keeper[i++] = --not-so-strict;
keeper[i++] = --fix-thin;
keeper[i++] = hdr_arg;
keeper[i++] = keep_arg;
diff --git a/fetch-pack.c b/fetch-pack.c
index 1f9c5ba..ae20ae5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -754,6 +754,8 @@ static int get_pack(struct fetch_pack_args *args,
? transfer_fsck_objects
: 0)
*av++ = --strict;
+ else
+ *av++ = --not-so-strict;
*av++ = NULL;
cmd.in = demux.out;
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 69ee13c..14d2935 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -60,7 +60,7 @@ test_expect_success 'fetch with transfer.fsckobjects' '
cat exp EOF
To dst
-! refs/heads/master:refs/heads/test [remote rejected] (missing
necessary objects)
+! refs/heads/master:refs/heads/test [remote rejected] (unpacker
error)
EOF
test_expect_success 'push without strict' '
--
1.8.2.83.gc99314b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de wrote: On 03/31/2013 10:32 AM, Thomas Rast wrote: + test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror + test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) though I currently cannot see what's wrong, probably because I don't know the format that parse_signature_lines gives. Can you look into it? Against what version/combination of the patches are you running the test? parse_signature_lines is called parse_gpg_output in v5. Perhaps just try again with v6 of the patch, the difference between v5 and v6 is parse_gpg_output (Junio did not like the v5 variant). Oh, my bad then. I used the version in pu. I just ran all tests under valgrind and this cropped up. If you have valgrind installed locally, you can also test yourself ;-) just pass --valgrind to the test script. -- http://code.google.com/p/k9mail -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 01:38 PM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de wrote: On 03/31/2013 10:32 AM, Thomas Rast wrote: + test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror + test_i18ngrep from an untrusted key mergeerror +' This test gives me the following: ==26527== Conditional jump or move depends on uninitialised value(s) ==26527==at 0x4C2D8BC: strchrnul (mc_replace_strmem.c:1084) ==26527==by 0x4989CC: parse_signature_lines (commit.c:1074) ==26527==by 0x498B33: check_commit_signature (commit.c:1100) ==26527==by 0x453719: cmd_merge (merge.c:1246) ==26527==by 0x4057B6: run_builtin (git.c:282) ==26527==by 0x405949: handle_internal_command (git.c:444) ==26527==by 0x405A63: run_argv (git.c:490) ==26527==by 0x405BF2: main (git.c:565) [...] If you have valgrind installed locally, you can also test yourself ;-) just pass --valgrind to the test script. Ok, I can reproduce this with v6 of the patch: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror test_i18ngrep has a good, untrusted GPG signature mergeerror ==1430== Conditional jump or move depends on uninitialised value(s) ==1430==at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711) ==1430==by 0x47B90B: check_commit_signature (commit.c:1057) ==1430==by 0x444212: cmd_merge (merge.c:1245) ==1430==by 0x4050E6: handle_internal_command (git.c:281) ==1430==by 0x40530C: main (git.c:489) Though I also can't see the problem. strchrnul gets passed a char* that is just fine. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror test_i18ngrep has a good, untrusted GPG signature mergeerror ==1430== Conditional jump or move depends on uninitialised value(s) ==1430==at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711) ==1430==by 0x47B90B: check_commit_signature (commit.c:1057) ==1430==by 0x444212: cmd_merge (merge.c:1245) ==1430==by 0x4050E6: handle_internal_command (git.c:281) ==1430==by 0x40530C: main (git.c:489) Though I also can't see the problem. strchrnul gets passed a char* that is just fine. Usually that means that the string *contents* are uninitialized, usually because you scanned past the end of the string... -- http://code.google.com/p/k9mail -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 02:16 PM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror test_i18ngrep has a good, untrusted GPG signature mergeerror ==1430== Conditional jump or move depends on uninitialised value(s) ==1430==at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711) ==1430==by 0x47B90B: check_commit_signature (commit.c:1057) ==1430==by 0x444212: cmd_merge (merge.c:1245) ==1430==by 0x4050E6: handle_internal_command (git.c:281) ==1430==by 0x40530C: main (git.c:489) Though I also can't see the problem. strchrnul gets passed a char* that is just fine. Usually that means that the string *contents* are uninitialized, usually because you scanned past the end of the string... I checked for that, everything looks fine to me. The pointer should point to a valid, 0-terminated string. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures
On Sun, Mar 31, 2013 at 02:27:20PM +0200, Sebastian Götte wrote: On 03/31/2013 02:16 PM, Thomas Rast wrote: Sebastian Götte ja...@physik.tu-berlin.de wrote: expecting success: test_must_fail git merge --ff-only --verify-signatures side-untrusted 2mergeerror test_i18ngrep has a good, untrusted GPG signature mergeerror ==1430== Conditional jump or move depends on uninitialised value(s) ==1430==at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711) ==1430==by 0x47B90B: check_commit_signature (commit.c:1057) ==1430==by 0x444212: cmd_merge (merge.c:1245) ==1430==by 0x4050E6: handle_internal_command (git.c:281) ==1430==by 0x40530C: main (git.c:489) Though I also can't see the problem. strchrnul gets passed a char* that is just fine. Usually that means that the string *contents* are uninitialized, usually because you scanned past the end of the string... I checked for that, everything looks fine to me. The pointer should point to a valid, 0-terminated string. It looks like the found pointer has wandered off the end of the string. In the test case here, the gpg_status is: -- 8 -- [GNUPG:] SIG_ID rzX3GbdzQyxB4Jdm1uD0CzL4B4Y 2013-03-31 1364735152 [GNUPG:] GOODSIG 61092E85B7227189 Eris Discordia disc...@example.net [GNUPG:] VALIDSIG D4BE22311AD3131E5EDA29A461092E85B7227189 2013-03-31 1364735152 0 4 0 1 2 00 D4BE22311AD3131E5EDA29A461092E85B7227189 [GNUPG:] TRUST_UNDEFINED -- 8 -- But the parse_signature_lines code assumes that after reading a signature it can fill in the key from the next 16 bytes and then look for a newline after that. In this case it clearly needs to only read the signature if it's a GOODSIG or BADSIG line. Wrapping a signature_check[i].result != 'U' condition around the lines that extract the key and advance the found pointer after doing so fixes this for me. John -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 0/5] Verify GPG signatures when merging and extend %G? pretty string
On 03/31/2013 03:33 PM, John Keeping wrote: It looks like the found pointer has wandered off the end of the string. In the test case here, the gpg_status is: -- 8 -- [GNUPG:] SIG_ID rzX3GbdzQyxB4Jdm1uD0CzL4B4Y 2013-03-31 1364735152 [GNUPG:] GOODSIG 61092E85B7227189 Eris Discordia disc...@example.net [GNUPG:] VALIDSIG D4BE22311AD3131E5EDA29A461092E85B7227189 2013-03-31 1364735152 0 4 0 1 2 00 D4BE22311AD3131E5EDA29A461092E85B7227189 [GNUPG:] TRUST_UNDEFINED -- 8 -- But the parse_signature_lines code assumes that after reading a signature it can fill in the key from the next 16 bytes and then look for a newline after that. In this case it clearly needs to only read the signature if it's a GOODSIG or BADSIG line. Wrapping a signature_check[i].result != 'U' condition around the lines that extract the key and advance the found pointer after doing so fixes this for me. This was in fact the case and your fix works. I modified the code a bit so it does not break at the end of the loop and it checks for untrusted signatures *last*, this way even in case 'signature_check.result' is 'U' (untrusted), 'key' and 'signer' are available. I also removed two stray spaces. 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| 34 +- commit.c | 69 +++ commit.h | 10 git-pull.sh| 10 +++- gpg-interface.h| 12 + 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, 215 insertions(+), 82 deletions(-) create mode 100755 t/t7612-merge-verify-signatures.sh -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 1/5] Move commit GPG signature verification to commit.c
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c| 59 +
commit.h| 10 +++
gpg-interface.h | 11 +++
pretty.c| 91 +
4 files changed, 93 insertions(+), 78 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..eb645af 100644
--- a/commit.c
+++ b/commit.c
@@ -1023,6 +1023,65 @@ free_return:
free(buf);
}
+static struct {
+ char result;
+ const char *check;
+} sigcheck_gpg_status[] = {
+ { 'G', \n[GNUPG:] GOODSIG },
+ { 'B', \n[GNUPG:] BADSIG },
+};
+
+static void parse_gpg_output(struct signature_check *sigc)
+{
+ const char *buf = sigc-gpg_status;
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ const char *found = strstr(buf, sigcheck_gpg_status[i].check);
+ const char *next;
+ if (!found)
+ continue;
+ sigc-result = sigcheck_gpg_status[i].result;
+ found += strlen(sigcheck_gpg_status[i].check);
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ break;
+ }
+}
+
+void check_commit_signature(const struct commit* commit, struct
signature_check *sigc)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+ struct strbuf gpg_output = STRBUF_INIT;
+ struct strbuf gpg_status = STRBUF_INIT;
+ int status;
+
+ sigc-result = 'N';
+
+ if (parse_signed_commit(commit-object.sha1,
+ payload, signature) = 0)
+ goto out;
+ status = verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len,
+ gpg_output, gpg_status);
+ if (status !gpg_output.len)
+ goto out;
+ sigc-gpg_output = strbuf_detach(gpg_output, NULL);
+ sigc-gpg_status = strbuf_detach(gpg_status, NULL);
+ parse_gpg_output(sigc);
+
+ out:
+ strbuf_release(gpg_status);
+ strbuf_release(gpg_output);
+ strbuf_release(payload);
+ strbuf_release(signature);
+}
+
+
+
void append_merge_tag_headers(struct commit_list *parents,
struct commit_extra_header ***tail)
{
diff --git a/commit.h b/commit.h
index 4138bb4..8bbcf13 100644
--- a/commit.h
+++ b/commit.h
@@ -5,6 +5,7 @@
#include tree.h
#include strbuf.h
#include decorate.h
+#include gpg-interface.h
struct commit_list {
struct commit *item;
@@ -230,4 +231,13 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+/*
+ * Check the signature of the given commit. The result of the check is stored
in
+ * sig-result, 'G' for a good signature, 'B' for a bad signature and 'N'
+ * for no signature at all.
+ * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
+ * and sig-key.
+ */
+extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
+
#endif /* COMMIT_H */
diff --git a/gpg-interface.h b/gpg-interface.h
index cf99021..5884aa4 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,6 +1,17 @@
#ifndef GPG_INTERFACE_H
#define GPG_INTERFACE_H
+struct signature_check {
+ char *gpg_output;
+ char *gpg_status;
+ char result; /* 0 (not checked),
+ * N (checked but no further result),
+ * G (good)
+ * B (bad) */
+ char *signer;
+ char *key;
+};
+
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const
char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size,
const char *signature, size_t signature_size, struct strbuf *gpg_output, struct
strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index b57adef..cf84d3a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -756,14 +756,7 @@ struct format_commit_context {
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
- unsigned commit_signature_parsed:1;
- struct {
- char *gpg_output;
- char *gpg_status;
- char good_bad;
- char *signer;
- char *key;
- } signature;
+ struct signature_check signature_check;
char *message;
size_t width, indent1, indent2;
@@ -946,64 +939,6 @@ static void rewrap_message_tail(struct strbuf *sb,
c-indent2 = new_indent2;
}
-static struct {
- char result;
- const char *check;
-}
[PATCH v7 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/commit.c b/commit.c
index eb645af..eda7f90 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,8 +1027,8 @@ static struct {
char result;
const char *check;
} sigcheck_gpg_status[] = {
- { 'G', \n[GNUPG:] GOODSIG },
- { 'B', \n[GNUPG:] BADSIG },
+ { 'G', [GNUPG:] GOODSIG },
+ { 'B', [GNUPG:] BADSIG },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1036,13 +1036,20 @@ static void parse_gpg_output(struct signature_check
*sigc)
const char *buf = sigc-gpg_status;
int i;
+ /* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found = strstr(buf, sigcheck_gpg_status[i].check);
- const char *next;
- if (!found)
- continue;
+ const char *found, *next;
+
+ if (!prefixcmp(buf, sigcheck_gpg_status[i].check + 1)) {
+ /* At the very beginning of the buffer */
+ found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+ } else {
+ found = strstr(buf, sigcheck_gpg_status[i].check);
+ if (!found)
+ continue;
+ found += strlen(sigcheck_gpg_status[i].check);
+ }
sigc-result = sigcheck_gpg_status[i].result;
- found += strlen(sigcheck_gpg_status[i].check);
sigc-key = xmemdupz(found, 16);
found += 17;
next = strchrnul(found, '\n');
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 3/5] merge/pull: verify GPG signatures of commits being merged
When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 5
builtin/merge.c| 32 ++-
git-pull.sh| 10 ++--
t/t7612-merge-verify-signatures.sh | 52 ++
4 files changed, 96 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..31f1067 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -83,6 +83,11 @@ option can be used to override --squash.
Pass merge strategy specific option through to the merge
strategy.
+--verify-signatures::
+--no-verify-signatures::
+ Verify that the commits being merged have good GPG signatures and abort
the
+ merge in case they do not.
+
--summary::
--no-summary::
Synonyms to --stat and --no-stat; these are deprecated and will be
diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..7a33d03 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = 1, allow_fast_forward = 1;
static int fast_forward_only, option_edit = -1;
-static int allow_trivial = 1, have_message;
+static int allow_trivial = 1, have_message, verify_signatures;
static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
@@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
OPT_BOOLEAN(0, ff-only, fast_forward_only,
N_(abort if fast-forward is not possible)),
OPT_RERERE_AUTOUPDATE(allow_rerere_auto),
+ OPT_BOOL(0, verify-signatures, verify_signatures,
+ N_(Verify that the named commit has a valid GPG signature)),
OPT_CALLBACK('s', strategy, use_strategies, N_(strategy),
N_(merge strategy to use), option_parse_strategy),
OPT_CALLBACK('X', strategy-option, xopts, N_(option=value),
@@ -1233,6 +1235,34 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
+ if (verify_signatures) {
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ char hex[41];
+ struct signature_check signature_check;
+ memset(signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, signature_check);
+
+ strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
+ switch(signature_check.result){
+ case 'G':
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
+ default: /* 'N' */
+ die(_(Commit %s does not have a GPG
signature.), hex, hex);
+ }
+ if (verbosity = 0 signature_check.result == 'G')
+ printf(_(Commit %s has a good GPG signature by
%s\n), hex, signature_check.signer);
+
+ free(signature_check.gpg_output);
+ free(signature_check.gpg_status);
+ free(signature_check.signer);
+ free(signature_check.key);
+ }
+ }
+
strbuf_addstr(buf, merge);
for (p = remoteheads; p; p = p-next)
strbuf_addf(buf, %s, merge_remote_util(p-item)-name);
diff --git a/git-pull.sh b/git-pull.sh
index 266e682..705940d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -39,7 +39,7 @@ test -z $(git ls-files -u) || die_conflict
test -f $GIT_DIR/MERGE_HEAD die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
merge_args= edit=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short=${curr_branch#refs/heads/}
@@ -125,6 +125,12 @@ do
--no-recurse-submodules)
recurse_submodules=--no-recurse-submodules
;;
+ --verify-signatures)
+ verify_signatures=--verify-signatures
+ ;;
+ --no-verify-signatures)
+ verify_signatures=--no-verify-signatures
+ ;;
[PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 4 ++--
builtin/merge.c| 2 ++
commit.c | 13 -
commit.h | 10 +-
gpg-interface.h| 1 +
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 | 9 +
10 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 31f1067..a0f022b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -85,8 +85,8 @@ option can be used to override --squash.
--verify-signatures::
--no-verify-signatures::
- Verify that the commits being merged have good GPG signatures and abort
the
- merge in case they do not.
+ Verify that the commits being merged have good and trusted GPG
signatures
+ and abort the merge in case they do not.
--summary::
--no-summary::
diff --git a/builtin/merge.c b/builtin/merge.c
index 7a33d03..752e3a9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1248,6 +1248,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
switch(signature_check.result){
case 'G':
break;
+ case 'U':
+ die(_(Commit %s has a good, untrusted
GPG signature allegedly by %s.), hex, signature_check.signer);
case 'B':
die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
default: /* 'N' */
diff --git a/commit.c b/commit.c
index eda7f90..bb2d9ad 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+ { 'U', [GNUPG:] TRUST_NEVER },
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
*sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ if (sigc-result != 'U') {
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ }
}
}
diff --git a/commit.h b/commit.h
index 8bbcf13..27d9b36 100644
--- a/commit.h
+++ b/commit.h
@@ -232,11 +232,11 @@ extern void print_commit_list(struct commit_list *list,
const char *format_last);
/*
- * Check the signature of the given commit. The result of the check is stored
in
- * sig-result, 'G' for a good signature, 'B' for a bad signature and 'N'
- * for no signature at all.
- * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
- * and sig-key.
+ * Check the signature of the given commit. The result of the check is stored
+ * in sig-check_result, 'G' for a good signature, 'U' for a good signature
+ * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
+ * at all. This may allocate memory for sig-gpg_output, sig-gpg_status,
+ * sig-signer and sig-key.
*/
extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index 5884aa4..a85cb5b 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -6,6 +6,7 @@ struct signature_check {
char *gpg_status;
char result; /* 0 (not checked),
* N (checked but no further result),
+ * U (untrusted good),
* G (good)
* B (bad) */
char *signer;
diff --git a/t/lib-gpg/pubring.gpg b/t/lib-gpg/pubring.gpg
index
83855fa4e1c6c37afe550c17afa1e7971042ded5..1a3c2d487c2fda9169751a3068fa51e853a1e519
100644
GIT binary patch
delta 1212
zcmV;t1Vj6b3AYlkj0As~0SyFEOqNFh2mr~8{AU1PG9!Ku9w|}@vpZJPg*#s86v-*O
zhafj(DLlFA0)`tvC$E@WHJ2r~0{0ZCh1kHo$b9ih^aD*~)oVvKyC1(yi)6x_y
zF8V3JpbIY^ZYQUk#j*ja0`;jw^J+5~!h3qc)ej%g1;Wb0U8cXSuLKdXkx2PUtB4
[PATCH v7 5/5] pretty printing: extend %G? to include 'N' and 'U'
Expand %G? in pretty format strings to 'N' in case of no GPG signature
and 'U' in case of a good but untrusted GPG signature in addition to
the previous 'G'ood and 'B'ad. This eases writing anyting parsing
git-log output.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/pretty-formats.txt | 3 ++-
pretty.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2939655..afac703 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -131,7 +131,8 @@ The placeholders are:
- '%B': raw body (unwrapped subject and body)
- '%N': commit notes
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show either G for Good or B for Bad for a signed commit
+- '%G?': show G for a Good signature, B for a Bad signature, U for a
good,
+ untrusted signature and N for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}`
diff --git a/pretty.c b/pretty.c
index cf84d3a..840c41f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1135,6 +1135,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (c-signature_check.result) {
case 'G':
case 'B':
+ case 'U':
+ case 'N':
strbuf_addch(sb, c-signature_check.result);
}
break;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
On Sun, Mar 31, 2013 at 04:32:52PM +0200, Sebastian Götte wrote:
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/commit.c b/commit.c
index eb645af..eda7f90 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,8 +1027,8 @@ static struct {
char result;
const char *check;
} sigcheck_gpg_status[] = {
- { 'G', \n[GNUPG:] GOODSIG },
- { 'B', \n[GNUPG:] BADSIG },
+ { 'G', [GNUPG:] GOODSIG },
+ { 'B', [GNUPG:] BADSIG },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1036,13 +1036,20 @@ static void parse_gpg_output(struct signature_check
*sigc)
const char *buf = sigc-gpg_status;
int i;
+ /* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found = strstr(buf, sigcheck_gpg_status[i].check);
- const char *next;
- if (!found)
- continue;
+ const char *found, *next;
+
+ if (!prefixcmp(buf, sigcheck_gpg_status[i].check + 1)) {
+ /* At the very beginning of the buffer */
This seems wrong. You're losing the \n in front of the status strings
above but adding a special first line check skipping the first
character. Surely it should be one of these changes or the other, not
both?
+ found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+ } else {
+ found = strstr(buf, sigcheck_gpg_status[i].check);
+ if (!found)
+ continue;
+ found += strlen(sigcheck_gpg_status[i].check);
+ }
sigc-result = sigcheck_gpg_status[i].result;
- found += strlen(sigcheck_gpg_status[i].check);
sigc-key = xmemdupz(found, 16);
found += 17;
next = strchrnul(found, '\n');
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 4 ++--
builtin/merge.c| 2 ++
commit.c | 13 -
commit.h | 10 +-
gpg-interface.h| 1 +
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 | 9 +
10 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 31f1067..a0f022b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -85,8 +85,8 @@ option can be used to override --squash.
--verify-signatures::
--no-verify-signatures::
- Verify that the commits being merged have good GPG signatures and abort
the
- merge in case they do not.
+ Verify that the commits being merged have good and trusted GPG
signatures
+ and abort the merge in case they do not.
--summary::
--no-summary::
diff --git a/builtin/merge.c b/builtin/merge.c
index 7a33d03..752e3a9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1248,6 +1248,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
switch(signature_check.result){
case 'G':
break;
+ case 'U':
+ die(_(Commit %s has a good, untrusted
GPG signature allegedly by %s.), hex, signature_check.signer);
case 'B':
die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
default: /* 'N' */
diff --git a/commit.c b/commit.c
index eda7f90..bb2d9ad 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+ { 'U', [GNUPG:] TRUST_NEVER },
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
*sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ if (sigc-result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ }
}
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
John Keeping j...@keeping.me.uk writes:
On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
diff --git a/commit.c b/commit.c
index eda7f90..bb2d9ad 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+{ 'U', [GNUPG:] TRUST_NEVER },
+{ 'U', [GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
*sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
-sigc-key = xmemdupz(found, 16);
-found += 17;
-next = strchrnul(found, '\n');
-sigc-signer = xmemdupz(found, next - found);
-break;
+if (sigc-result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
Wouldn't it be even less magical if there was an explicit field in the
struct that says whether we need to read a sig from such a line?
And furthermore, to use an enum instead of a char so that you can easily
spell out the details in the code? This also has the advantage that the
compiler can check that your 'switch'es cover all cases.
That's of course assuming that I interpret the logic right; my current
understanding is that:
* U means untrusted, which is bettern than B but worse than G;
* GPG guarantees that the last line matching any of the patterns is the
one we care about, so we can blindly override one with the other
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 05:03 PM, Thomas Rast wrote:
John Keeping j...@keeping.me.uk writes:
On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
diff --git a/commit.c b/commit.c
index eda7f90..bb2d9ad 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+ { 'U', [GNUPG:] TRUST_NEVER },
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
*sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ if (sigc-result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
Wouldn't it be even less magical if there was an explicit field in the
struct that says whether we need to read a sig from such a line?
I think that special-case if a few lines below is OK for now.
And furthermore, to use an enum instead of a char so that you can easily
spell out the details in the code? This also has the advantage that the
compiler can check that your 'switch'es cover all cases.
This char is actually from Junios original code. I think we can afford
three chars. This could be changed if we ever need more than that. Another
possible future feature would be to distinguish between no signature and
public key not found and to allow pass-through of that GPG retrieve
missing public keys from keyserver option.
That's of course assuming that I interpret the logic right; my current
understanding is that:
* U means untrusted, which is bettern than B but worse than G;
Correct. Also, BADSIG is never followed by trust information.
* GPG guarantees that the last line matching any of the patterns is the
one we care about, so we can blindly override one with the other
Actually, we are searching *all* GPG status lines for every search string
in the array. This way, first GOODSIG may be matched to fill in the key
and signer information, but a subsequent TRUST_* match finally sets the
result code.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
On Sun, Mar 31, 2013 at 05:03:44PM +0200, Thomas Rast wrote:
John Keeping j...@keeping.me.uk writes:
On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
diff --git a/commit.c b/commit.c
index eda7f90..bb2d9ad 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+ { 'U', [GNUPG:] TRUST_NEVER },
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct
signature_check *sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ if (sigc-result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
Wouldn't it be even less magical if there was an explicit field in the
struct that says whether we need to read a sig from such a line?
Yeah, that would be even better.
And furthermore, to use an enum instead of a char so that you can easily
spell out the details in the code? This also has the advantage that the
compiler can check that your 'switch'es cover all cases.
That's of course assuming that I interpret the logic right; my current
understanding is that:
* U means untrusted, which is bettern than B but worse than G;
Yes, although I wonder if we should split TRUST_NEVER and
TRUST_UNDEFINED (and maybe handle TRUST_MARGINAL as well) and print
different messages in each case.
* GPG guarantees that the last line matching any of the patterns is the
one we care about, so we can blindly override one with the other
The DETAILS file in GPG says:
For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted.
so we should be OK here.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte ja...@physik.tu-berlin.de writes:
On 03/31/2013 05:03 PM, Thomas Rast wrote:
} sigcheck_gpg_status[] = {
{ 'G', [GNUPG:] GOODSIG },
{ 'B', [GNUPG:] BADSIG },
+ { 'U', [GNUPG:] TRUST_NEVER },
+ { 'U', [GNUPG:] TRUST_UNDEFINED },
[...]
And furthermore, to use an enum instead of a char so that you can easily
spell out the details in the code? This also has the advantage that the
compiler can check that your 'switch'es cover all cases.
This char is actually from Junios original code. I think we can afford
three chars. This could be changed if we ever need more than that.
*shrug*
I'm tempted to count the above as an argument in favor of the enum,
since there are in fact *four* chars... 'N' also counts. ;-)
But either way... I don't care too deeply and I don't know this corner
of the code. I just came here because of the valgrind discovery.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 0/5] Verify GPG signatures when merging and extend %G? pretty string
On 03/31/2013 04:41 PM, John Keeping wrote: On Sun, Mar 31, 2013 at 04:32:52PM
+0200, Sebastian Götte wrote:
+/* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
-const char *found = strstr(buf, sigcheck_gpg_status[i].check);
-const char *next;
-if (!found)
-continue;
+const char *found, *next;
+
+if (!prefixcmp(buf, sigcheck_gpg_status[i].check + 1)) {
+/* At the very beginning of the buffer */
This seems wrong. You're losing the \n in front of the status strings
above but adding a special first line check skipping the first
character. Surely it should be one of these changes or the other, not
both?
You're right, that does not make a whole lot of sense.
On 03/31/2013 04:44 PM, John Keeping wrote:
+if (sigc-result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
Fixed.
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| 34 +-
commit.c | 70
commit.h | 10
git-pull.sh| 10 +++-
gpg-interface.h| 12 +
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, 216 insertions(+), 82 deletions(-)
create mode 100755 t/t7612-merge-verify-signatures.sh
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 1/5] Move commit GPG signature verification to commit.c
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c| 59 +
commit.h| 10 +++
gpg-interface.h | 11 +++
pretty.c| 91 +
4 files changed, 93 insertions(+), 78 deletions(-)
diff --git a/commit.c b/commit.c
index e8eb0ae..eb645af 100644
--- a/commit.c
+++ b/commit.c
@@ -1023,6 +1023,65 @@ free_return:
free(buf);
}
+static struct {
+ char result;
+ const char *check;
+} sigcheck_gpg_status[] = {
+ { 'G', \n[GNUPG:] GOODSIG },
+ { 'B', \n[GNUPG:] BADSIG },
+};
+
+static void parse_gpg_output(struct signature_check *sigc)
+{
+ const char *buf = sigc-gpg_status;
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ const char *found = strstr(buf, sigcheck_gpg_status[i].check);
+ const char *next;
+ if (!found)
+ continue;
+ sigc-result = sigcheck_gpg_status[i].result;
+ found += strlen(sigcheck_gpg_status[i].check);
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ break;
+ }
+}
+
+void check_commit_signature(const struct commit* commit, struct
signature_check *sigc)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+ struct strbuf gpg_output = STRBUF_INIT;
+ struct strbuf gpg_status = STRBUF_INIT;
+ int status;
+
+ sigc-result = 'N';
+
+ if (parse_signed_commit(commit-object.sha1,
+ payload, signature) = 0)
+ goto out;
+ status = verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len,
+ gpg_output, gpg_status);
+ if (status !gpg_output.len)
+ goto out;
+ sigc-gpg_output = strbuf_detach(gpg_output, NULL);
+ sigc-gpg_status = strbuf_detach(gpg_status, NULL);
+ parse_gpg_output(sigc);
+
+ out:
+ strbuf_release(gpg_status);
+ strbuf_release(gpg_output);
+ strbuf_release(payload);
+ strbuf_release(signature);
+}
+
+
+
void append_merge_tag_headers(struct commit_list *parents,
struct commit_extra_header ***tail)
{
diff --git a/commit.h b/commit.h
index 4138bb4..8bbcf13 100644
--- a/commit.h
+++ b/commit.h
@@ -5,6 +5,7 @@
#include tree.h
#include strbuf.h
#include decorate.h
+#include gpg-interface.h
struct commit_list {
struct commit *item;
@@ -230,4 +231,13 @@ extern void print_commit_list(struct commit_list *list,
const char *format_cur,
const char *format_last);
+/*
+ * Check the signature of the given commit. The result of the check is stored
in
+ * sig-result, 'G' for a good signature, 'B' for a bad signature and 'N'
+ * for no signature at all.
+ * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
+ * and sig-key.
+ */
+extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
+
#endif /* COMMIT_H */
diff --git a/gpg-interface.h b/gpg-interface.h
index cf99021..5884aa4 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,6 +1,17 @@
#ifndef GPG_INTERFACE_H
#define GPG_INTERFACE_H
+struct signature_check {
+ char *gpg_output;
+ char *gpg_status;
+ char result; /* 0 (not checked),
+ * N (checked but no further result),
+ * G (good)
+ * B (bad) */
+ char *signer;
+ char *key;
+};
+
extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const
char *signing_key);
extern int verify_signed_buffer(const char *payload, size_t payload_size,
const char *signature, size_t signature_size, struct strbuf *gpg_output, struct
strbuf *gpg_status);
extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index b57adef..cf84d3a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -756,14 +756,7 @@ struct format_commit_context {
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
- unsigned commit_signature_parsed:1;
- struct {
- char *gpg_output;
- char *gpg_status;
- char good_bad;
- char *signer;
- char *key;
- } signature;
+ struct signature_check signature_check;
char *message;
size_t width, indent1, indent2;
@@ -946,64 +939,6 @@ static void rewrap_message_tail(struct strbuf *sb,
c-indent2 = new_indent2;
}
-static struct {
- char result;
- const char *check;
-}
[PATCH v8 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/commit.c b/commit.c
index eb645af..3a8453d 100644
--- a/commit.c
+++ b/commit.c
@@ -1036,13 +1036,20 @@ static void parse_gpg_output(struct signature_check
*sigc)
const char *buf = sigc-gpg_status;
int i;
+ /* Iterate over all search strings */
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found = strstr(buf, sigcheck_gpg_status[i].check);
- const char *next;
- if (!found)
- continue;
+ const char *found, *next;
+
+ if (!prefixcmp(buf, sigcheck_gpg_status[i].check + 1)) {
+ /* At the very beginning of the buffer */
+ found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+ } else {
+ found = strstr(buf, sigcheck_gpg_status[i].check);
+ if (!found)
+ continue;
+ found += strlen(sigcheck_gpg_status[i].check);
+ }
sigc-result = sigcheck_gpg_status[i].result;
- found += strlen(sigcheck_gpg_status[i].check);
sigc-key = xmemdupz(found, 16);
found += 17;
next = strchrnul(found, '\n');
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 3/5] merge/pull: verify GPG signatures of commits being merged
When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 5
builtin/merge.c| 32 ++-
git-pull.sh| 10 ++--
t/t7612-merge-verify-signatures.sh | 52 ++
4 files changed, 96 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..31f1067 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -83,6 +83,11 @@ option can be used to override --squash.
Pass merge strategy specific option through to the merge
strategy.
+--verify-signatures::
+--no-verify-signatures::
+ Verify that the commits being merged have good GPG signatures and abort
the
+ merge in case they do not.
+
--summary::
--no-summary::
Synonyms to --stat and --no-stat; these are deprecated and will be
diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..7a33d03 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
static int show_diffstat = 1, shortlog_len = -1, squash;
static int option_commit = 1, allow_fast_forward = 1;
static int fast_forward_only, option_edit = -1;
-static int allow_trivial = 1, have_message;
+static int allow_trivial = 1, have_message, verify_signatures;
static int overwrite_ignore = 1;
static struct strbuf merge_msg = STRBUF_INIT;
static struct strategy **use_strategies;
@@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
OPT_BOOLEAN(0, ff-only, fast_forward_only,
N_(abort if fast-forward is not possible)),
OPT_RERERE_AUTOUPDATE(allow_rerere_auto),
+ OPT_BOOL(0, verify-signatures, verify_signatures,
+ N_(Verify that the named commit has a valid GPG signature)),
OPT_CALLBACK('s', strategy, use_strategies, N_(strategy),
N_(merge strategy to use), option_parse_strategy),
OPT_CALLBACK('X', strategy-option, xopts, N_(option=value),
@@ -1233,6 +1235,34 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_merge_usage,
builtin_merge_options);
+ if (verify_signatures) {
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ char hex[41];
+ struct signature_check signature_check;
+ memset(signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, signature_check);
+
+ strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
+ switch(signature_check.result){
+ case 'G':
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
+ default: /* 'N' */
+ die(_(Commit %s does not have a GPG
signature.), hex, hex);
+ }
+ if (verbosity = 0 signature_check.result == 'G')
+ printf(_(Commit %s has a good GPG signature by
%s\n), hex, signature_check.signer);
+
+ free(signature_check.gpg_output);
+ free(signature_check.gpg_status);
+ free(signature_check.signer);
+ free(signature_check.key);
+ }
+ }
+
strbuf_addstr(buf, merge);
for (p = remoteheads; p; p = p-next)
strbuf_addf(buf, %s, merge_remote_util(p-item)-name);
diff --git a/git-pull.sh b/git-pull.sh
index 266e682..705940d 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -39,7 +39,7 @@ test -z $(git ls-files -u) || die_conflict
test -f $GIT_DIR/MERGE_HEAD die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
merge_args= edit=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short=${curr_branch#refs/heads/}
@@ -125,6 +125,12 @@ do
--no-recurse-submodules)
recurse_submodules=--no-recurse-submodules
;;
+ --verify-signatures)
+ verify_signatures=--verify-signatures
+ ;;
+ --no-verify-signatures)
+ verify_signatures=--no-verify-signatures
+ ;;
[PATCH v8 4/5] merge/pull Check for untrusted good GPG signatures
When --verify-signatures is specified, abort the merge in case a good
GPG signature from an untrusted key is encountered.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/merge-options.txt| 4 ++--
builtin/merge.c| 2 ++
commit.c | 14 +-
commit.h | 10 +-
gpg-interface.h| 1 +
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 | 9 +
10 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 31f1067..a0f022b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -85,8 +85,8 @@ option can be used to override --squash.
--verify-signatures::
--no-verify-signatures::
- Verify that the commits being merged have good GPG signatures and abort
the
- merge in case they do not.
+ Verify that the commits being merged have good and trusted GPG
signatures
+ and abort the merge in case they do not.
--summary::
--no-summary::
diff --git a/builtin/merge.c b/builtin/merge.c
index 7a33d03..752e3a9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1248,6 +1248,8 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
switch(signature_check.result){
case 'G':
break;
+ case 'U':
+ die(_(Commit %s has a good, untrusted
GPG signature allegedly by %s.), hex, signature_check.signer);
case 'B':
die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
default: /* 'N' */
diff --git a/commit.c b/commit.c
index 3a8453d..d590724 100644
--- a/commit.c
+++ b/commit.c
@@ -1029,6 +1029,8 @@ static struct {
} sigcheck_gpg_status[] = {
{ 'G', \n[GNUPG:] GOODSIG },
{ 'B', \n[GNUPG:] BADSIG },
+ { 'U', \n[GNUPG:] TRUST_NEVER },
+ { 'U', \n[GNUPG:] TRUST_UNDEFINED },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -1050,11 +1052,13 @@ static void parse_gpg_output(struct signature_check
*sigc)
found += strlen(sigcheck_gpg_status[i].check);
}
sigc-result = sigcheck_gpg_status[i].result;
- sigc-key = xmemdupz(found, 16);
- found += 17;
- next = strchrnul(found, '\n');
- sigc-signer = xmemdupz(found, next - found);
- break;
+ /* The trust messages are not followed by key/signer
information */
+ if (sigc-result != 'U') {
+ sigc-key = xmemdupz(found, 16);
+ found += 17;
+ next = strchrnul(found, '\n');
+ sigc-signer = xmemdupz(found, next - found);
+ }
}
}
diff --git a/commit.h b/commit.h
index 8bbcf13..27d9b36 100644
--- a/commit.h
+++ b/commit.h
@@ -232,11 +232,11 @@ extern void print_commit_list(struct commit_list *list,
const char *format_last);
/*
- * Check the signature of the given commit. The result of the check is stored
in
- * sig-result, 'G' for a good signature, 'B' for a bad signature and 'N'
- * for no signature at all.
- * This may allocate memory for sig-gpg_output, sig-gpg_status, sig-signer
- * and sig-key.
+ * Check the signature of the given commit. The result of the check is stored
+ * in sig-check_result, 'G' for a good signature, 'U' for a good signature
+ * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
+ * at all. This may allocate memory for sig-gpg_output, sig-gpg_status,
+ * sig-signer and sig-key.
*/
extern void check_commit_signature(const struct commit* commit, struct
signature_check *sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index 5884aa4..a85cb5b 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -6,6 +6,7 @@ struct signature_check {
char *gpg_status;
char result; /* 0 (not checked),
* N (checked but no further result),
+ * U (untrusted good),
* G (good)
* B (bad) */
char *signer;
diff --git a/t/lib-gpg/pubring.gpg b/t/lib-gpg/pubring.gpg
index
83855fa4e1c6c37afe550c17afa1e7971042ded5..1a3c2d487c2fda9169751a3068fa51e853a1e519
100644
GIT binary patch
delta 1212
zcmV;t1Vj6b3AYlkj0As~0SyFEOqNFh2mr~8{AU1PG9!Ku9w|}@vpZJPg*#s86v-*O
[PATCH v8 5/5] pretty printing: extend %G? to include 'N' and 'U'
Expand %G? in pretty format strings to 'N' in case of no GPG signature
and 'U' in case of a good but untrusted GPG signature in addition to
the previous 'G'ood and 'B'ad. This eases writing anyting parsing
git-log output.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
Documentation/pretty-formats.txt | 3 ++-
pretty.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 2939655..afac703 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -131,7 +131,8 @@ The placeholders are:
- '%B': raw body (unwrapped subject and body)
- '%N': commit notes
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show either G for Good or B for Bad for a signed commit
+- '%G?': show G for a Good signature, B for a Bad signature, U for a
good,
+ untrusted signature and N for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}`
diff --git a/pretty.c b/pretty.c
index cf84d3a..840c41f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1135,6 +1135,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (c-signature_check.result) {
case 'G':
case 'B':
+ case 'U':
+ case 'N':
strbuf_addch(sb, c-signature_check.result);
}
break;
--
1.8.1.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Thanks for taking the time and effort to review my thoughts. Jens Lehmann wrote: A commit is the thing to record here because it *is* the perfect fit Might be, but saying that doesn't help one bit. I want to know why. I want to improve the user experience of submodules and don't care much in what language we achieve that. You missed the point entirely. If git didn't have a commit object, would you use a special kind of blob and code around everything to avoid fixing a more fundamental issue? What happens when you rename magit to foo in that branch and want to check out an older commit of the same branch? That is one of the reasons why that belongs in to a checked in .gitmodules and not someplace untracked. Good point. I learnt something new. Are you aware that current Git code already stats all files across all submodules recursive by default? So (again) no problem here, we do that already (unless configured otherwise). I didn't know that. Why does it do this? Guess what: submodules are the solution for a certain set of use cases, and tools like subtree are a solution for another set of use cases. There is no silver bullet. That's the core of your argument: submodules already solve what it was meant to, and we can't get it to solve a larger class of problems. In other words, you're implying that it's impossible to build a tool that will be able to compose git repositories in a way that solves a very large class of problems. I don't see conclusive proof for this, so I have to disagree. To summarize, everyone seems to be elated with the current state of submodules and is vehemently defending it. I'm a little unhappy, but am unable to express my discontent in better prose. Let's just go back to writing patches, and come back to this if and when I have a full design. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? Sometimes it's annoyingly verbose to break down a compound function. But I think in this case, you can make your tests more robust by just checking the affirmative that the ref is still where we expect it to be, like: check_push_result up_repo $the_first_commit heads/master Doesn't that change the meaning of the test though? I really like how the original tests read. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic
Junio C Hamano wrote:
sub foo ($) { my ($arg) = @_; print $arg\n; }
sub bar { my ($arg) = @_; print $arg\n; }
my @baz = (100, 101, 102);
foo @baz; # says 3
bar @baz; # says 100
Ouch. Please drop this patch; I'll resubmit when I feel confident
about my change.
This patch fell under the cracks, and reminding me with a what
happened to it? was the right thing to do. Literally, that is what
I ask in the Notes from the maintainer message.
Right. Thanks for clarifying. I'll actively track the patches I submit.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
On Mon, Apr 01, 2013 at 02:21:22AM +0530, Ramkumar Ramachandra wrote: Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? Right. Sometimes it's annoyingly verbose to break down a compound function. But I think in this case, you can make your tests more robust by just checking the affirmative that the ref is still where we expect it to be, like: check_push_result up_repo $the_first_commit heads/master Doesn't that change the meaning of the test though? I really like how the original tests read. Does it? I thought the original was: test_must_fail check_push_result up_repo $the_commit heads/master which is checking that we did _not_ push $the_commit to up_repo. Checking that without a negative means confirming that what _used_ to be there is still there, which is $the_first_commit. But I didn't actually run it, so I might be wrong about what is supposed to be there after the (lack of) push. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] peel_onion: disambiguate to favor tree-ish when we want a tree-ish
The function already knows when interpreting $foo^{commit} to tell
the underlying get_sha1_1() to expect a commit-ish while evaluating
$foo. Teach it to do the same when asked for $foo^{tree}; we are
expecting a tree-ish and $foo should be disambiguated in favor of a
tree-ish, discarding a possible ambiguous match with a blob object.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Junio C Hamano gits...@pobox.com writes:
Perhaps something like this. Note that the last hunk is unrelated
thinko-fix I noticed while browsing the code.
sha1_name.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sha1_name.c b/sha1_name.c
index c50630a..45788df 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -654,6 +654,8 @@ static int peel_onion(const char *name, int len, unsigned
char *sha1)
if (expected_type == OBJ_COMMIT)
lookup_flags = GET_SHA1_COMMITTISH;
+ else if (expected_type == OBJ_TREE)
+ lookup_flags = GET_SHA1_TREEISH;
if (get_sha1_1(name, sp - name - 2, outer, lookup_flags))
return -1;
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] peel_onion(): teach $foo^{object} peeler
A string that names an object can be suffixed with ^{type} peeler to
say I have this object name; peel it until you get this type. If
you cannot do so, it is an error. v1.8.2^{commit} asks for a commit
that is pointed at an annotated tag v1.8.2; v1.8.2^{tree} unwraps it
further to the top-level tree object. A special suffix ^{} (i.e. no
type specified) means I do not care what it unwraps to; just peel
annotated tag until you get something that is not a tag.
When you have a random user-supplied string, you can turn it to a
bare 40-hex object name, and cause it to error out if such an object
does not exist, with:
git rev-parse --verify $userstring^{}
for most objects, but this does not yield the tag object name when
$userstring refers to an annotated tag.
Introduce a new suffix, ^{object}, that only makes sure the given
name refers to an existing object. Then
git rev-parse --verify $userstring^{object}
becomes a way to make sure $userstring refers to an existing object.
This is necessary because the plumbing rev-parse --verify is only
about make sure the argument is something we can feed to get_sha1()
and turn it into a raw 20-byte object name SHA-1 and is not about
make sure that 20-byte object name SHA-1 refers to an object that
exists in our object store. When the given $userstring is already
a 40-hex, by definition rev-parse --verify $userstring can turn it
into a raw 20-byte object name. With $userstring^{object}, we can
make sure that the 40-hex string names an object that exists in our
object store before --verify kicks in.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
sha1_name.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 45788df..85b6e75 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -594,7 +594,7 @@ struct object *peel_to_type(const char *name, int namelen,
while (1) {
if (!o || (!o-parsed !parse_object(o-sha1)))
return NULL;
- if (o-type == expected_type)
+ if (expected_type == OBJ_ANY || o-type == expected_type)
return o;
if (o-type == OBJ_TAG)
o = ((struct tag*) o)-tagged;
@@ -645,6 +645,8 @@ static int peel_onion(const char *name, int len, unsigned
char *sha1)
expected_type = OBJ_TREE;
else if (!strncmp(blob_type, sp, 4) sp[4] == '}')
expected_type = OBJ_BLOB;
+ else if (!prefixcmp(sp, object}))
+ expected_type = OBJ_ANY;
else if (sp[0] == '}')
expected_type = OBJ_NONE;
else if (sp[0] == '/')
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: clarify documentation for the --verify option
Michael Haggerty mhag...@alum.mit.edu writes:
... Though honestly, I don't see the point of using
--default as opposed to
$ git rev-parse --verify ${REV:-master}^{commit}
I would agree ${VAR:-default} is sufficient in that particular case.
The --default is more about the use of the pluming command not with
--verify but as its original use of an argument sifter when
composing git rev-list piped into git diff-tree --stdin, i.e.
git rev-list $(git rev-parse --default HEAD --revs-only $@) |
git diff-tree --stdin $(git rev-parse --no-revs $@)
which was the original way to write commands in the git log family
using the plumbing command as a scripted Porcelain.
--verify::
+ If the parameter can be used as a single object name, output
+ that name; otherwise, emit an error message and exit with a
+ non-zero status. Please note that the existence and validity
+ of the named object itself are not checked.
Perhaps s/used as a single object name/turned into a raw 20-byte SHA-1/;
Because the primary use case of this option is to implement end-user
input validation, I think it would be helpful to clarify use of the
peeler here. Perhaps
--verify::
Make sure the single given parameter can be turned into a
raw 20-byte SHA-1, something that can be used to access the
object database, and emit the SHA-1 in 40-hex format (unless
--symbolic and other formatting option is given); otherwise,
error out.
+
If you want to make sure that the output from this actually
names an object in your object database and/or can be used
as a specific type of object you require, it is a good idea
to add ^{type} peeling operator to the parmeter. For
example, `git rev-parse $VAR^{commit}` will make sure $VAR
names an existing object that is a commit-ish (i.e. a
commit, or an annotated tag that points at a commit). To
make sure that $VAR names an existing object of any type,
you can say `git rev-parse $VAR^{object}`.
or something.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Ramkumar Ramachandra wrote: Jens Lehmann wrote: A commit is the thing to record here because it *is* the perfect fit Might be, but saying that doesn't help one bit. I want to know why. [...] To summarize, everyone seems to be elated with the current state of submodules and is vehemently defending it. I'm a little unhappy, but am unable to express my discontent in better prose. Let's just go back to writing patches, and come back to this if and when I have a full design. Elated is probably not the right word. More annoyed at being told their work is ugly without an accompanying concrete and actionable bug report. :) If you are curious, at a quieter time it might be useful to ask for pointers to the discussions that led to the current design, and folks on the list might be glad to help. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
On Sun, Mar 31, 2013 at 4:34 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Thanks for taking the time and effort to review my thoughts. Jens Lehmann wrote: A commit is the thing to record here because it *is* the perfect fit Might be, but saying that doesn't help one bit. I want to know why. I want to improve the user experience of submodules and don't care much in what language we achieve that. You missed the point entirely. If git didn't have a commit object, would you use a special kind of blob and code around everything to avoid fixing a more fundamental issue? What happens when you rename magit to foo in that branch and want to check out an older commit of the same branch? That is one of the reasons why that belongs in to a checked in .gitmodules and not someplace untracked. Good point. I learnt something new. Are you aware that current Git code already stats all files across all submodules recursive by default? So (again) no problem here, we do that already (unless configured otherwise). I didn't know that. Why does it do this? Guess what: submodules are the solution for a certain set of use cases, and tools like subtree are a solution for another set of use cases. There is no silver bullet. That's the core of your argument: submodules already solve what it was meant to, and we can't get it to solve a larger class of problems. In other words, you're implying that it's impossible to build a tool that will be able to compose git repositories in a way that solves a very large class of problems. I don't see conclusive proof for this, so I have to disagree. I think it is possible to solve larger classes of problems with submodules, but it is a harder problem than you seem to think. In any case I do not think you need to re-engineer submodules to improve them. Sumodules are good for preserving history. When properly managed, they answer the question git always answers, Where was my code in the past? I would like proper management to be easier, but I understand why it is difficult; and I see it getting easier. Some users also want submodules to handle other tasks, like Import branch-tracked upstream changes (i.e. git pull origin master). This too is useful, but it is a different problem than submodules' primarily try to solve. But they do already solve _part_ of that problem (Show me how these modules are related), so it seems a trivial thing to ask them also to handle the floating branch task. The trick is to handle this task in a way that does not break the task they are designed and used for already. Some other users want submodules to solve the problem of composition, like Show me the combined log of all these submodules. (Replace show log with diff, merge, bisect or even rebase if you like.) I think this is where Jens is leaning when working to improve the user experience. But this direction does not require re-architecting the fundamentals of submodules. To summarize, everyone seems to be elated with the current state of submodules and is vehemently defending it. That's a gross overstatement. Everyone understands that is complicated and dangerous tweak a machine in operation. Such tweaks should be safe, prudent and justifiable, in roughly that order. Phil -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
In message CALkWK0=csuawqwk5guf0pbc4_zeoziwqpamcrvbgz5lj0qg...@mail.gmail.com, Ramkumar Ramachandra writes: As a user inexperienced with recursive submodules (I've only used them in this repository), I found it highly confusing. Thanks for clearing them up. You may want to investigate third party alternatives like gitslave http://gitslave.sf.net Depending on what problem you are trying to solve, it can be better (or worse) to use compared to submodules. It provides a usually conceptually easier method to group subprojects together into a superproject. You can replace practically any git command you want with gits and it will usually work as you might more or less expect. Conceptually it has a list of git repositories and will execute the listed git command on each in turn. It seems to resolve most of the issues that you raise, but of course it has some warts of its own. Some could be resolved with sufficient effort, others are fundamental. (An example of the latter, you cannot trivially tell what commit in other repositories a particular user was at when he made a commit in a specific repository (absent a gits tag being created). An example of the former, if you have git output paging turned on and many subprojects to check out, `gits clone` pages the output and more to the point, blocks the clones until you page through the output which you must typically do many times). -Seth Robertson -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
sub foo ($) { my ($arg) = @_; print $arg\n; }
sub bar { my ($arg) = @_; print $arg\n; }
my @baz = (100, 101, 102);
foo @baz; # says 3
bar @baz; # says 100
Ouch. Please drop this patch; I'll resubmit when I feel confident
about my change.
No, let's not do that. I will forget and end up spending time to
read the same patch again.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] send-email: use return; not return undef; on error codepaths
From: Ramkumar Ramachandra artag...@gmail.com
All the callers of ask, extract_valid_address, and validate_patch
subroutines assign the return values from them to a single scaler:
$var = subr(...);
and return undef; in these subroutine can safely be turned into a
simpler return;. Doing so will also future-proof a new caller that
mistakenly does this:
@foo = ask(...);
if (@foo) { ... we got an answer ... } else { ... we did not ... }
Note that we leave return undef; in validate_address on purpose,
even though Perlcritic may complain. The primary return site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other return undef; to return.
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-send-email.perl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..79cc5be 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -711,7 +711,7 @@ sub ask {
}
}
}
- return undef;
+ return;
}
my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
# less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency
return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
- return undef;
+ return;
}
sub extract_valid_address_or_die {
@@ -1484,7 +1484,7 @@ sub validate_patch {
return $.: patch contains a line longer than 998
characters;
}
}
- return undef;
+ return;
}
sub file_has_nonascii {
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] send-email: drop misleading function prototype
From: Ramkumar Ramachandra artag...@gmail.com
The subroutine check_file_rev_conflict() is called from two places,
both of which expects to pass a single scaler variable and see if
that can be interpreted as a pathname or a revision name. It is
defined with a function prototype ($) to force a scaler context
while evaluating the arguments at the calling site but it does not
help the current calling sites. The only effect it has is to hurt
future calling sites that may want to build an argument list in an
array variable and call it as check_file_rev_confict(@args).
Drop the misleading prototype, as Perlcritic suggests.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 79cc5be..86dd593 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -513,7 +513,7 @@ sub split_addrs {
($sender) = expand_aliases($sender) if defined $sender;
# returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+sub check_file_rev_conflict {
return unless $repo;
my $f = shift;
try {
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd
From: Ramkumar Ramachandra artag...@gmail.com
Perlcritic does not want to see the trailing pipe in the two-args
form of open(), i.e.
open my $fh, $cmd \Q$file\E |;
If $cmd were a single-token command name, it would make a lot more
sense to use four-or-more-args form open FILEHANDLE,MODE,CMD,ARGS
to avoid shell from expanding metacharacters in $file, but we do
expect multi-word string in $to_cmd and $cc_cmd to be expanded by
the shell, so we cannot rewrite it to
open my $fh, -|, $cmd, $file;
for extra safety. At least, by using this in the three-arg form:
open my $fh, -|, $cmd \Q$file\E;
we can silence Perlcritique, even though we do not gain much safety
by doing so.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-send-email.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 86dd593..fb92227 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1438,7 +1438,7 @@ sub recipients_cmd {
my $sanitized_sender = sanitize_address($sender);
my @addresses = ();
- open my $fh, $cmd \Q$file\E |
+ open my $fh, -|, $cmd \Q$file\E
or die ($prefix) Could not execute '$cmd';
while (my $address = $fh) {
$address =~ s/^\s*//g;
--
1.8.2-441-g6e6f07b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Jeff King p...@peff.net writes: On Mon, Apr 01, 2013 at 02:21:22AM +0530, Ramkumar Ramachandra wrote: Jeff King wrote: [...] So, you're saying: don't test compound statements for failure, since anything in the chain could fail and propagate failure. I should only test simple git-foo commands for failure? Right. I think another thing to keep in mind is that test_must_fail is used only to check for an expected failure from git-foo commands, not for a random command (or sequence of commands). The primary point of test_must_fail is not to declare _any_ failure as Oh, good, I wanted see it to fail and it returned non-zero exit status so I am happy, but exclude uncontrolled failures, by saying Yuck, it returned non-zero exit status, but it died by signal (e.g. SEGV), which is not the way I wanted to see it die. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. The only reason a topic is queued in 'pu' is to let me not pay attention to it, without risking to forget about it completely ;-). The topics on 'pu' have potential to be a useful change even though they are far from ready for 'next'. By queuing on 'pu', rather than just letting them sit in the mail archive and relying on the author to send follow-ups, I can send out what happened to this topic? inquiries from time to time, without paying constant attention to them. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] send-email: drop misleading function prototype
Junio C Hamano wrote:
From: Ramkumar Ramachandra artag...@gmail.com
The subroutine check_file_rev_conflict() is called from two places,
both of which expects to pass a single scaler variable and see if
that can be interpreted as a pathname or a revision name. It is
defined with a function prototype ($) to force a scaler context
while evaluating the arguments at the calling site but it does not
help the current calling sites. The only effect it has is to hurt
future calling sites that may want to build an argument list in an
array variable and call it as check_file_rev_confict(@args).
Drop the misleading prototype, as Perlcritic suggests.
Nice explanation. Here's a similar patch I wrote before I noticed
your patch, with commit message stolen from the above.
-- 8 --
Subject: send-email: drop misleading function prototype
The subroutine check_file_rev_conflict() is called from two places,
both of which expects to pass a single scaler variable and see if
that can be interpreted as a pathname or a revision name. It is
defined with a function prototype ($) to force a scaler context
while evaluating the arguments at the calling site but it does not
help the current calling sites. The only effect it has is to hurt
future calling sites that may want to build an argument list in an
array variable and call it as check_file_rev_confict(@args).
Drop the misleading prototype, as Perlcritic suggests.
While at it, rename the function to avoid new call sites unaware of
this change arising and add a comment clarifying what this function
is for.
Reported-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
git-send-email.perl | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index c3501d98..d6b3c32b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -512,8 +512,9 @@ sub split_addrs {
($sender) = expand_aliases($sender) if defined $sender;
-# returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+# is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if
+# $f is a revision list specification to be passed to format-patch.
+sub is_format_patch_arg {
return unless $repo;
my $f = shift;
try {
@@ -529,6 +530,7 @@ ($)
* Giving --format-patch option if you mean a range.
EOF
} catch Git::Error::Command with {
+ # Not a valid revision. Treat it as a filename.
return 0;
}
}
@@ -540,14 +542,14 @@ ($)
if ($f eq --) {
push @rev_list_opts, --, @ARGV;
@ARGV = ();
- } elsif (-d $f and !check_file_rev_conflict($f)) {
+ } elsif (-d $f and !is_format_patch_arg($f)) {
opendir my $dh, $f
or die Failed to opendir $f: $!;
push @files, grep { -f $_ } map { catfile($f, $_) }
sort readdir $dh;
closedir $dh;
- } elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) {
+ } elsif ((-f $f or -p $f) and !is_format_patch_arg($f)) {
push @files, $f;
} else {
push @rev_list_opts, $f;
--
1.8.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] send-email: use the three-arg form of open in recipients_cmd
Junio C Hamano wrote: we cannot rewrite it to open my $fh, -|, $cmd, $file; for extra safety. At least, by using this in the three-arg form: open my $fh, -|, $cmd \Q$file\E; we can silence Perlcritique, even though we do not gain much safety by doing so. Yeah, I think this is the right thing to do. This means that if some later code refactoring parses $tocmd once and passes an array around, it would be easy to change this to open my $fh, -|, @$cmd, $file; and there would be no temptation to do something involving pasting @$cmd back together into a single string. Of course such a refactoring is not very likely, but that kind of thing is a good reason to prefer this style in general. So for what it's worth, I like this patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/5] merge/pull: verify GPG signatures of commits being merged
Sebastian Götte ja...@physik.tu-berlin.de writes:
+ if (verify_signatures) {
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ char hex[41];
+ struct signature_check signature_check;
+ memset(signature_check, 0, sizeof(signature_check));
+
+ check_commit_signature(commit, signature_check);
+
+ strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
+ switch(signature_check.result){
+ case 'G':
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
+ default: /* 'N' */
+ die(_(Commit %s does not have a GPG
signature.), hex, hex);
Count %s and number and arguments.
+ }
Style.
switch (expr) {
case 'G':
do_something_for_G();
break;
...
}
Also avoid overlong lines.
I'll squash in something like the following and push out the result
on 'pu' tonight. Please check to see if I made silly mistakes while
doing so.
Thanks.
builtin/merge.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 7a33d03..e57c42c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1245,16 +1245,18 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
check_commit_signature(commit, signature_check);
strcpy(hex, find_unique_abbrev(commit-object.sha1,
DEFAULT_ABBREV));
- switch(signature_check.result){
- case 'G':
- break;
- case 'B':
- die(_(Commit %s has a bad GPG
signature allegedly by %s.), hex, signature_check.signer);
- default: /* 'N' */
- die(_(Commit %s does not have a GPG
signature.), hex, hex);
+ switch (signature_check.result) {
+ case 'G':
+ break;
+ case 'B':
+ die(_(Commit %s has a bad GPG signature
+ allegedly by %s.), hex,
signature_check.signer);
+ default: /* 'N' */
+ die(_(Commit %s does not have a GPG
signature.), hex);
}
if (verbosity = 0 signature_check.result == 'G')
- printf(_(Commit %s has a good GPG signature by
%s\n), hex, signature_check.signer);
+ printf(_(Commit %s has a good GPG signature by
%s\n),
+ hex, signature_check.signer);
free(signature_check.gpg_output);
free(signature_check.gpg_status);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fixup! pathspec: support :(glob) syntax
John Keeping j...@keeping.me.uk writes: A formatting fix for a patch currently cooking on nd/magic-pathspecs (cc3d8045ec1e2323c5654e2af834e887f26deb7e). --- The latest version of this wasn't posted to the list in full, so I'm not sure about the recommended way to provide feedback. Hopefully this is easy to squash in. This is one of the reasons why I do not like to directly pull from contributors. Checking by others from the sidelines what was requested to be pulled like you did is greatly appreciated. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bash: teach __git_ps1 about REVERT_HEAD
Robin Rosenberg robin.rosenb...@dewire.com writes: Signed-off-by: Robin Rosenberg robin.rosenb...@dewire.com --- contrib/completion/git-prompt.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 341422a..756a951 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -282,6 +282,8 @@ __git_ps1 () r=|MERGING elif [ -f $g/CHERRY_PICK_HEAD ]; then r=|CHERRY-PICKING + elif [ -f $g/REVERT_HEAD ]; then + r=|REVERTING Obviously a good thing to do; thanks. elif [ -f $g/BISECT_LOG ]; then r=|BISECTING fi -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] branch: give better message when no names specified for rename
Jonathon Mah m...@jonathonmah.com writes:
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
The previous message was incorrect when not enough arguments were
specified:
$ git branch -m
fatal: too many branches for a rename operation
I changed to branch name required instead of new branch name required in
the hope that existing translations can be used.
builtin/branch.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 00d17d2..580107f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -880,7 +880,9 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
if (edit_branch_description(branch_name))
return 1;
} else if (rename) {
- if (argc == 1)
+ if (!argc)
+ die(_(branch name required));
+ else if (argc == 1)
rename_branch(head, argv[0], rename 1);
else if (argc == 2)
rename_branch(argv[0], argv[1], rename 1);
Obviously a good thing to do; thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] branch: give better message when no names specified for rename
Junio C Hamano gits...@pobox.com writes:
Jonathon Mah m...@jonathonmah.com writes:
Signed-off-by: Jonathon Mah m...@jonathonmah.com
---
The previous message was incorrect when not enough arguments were
specified:
$ git branch -m
fatal: too many branches for a rename operation
I changed to branch name required instead of new branch name required in
the hope that existing translations can be used.
builtin/branch.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 00d17d2..580107f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -880,7 +880,9 @@ int cmd_branch(int argc, const char **argv, const char
*prefix)
if (edit_branch_description(branch_name))
return 1;
} else if (rename) {
-if (argc == 1)
+if (!argc)
+die(_(branch name required));
+else if (argc == 1)
rename_branch(head, argv[0], rename 1);
else if (argc == 2)
rename_branch(argv[0], argv[1], rename 1);
Obviously a good thing to do; thanks.
Next time, please run the testsuite before sending a patch. I've
fixed t3200 locally when queuing this patch, so no need to resend
this one.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fixup! pathspec: support :(glob) syntax
On Mon, Apr 1, 2013 at 9:51 AM, Junio C Hamano gits...@pobox.com wrote: John Keeping j...@keeping.me.uk writes: A formatting fix for a patch currently cooking on nd/magic-pathspecs (cc3d8045ec1e2323c5654e2af834e887f26deb7e). --- The latest version of this wasn't posted to the list in full, so I'm not sure about the recommended way to provide feedback. Hopefully this is easy to squash in. This is one of the reasons why I do not like to directly pull from contributors. Checking by others from the sidelines what was requested to be pulled like you did is greatly appreciated. This bug exists in v1 [1] as well. Do you want me to resend or you will squash it in yourself? [1] http://article.gmane.org/gmane.comp.version-control.git/218231 -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: The only reason a topic is queued in 'pu' is to let me not pay attention to it, without risking to forget about it completely ;-). The topics on 'pu' have potential to be a useful change even though they are far from ready for 'next'. That's not even though but should have been even when. Sometimes when I feel a topic is already of 'next' quality, the author suggests that a reroll is coming, and I hold such a topic off. Also, inverse is not true, as always. Some topics may not be queued in 'pu' when I push a day's integration results out, but that does not mean they do not have potential to be useful. They may not be in 'pu' because I didn't have enough time to get around to them, or because there was discussion ongoing and I saw the author was actively responding to the comments, relieving me from having to keep an eye on the topic at all until the dust settled without even queueing it on 'pu'. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] send-email: use return; not return undef; on error codepaths
On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano gits...@pobox.com wrote:
All the callers of ask, extract_valid_address, and validate_patch
subroutines assign the return values from them to a single scaler:
s/scaler/scalar/g
(note the /g)
$var = subr(...);
and return undef; in these subroutine can safely be turned into a
simpler return;. Doing so will also future-proof a new caller that
mistakenly does this:
@foo = ask(...);
if (@foo) { ... we got an answer ... } else { ... we did not ... }
Note that we leave return undef; in validate_address on purpose,
even though Perlcritic may complain. The primary return site of
the function returns whatever is in the scaler variable $address, so
it is pointless to change only the other return undef; to return.
The caller must be prepared to see an array with a single undef as
the return value from this subroutine anyway.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] send-email: drop misleading function prototype
On Sun, Mar 31, 2013 at 9:40 PM, Junio C Hamano gits...@pobox.com wrote: The subroutine check_file_rev_conflict() is called from two places, both of which expects to pass a single scaler variable and see if s/scaler/scalar/g (note the /g) that can be interpreted as a pathname or a revision name. It is defined with a function prototype ($) to force a scaler context while evaluating the arguments at the calling site but it does not help the current calling sites. The only effect it has is to hurt future calling sites that may want to build an argument list in an array variable and call it as check_file_rev_confict(@args). Drop the misleading prototype, as Perlcritic suggests. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] checkout: add --no-widen for restoring files in sparse checkout mode
Hi, Nguyễn Thái Ngọc Duy wrote: git checkout -- paths is usually used to restore all modified files in paths. In sparse checkout mode, this command is overloaded with another meaning: to add back all files in paths that are excluded by sparse patterns. Add --no-widen option to do what normal mode does: restore all modified files and nothing else. In an ideal world, I would like git checkout --widen to modify the .git/info/sparse-checkout file, to be able to do: git clone --sparse-checkout=Documentation git://repo.or.cz/git.git cd git git checkout --widen -- README COPYING INSTALL and hack on a tree with Documentation/, README, COPYING, and INSTALL present with no actual code to distract. And git checkout --no-widen could be a way to ask to respect the existing sparse pattern. This patch isn't about tweaking the sparse-checkout pattern; instead, it's about how git checkout interacts with the skip-worktree bit. Maybe a good name would be --respect-skip-worktree? [...] --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -180,6 +180,17 @@ branch by running git rm -rf . from the top level of the working tree. Afterwards you will be ready to prepare your new files, repopulating the working tree, by copying them from elsewhere, extracting a tarball, etc. +--no-widen:: + In sparse checkout mode, `git checkout -- paths` would + update all entries matched by paths regardless of sparse + patterns. This option only updates entries matched by paths + and sparse patterns. + +--widen:: + Revert the effect of `--no-widen` if specified and make + `git checkout -- paths` update all entries matched by + paths regardless of sparse patterns. Perhaps, combining the descriptions of the positive and negative forms: --respect-skip-worktree:: By default, `git checkout -- paths` creates or updates files matching paths regardless of the skip-worktree bit. This option makes 'git checkout' skips entries with the skip-worktree bit set, which can be useful in sparse checkout mode. I'm afraid I can't imagine when --no-respect-skip-worktree would be useful. That can easily be a failure of my imagination, though. What do you think? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. By the way, this series seems to break a few tests in the test suite, both as a standalone topic branch and as part of the 'pu'. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] checkout: add --no-widen for restoring files in sparse checkout mode
Jonathan Nieder jrnie...@gmail.com writes: Nguyễn Thái Ngọc Duy wrote: git checkout -- paths is usually used to restore all modified files in paths. In sparse checkout mode, this command is overloaded with another meaning: to add back all files in paths that are excluded by sparse patterns. Add --no-widen option to do what normal mode does: restore all modified files and nothing else. In an ideal world, I would like git checkout --widen to modify the .git/info/sparse-checkout file, to be able to do: git clone --sparse-checkout=Documentation git://repo.or.cz/git.git cd git git checkout --widen -- README COPYING INSTALL and hack on a tree with Documentation/, README, COPYING, and INSTALL present with no actual code to distract. And git checkout --no-widen could be a way to ask to respect the existing sparse pattern. Yeah, I think the above makes tons of sense, and --widen would be an ideal name for that optional behaviour. When you are limited by your original sparse pathspecs, that would be a way to explicitly widen the paths you interact with. In that sense, making it off by default would be a sensible thing to do. When you limited yourself to a subset of dir/, you do not want git checkout dir/ to automatically widen it by accident. This patch isn't about tweaking the sparse-checkout pattern; instead, it's about how git checkout interacts with the skip-worktree bit. Maybe a good name would be --respect-skip-worktree? Yeah, but when would one want to say --no-respect without widening the sparce pathspecs? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] checkout: add --no-widen for restoring files in sparse checkout mode
On Mon, Apr 1, 2013 at 11:48 AM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Nguyễn Thái Ngọc Duy wrote: git checkout -- paths is usually used to restore all modified files in paths. In sparse checkout mode, this command is overloaded with another meaning: to add back all files in paths that are excluded by sparse patterns. Add --no-widen option to do what normal mode does: restore all modified files and nothing else. In an ideal world, I would like git checkout --widen to modify the .git/info/sparse-checkout file, to be able to do: git clone --sparse-checkout=Documentation git://repo.or.cz/git.git cd git git checkout --widen -- README COPYING INSTALL and hack on a tree with Documentation/, README, COPYING, and INSTALL present with no actual code to distract. And git checkout --no-widen could be a way to ask to respect the existing sparse pattern. In an ideal world, I would spend more time on this and add --edit-sparse, which opens up $EDITOR, lets you edit the patterns and reapplies the patterns after $EDITOR exits (catching faults if possible). Unfortunately I don't use sparse checkout as much as before and therefore have little motivation to do it. I would really like narrow clone to replace sparse checkout, but I haven't made much progress on that front either. I'll try to get back on that once pathspec-magics topic is settled. This patch isn't about tweaking the sparse-checkout pattern; instead, it's about how git checkout interacts with the skip-worktree bit. Maybe a good name would be --respect-skip-worktree? I'm bad at naming. If nobody objects, I'll take this as the new option name. --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -180,6 +180,17 @@ branch by running git rm -rf . from the top level of the working tree. Afterwards you will be ready to prepare your new files, repopulating the working tree, by copying them from elsewhere, extracting a tarball, etc. +--no-widen:: + In sparse checkout mode, `git checkout -- paths` would + update all entries matched by paths regardless of sparse + patterns. This option only updates entries matched by paths + and sparse patterns. + +--widen:: + Revert the effect of `--no-widen` if specified and make + `git checkout -- paths` update all entries matched by + paths regardless of sparse patterns. Perhaps, combining the descriptions of the positive and negative forms: --respect-skip-worktree:: By default, `git checkout -- paths` creates or updates files matching paths regardless of the skip-worktree bit. This option makes 'git checkout' skips entries with the skip-worktree bit set, which can be useful in sparse checkout mode. OK I'm afraid I can't imagine when --no-respect-skip-worktree would be useful. That can easily be a failure of my imagination, though. There may be scripts that expect git checkout -- foo to reset everything in foo. Or maybe you just want to check out a single file and do not bother to edit sparse patterns as you won't need it for long. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Jeff King p...@peff.net writes: [...] Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. I noticed that you queued my original series without modification in rr/triangle. Should I submit a re-roll with Peff's suggestion incorporated? If you want the topic to make progress, yes. By the way, this series seems to break a few tests in the test suite,... I suspect this could be interaction with push-default change near the tip of 'pu'. Setting push.default explicitly to matching in the test may be necessary. Also the t5516 is involved in in-flight churns, so there could be some merge mixups. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
