* [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
We store the hex prefix in a 40-byte buffer with the prefix
itself followed by 40-minus-len "x" characters. These x's
serve no purpose, and the lack of NUL termination makes the
prefix string annoying to use. Let's just terminate it.
Note that this is in contrast to the binary prefix, which
_must_ be zero-padded, because we look at the whole thing
during a binary search to find the first potential match in
each pack index. The loose-object hex search cannot use the
same trick because it has to do a linear walk through the
unsorted results of readdir() (and even if it could, you'd
want zeroes instead of x's).
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 79eb1ee..549ef3f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -14,7 +14,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
int len; /* length of prefix in hex chars */
- char hex_pfx[GIT_SHA1_HEXSZ];
+ char hex_pfx[GIT_SHA1_HEXSZ + 1];
unsigned char bin_pfx[GIT_SHA1_RAWSZ];
disambiguate_hint_fn fn;
@@ -291,7 +291,6 @@ static int init_object_disambiguation(const char *name, int len,
return -1;
memset(ds, 0, sizeof(*ds));
- memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);
for (i = 0; i < len ;i++) {
unsigned char c = name[i];
@@ -313,6 +312,7 @@ static int init_object_disambiguation(const char *name, int len,
}
ds->len = len;
+ ds->hex_pfx[len] = '\0';
prepare_alt_odb();
return 0;
}
@@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
status = finish_object_disambiguation(&ds, sha1);
if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error("short SHA1 %.*s is ambiguous.", ds.len, ds.hex_pfx);
+ return error("short SHA1 %s is ambiguous.", ds.hex_pfx);
return status;
}
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 07/10] get_short_sha1: mark ambiguity error for translation
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
This is a human-readable message, and there's no reason it
should not be translated. While we're at it, let's drop the
period from the end, which is not our usual style.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 549ef3f..d4c7e26 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
status = finish_object_disambiguation(&ds, sha1);
if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error("short SHA1 %s is ambiguous.", ds.hex_pfx);
+ return error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
return status;
}
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 08/10] sha1_array: let callbacks interrupt iteration
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The callbacks for iterating a sha1_array must have a void
return. This is unlike our usual for_each semantics, where
a callback may interrupt iteration and have its value
propagated. Let's switch it to the usual form, which will
enable its use in more places (e.g., where we are replacing
an existing iteration with a different data structure).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-sha1-array.txt | 8 ++++++--
builtin/cat-file.c | 3 ++-
builtin/receive-pack.c | 3 ++-
sha1-array.c | 8 ++++++--
sha1-array.h | 8 ++++----
submodule.c | 3 ++-
t/helper/test-sha1-array.c | 3 ++-
7 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/Documentation/technical/api-sha1-array.txt b/Documentation/technical/api-sha1-array.txt
index 3e75497..dcc5294 100644
--- a/Documentation/technical/api-sha1-array.txt
+++ b/Documentation/technical/api-sha1-array.txt
@@ -38,16 +38,20 @@ Functions
`sha1_array_for_each_unique`::
Efficiently iterate over each unique element of the list,
executing the callback function for each one. If the array is
- not sorted, this function has the side effect of sorting it.
+ not sorted, this function has the side effect of sorting it. If
+ the callback returns a non-zero value, the iteration ends
+ immediately and the callback's return is propagated; otherwise,
+ 0 is returned.
Examples
--------
-----------------------------------------
-void print_callback(const unsigned char sha1[20],
+int print_callback(const unsigned char sha1[20],
void *data)
{
printf("%s\n", sha1_to_hex(sha1));
+ return 0; /* always continue */
}
void some_func(void)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 94e67eb..cca97a8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -401,11 +401,12 @@ struct object_cb_data {
struct expand_data *expand;
};
-static void batch_object_cb(const unsigned char sha1[20], void *vdata)
+static int batch_object_cb(const unsigned char sha1[20], void *vdata)
{
struct object_cb_data *data = vdata;
hashcpy(data->expand->oid.hash, sha1);
batch_object_write(NULL, data->opt, data->expand);
+ return 0;
}
static int batch_loose_object(const unsigned char *sha1,
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 896b16f..f7cd180 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -268,9 +268,10 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
return 0;
}
-static void show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
+static int show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
{
show_ref(".have", sha1);
+ return 0;
}
static void collect_one_alternate_ref(const struct ref *ref, void *data)
diff --git a/sha1-array.c b/sha1-array.c
index 6f4a224..af1d7d5 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -42,7 +42,7 @@ void sha1_array_clear(struct sha1_array *array)
array->sorted = 0;
}
-void sha1_array_for_each_unique(struct sha1_array *array,
+int sha1_array_for_each_unique(struct sha1_array *array,
for_each_sha1_fn fn,
void *data)
{
@@ -52,8 +52,12 @@ void sha1_array_for_each_unique(struct sha1_array *array,
sha1_array_sort(array);
for (i = 0; i < array->nr; i++) {
+ int ret;
if (i > 0 && !hashcmp(array->sha1[i], array->sha1[i-1]))
continue;
- fn(array->sha1[i], data);
+ ret = fn(array->sha1[i], data);
+ if (ret)
+ return ret;
}
+ return 0;
}
diff --git a/sha1-array.h b/sha1-array.h
index 72bb33b..b3230be 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -14,10 +14,10 @@ void sha1_array_append(struct sha1_array *array, const unsigned char *sha1);
int sha1_array_lookup(struct sha1_array *array, const unsigned char *sha1);
void sha1_array_clear(struct sha1_array *array);
-typedef void (*for_each_sha1_fn)(const unsigned char sha1[20],
- void *data);
-void sha1_array_for_each_unique(struct sha1_array *array,
- for_each_sha1_fn fn,
+typedef int (*for_each_sha1_fn)(const unsigned char sha1[20],
void *data);
+int sha1_array_for_each_unique(struct sha1_array *array,
+ for_each_sha1_fn fn,
+ void *data);
#endif /* SHA1_ARRAY_H */
diff --git a/submodule.c b/submodule.c
index 0ef2ff4..aba94dd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -728,9 +728,10 @@ void check_for_new_submodule_commits(unsigned char new_sha1[20])
sha1_array_append(&ref_tips_after_fetch, new_sha1);
}
-static void add_sha1_to_argv(const unsigned char sha1[20], void *data)
+static int add_sha1_to_argv(const unsigned char sha1[20], void *data)
{
argv_array_push(data, sha1_to_hex(sha1));
+ return 0;
}
static void calculate_changed_submodule_paths(void)
diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c
index 09f7790..f7a53c4 100644
--- a/t/helper/test-sha1-array.c
+++ b/t/helper/test-sha1-array.c
@@ -1,9 +1,10 @@
#include "cache.h"
#include "sha1-array.h"
-static void print_sha1(const unsigned char sha1[20], void *data)
+static int print_sha1(const unsigned char sha1[20], void *data)
{
puts(sha1_to_hex(sha1));
+ return 0;
}
int cmd_main(int argc, const char **argv)
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 09/10] for_each_abbrev: drop duplicate objects
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
If an object appears multiple times in the object database
(e.g., in both loose and packed form, or in two separate
packs), the disambiguation machinery may see it more than
once. The get_short_sha1() function handles this already,
but for_each_abbrev() blindly fires the callback for each
instance it finds.
We can fix this by collecting the output in a sha1 array and
de-duplicating it. As a bonus, the sort done for the
de-duplication means that our output will be stable,
regardless of the order in which the objects are found.
Note that the old code normalized the callback's output to
0/1 to store in the 1-bit ds->ambiguous flag (which both
halted the iteration and was returned from the
for_each_abbrev function). Now that we are using sha1_array,
we can return the real value. In practice, it doesn't matter
as the sole caller only ever returns 0.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 19 +++++++++++++++----
t/t1512-rev-parse-disambiguation.sh | 7 +++++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index d4c7e26..f7403d7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -7,6 +7,7 @@
#include "refs.h"
#include "remote.h"
#include "dir.h"
+#include "sha1-array.h"
static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
@@ -355,20 +356,30 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
return status;
}
+static int collect_ambiguous(const unsigned char *sha1, void *data)
+{
+ sha1_array_append(data, sha1);
+ return 0;
+}
+
int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
{
+ struct sha1_array collect = SHA1_ARRAY_INIT;
struct disambiguate_state ds;
+ int ret;
if (init_object_disambiguation(prefix, strlen(prefix), &ds) < 0)
return -1;
ds.always_call_fn = 1;
- ds.cb_data = cb_data;
- ds.fn = fn;
-
+ ds.fn = collect_ambiguous;
+ ds.cb_data = &collect;
find_short_object_filename(&ds);
find_short_packed_object(&ds);
- return ds.ambiguous;
+
+ ret = sha1_array_for_each_unique(&collect, fn, cb_data);
+ sha1_array_clear(&collect);
+ return ret;
}
int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index dfd3567..1d8f550 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -280,6 +280,13 @@ test_expect_success 'rev-parse --disambiguate' '
test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
'
+test_expect_success 'rev-parse --disambiguate drops duplicates' '
+ git rev-parse --disambiguate=000000000 >expect &&
+ git pack-objects .git/objects/pack/pack <expect &&
+ git rev-parse --disambiguate=000000000 >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'ambiguous 40-hex ref' '
TREE=$(git mktree </dev/null) &&
REF=$(git rev-parse HEAD) &&
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
When the user gives us an ambiguous short sha1, we print an
error and refuse to resolve it. In some cases, the next step
is for them to feed us more characters (e.g., if they were
retyping or cut-and-pasting from a full sha1). But in other
cases, that might be all they have. For example, an old
commit message may have used a 7-character hex that was
unique at the time, but is now ambiguous. Git doesn't
provide any information about the ambiguous objects it
found, so it's hard for the user to find out which one they
probably meant.
This patch teaches get_short_sha1() to list the sha1s of the
objects it found, along with a few bits of information that
may help the user decide which one they meant. Here's what
it looks like on git.git:
$ git rev-parse b2e1
error: short SHA1 b2e1 is ambiguous
hint: The candidates are:
hint: b2e1196 tag v2.8.0-rc1
hint: b2e11d1 tree
hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
hint: b2e1759 blob
hint: b2e18954 blob
hint: b2e1895c blob
fatal: ambiguous argument 'b2e1': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
We show the tagname for tags, and the date and subject for
commits. For trees and blobs, in theory we could dig in the
history to find the paths at which they were present. But
that's very expensive (on the order of 30s for the kernel),
and it's not likely to be all that helpful. Most short
references are to commits, so the useful information is
typically going to be that the object in question _isn't_ a
commit. So it's silly to spend a lot of CPU preemptively
digging up the path; the user can do it themselves if they
really need to.
And of course it's somewhat ironic that we abbreviate the
sha1s in the disambiguation hint. But full sha1s would cause
annoying line wrapping for the commit lines, and presumably
the user is going to just re-issue their command immediately
with the corrected sha1.
We also restrict the list to those that match any
disambiguation hint. E.g.:
$ git rev-parse b2e1:foo
error: short SHA1 b2e1 is ambiguous
hint: The candidates are:
hint: b2e1196 tag v2.8.0-rc1
hint: b2e11d1 tree
hint: b2e1632 commit 2007-11-14 - Merge branch 'bs/maint-commit-options'
fatal: Invalid object name 'b2e1'.
does not bother reporting the blobs, because they cannot
work as a treeish.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 50 +++++++++++++++++++++++++++++++++++--
t/t1512-rev-parse-disambiguation.sh | 24 ++++++++++++++++++
2 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index f7403d7..35d943d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -318,6 +318,38 @@ static int init_object_disambiguation(const char *name, int len,
return 0;
}
+static int show_ambiguous_object(const unsigned char *sha1, void *data)
+{
+ const struct disambiguate_state *ds = data;
+ struct strbuf desc = STRBUF_INIT;
+ int type;
+
+ if (ds->fn && !ds->fn(sha1, ds->cb_data))
+ return 0;
+
+ type = sha1_object_info(sha1, NULL);
+ if (type == OBJ_COMMIT) {
+ struct commit *commit = lookup_commit(sha1);
+ if (commit) {
+ struct pretty_print_context pp = {0};
+ pp.date_mode.type = DATE_SHORT;
+ format_commit_message(commit, " %ad - %s", &desc, &pp);
+ }
+ } else if (type == OBJ_TAG) {
+ struct tag *tag = lookup_tag(sha1);
+ if (!parse_tag(tag) && tag->tag)
+ strbuf_addf(&desc, " %s", tag->tag);
+ }
+
+ advise(" %s %s%s",
+ find_unique_abbrev(sha1, DEFAULT_ABBREV),
+ typename(type) ? typename(type) : "unknown type",
+ desc.buf);
+
+ strbuf_release(&desc);
+ return 0;
+}
+
static int multiple_bits_set(unsigned flags)
{
return !!(flags & (flags - 1));
@@ -351,8 +383,22 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
find_short_packed_object(&ds);
status = finish_object_disambiguation(&ds, sha1);
- if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
+ if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
+ error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
+
+ /*
+ * We may still have ambiguity if we simply saw a series of
+ * candidates that did not satisfy our hint function. In
+ * that case, we still want to show them, so disable the hint
+ * function entirely.
+ */
+ if (!ds.ambiguous)
+ ds.fn = NULL;
+
+ advise(_("The candidates are:"));
+ for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
+ }
+
return status;
}
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 1d8f550..c5447ef 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -323,4 +323,28 @@ test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (peel)' '
test_line_count = 1 errors
'
+test_expect_success C_LOCALE_OUTPUT 'ambiguity hints' '
+ test_must_fail git rev-parse 000000000 2>stderr &&
+ grep ^hint: stderr >hints &&
+ # 16 candidates, plus one intro line
+ test_line_count = 17 hints
+'
+
+test_expect_success C_LOCALE_OUTPUT 'ambiguity hints respect type' '
+ test_must_fail git rev-parse 000000000^{commit} 2>stderr &&
+ grep ^hint: stderr >hints &&
+ # 5 commits, 1 tag (which is a commitish), plus intro line
+ test_line_count = 7 hints
+'
+
+test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '
+ # these two blobs share the same prefix "ee3d", but neither
+ # will pass for a commit
+ echo 851 | git hash-object --stdin -w &&
+ echo 872 | git hash-object --stdin -w &&
+ test_must_fail git rev-parse ee3d^{commit} 2>stderr &&
+ grep ^hint: stderr >hints &&
+ test_line_count = 3 hints
+'
+
test_done
--
2.10.0.492.g14f803f
^ permalink raw reply related
* Re: Changing the default for "core.abbrev"?
From: Jeff King @ 2016-09-26 12:09 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Linus Torvalds, Git Mailing List
In-Reply-To: <vpq37kntbjj.fsf@anie.imag.fr>
On Mon, Sep 26, 2016 at 08:33:52AM +0200, Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am not opposed to bump the default to 12 or whatever, but I
> > suspect any lengthening today may need to be accompanied by a tool
> > support that finds the set of objects that are reachable from a
> > commit whose names begin with non-unique abbreviations that appear
> > in the commit log message.
>
> Something much simpler would be to set core.abbrev at clone time,
> depending on the size of the project just cloned. So, when cloning a
> hello-world, we'd keep the 7 but when cloning a big project we'd get a
> larger value.
>
> This doesn't cover the case of someone growing his own project without
> cloning, and isn't as clever as actually looking for colision, but it
> would probably provide a sane default in 99% cases, and wouldn't be
> worse than hardcoding 7 in the 1% remaining cases.
I think we could easily make this even more dynamic, and just base the
minimum for DEFAULT_ABBREV on the number of objects _currently_ in the
repository, plus some safety factor. We could do this cheaply by just
counting the number of objects in the packs (which we get for free when
we open their pack index). That misses loose objects, but if you have 4
million loose objects you have bigger problems than abbreviation
lengths, I think.
OTOH, any scheme that looks at the current repository size will
eventually grow outdated. The safety factor depends on how fast your
repository grows, and how big you expect it to eventually get. Such a
default might still have been using 7-character abbreviations on
linux.git in 2006, and we'd be stuck with them now.
The idea of a 12-character default is basically that we'd expect decades
or more for even the largest projects to get there, so you err on the
side of future-proofing.
-Peff
^ permalink raw reply
* Re: [PATCH 04/10] get_short_sha1: peel tags when looking for treeish
From: Jeff King @ 2016-09-26 12:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115947.hksmtkqp3i4tfftx@sigill.intra.peff.net>
On Mon, Sep 26, 2016 at 07:59:48AM -0400, Jeff King wrote:
> Subject: Re: [PATCH 04/10] get_short_sha1: peel tags when looking for treeish
>
> The treeish disambiguation function tries to peel tags, but
> it does so by calling:
Probably the subject should be "parse tags when...". We already try to
peel, we just don't do it right.
-Peff
^ permalink raw reply
* BUG: Git blame provides incorrect previous commit if the line is uncommitted
From: Eric Amodio @ 2016-09-26 12:29 UTC (permalink / raw)
To: git
This is the first time I've reported a bug with Git so please forgive
me if this isn't the right place, format, etc.
If git blame --porcelain (or --line-porcelain or --incremental) is run
on a file that has uncommitted changes any uncommitted lines have the
wrong previous sha. Instead of the sha the last time that line was
changed or even the last time the file was changed it seem to return
the last commit in the repository. This seems to only affect
uncommitted lines, other line seem to be populated properly.
I am using git version 2.10.0 on macOS 10.12 (16A323).
Please let me know if I can provide any more information.
Thanks,
Eric Amodio
^ permalink raw reply
* RE: git-upload-pack hangs
From: Jason Pyeron @ 2016-09-26 13:26 UTC (permalink / raw)
To: git
In-Reply-To: <66A60DA77398CD439FA676CEF593977D692508@exchange.1.internal.pdinc.us>
> -----Original Message-----
> From: Jason Pyeron
> Sent: Monday, September 26, 2016 01:51
>
> git is hanging on clone. I am runnig (cygwin) git 2.8.3 on
> IIS7 (windows server 2012 R2).
>
> Where can I start to perform additional debugging?
>
Reading this thread, it seems plausible as a cause since it aligns with my testing.
http://www.spinics.net/lists/git/msg279437.html [ and http://www.spinics.net/lists/git/attachments/binQFGHirNLw3.bin ]
I will start to trudge into the code to see if this (or similar) has been applied and if not, does it fix it.
> Selected items I have read, but they did not help:
>
> http://unix.stackexchange.com/questions/98959/git-upload-pack-
> hangs-indefinitely
>
> https://sparethought.wordpress.com/2012/12/06/setting-git-to-w
ork-behind-ntlm-authenticated-proxy-cntlm-to-the-rescue/
>
> https://sourceforge.net/p/cntlm/bugs/24/
>
> invocation of the clone:
>
> jpyeron.adm@SERVER /tmp
> $ GIT_TRACE=1 GIT_CURL_VERBOSE=true git clone
> http://SERVER.domain.com/git/test.git
> 01:23:37.020476 git.c:350 trace: built-in: git
> 'clone' 'http://SERVER.domain.com/git/test.git'
> Cloning into 'test'...
> 01:23:37.206046 run-command.c:336 trace: run_command:
> 'git-remote-http' 'origin' 'http://SERVER.domain.com/git/test.git'
> * STATE: INIT => CONNECT handle 0x60009a140; line 1397
> (connection #-5000)
> * Couldn't find host SERVER.domain.com in the .netrc file;
> using defaults
> * Added connection 0. The cache now contains 1 members
> * Trying ::1...
> * TCP_NODELAY set
> * STATE: CONNECT => WAITCONNECT handle 0x60009a140; line 1450
> (connection #0)
> * Connected to SERVER.domain.com (::1) port 80 (#0)
> * STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x60009a140;
> line 1557 (connection #0)
> * Marked for [keep alive]: HTTP default
> * STATE: SENDPROTOCONNECT => DO handle 0x60009a140; line 1575
> (connection #0)
> > GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1
> Host: SERVER.domain.com
> User-Agent: git/2.8.3
> Accept: */*
> Accept-Encoding: gzip
> Accept-Language: en-US, *;q=0.9
> Pragma: no-cache
>
> * STATE: DO => DO_DONE handle 0x60009a140; line 1654 (connection #0)
> * STATE: DO_DONE => WAITPERFORM handle 0x60009a140; line 1781
> (connection #0)
> * STATE: WAITPERFORM => PERFORM handle 0x60009a140; line 1791
> (connection #0)
> * HTTP 1.1 or later with persistent connection, pipelining supported
> < HTTP/1.1 200 OK
> < Cache-Control: no-cache, max-age=0, must-revalidate
> < Pragma: no-cache
> < Content-Type: application/x-git-upload-pack-advertisement
> < Expires: Fri, 01 Jan 1980 00:00:00 GMT
> * Server Microsoft-IIS/8.5 is not blacklisted
> < Server: Microsoft-IIS/8.5
> < X-Powered-By: ASP.NET
> < Date: Mon, 26 Sep 2016 05:23:37 GMT
> * Marked for [closure]: Connection: close used
> < Connection: close
> < Content-Length: 310
> <
> * STATE: PERFORM => DONE handle 0x60009a140; line 1955 (connection #0)
> * multi_done
> * Curl_http_done: called premature == 0
> * Closing connection 0
> * The cache now contains 0 members
> 01:23:37.688252 run-command.c:336 trace: run_command:
> 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> '--thin' '--check-self-contained-and-connected' '--cloning'
> 'http://SERVER.domain.com/git/test.git/'
> 01:23:37.717168 exec_cmd.c:120 trace: exec: 'git'
> 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> '--thin' '--check-self-contained-and-connected' '--cloning'
> 'http://SERVER.domain.com/git/test.git/'
> 01:23:37.749820 git.c:350 trace: built-in: git
> 'fetch-pack' '--stateless-rpc' '--stdin' '--lock-pack'
> '--thin' '--check-self-contained-and-connected' '--cloning'
> 'http://SERVER.domain.com/git/test.git/'
> * STATE: INIT => CONNECT handle 0x60009a140; line 1397
> (connection #-5000)
> * Couldn't find host SERVER.domain.com in the .netrc file;
> using defaults
> * Added connection 1. The cache now contains 1 members
> * Hostname SERVER.domain.com was found in DNS cache
> * Trying ::1...
> * TCP_NODELAY set
> * STATE: CONNECT => WAITCONNECT handle 0x60009a140; line 1450
> (connection #1)
> * Connected to SERVER.domain.com (::1) port 80 (#1)
> * STATE: WAITCONNECT => SENDPROTOCONNECT handle 0x60009a140;
> line 1557 (connection #1)
> * Marked for [keep alive]: HTTP default
> * STATE: SENDPROTOCONNECT => DO handle 0x60009a140; line 1575
> (connection #1)
> > POST /git/test.git/git-upload-pack HTTP/1.1
> Host: SERVER.domain.com
> User-Agent: git/2.8.3
> Accept-Encoding: gzip
> Content-Type: application/x-git-upload-pack-request
> Accept: application/x-git-upload-pack-result
> Content-Length: 140
>
> * upload completely sent off: 140 out of 140 bytes
> * STATE: DO => DO_DONE handle 0x60009a140; line 1654 (connection #1)
> * STATE: DO_DONE => WAITPERFORM handle 0x60009a140; line 1781
> (connection #1)
> * STATE: WAITPERFORM => PERFORM handle 0x60009a140; line 1791
> (connection #1)
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
- -
- Jason Pyeron PD Inc. http://www.pdinc.us -
- Principal Consultant 10 West 24th Street #100 -
- +1 (443) 269-1555 x333 Baltimore, Maryland 21218 -
- -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Jeff King @ 2016-09-26 13:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gustavo Grieco, git
In-Reply-To: <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 05:10:31PM -0700, Junio C Hamano wrote:
> Gustavo Grieco <gustavo.grieco@imag.fr> writes:
>
> > We found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:
> >
> > $ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
>
> Interesting. If you prepare such a broken loose object file in your
> local repository, I would expect that either unpack_sha1_header() or
> unpack_sha1_header_to_strbuf() that sha1_loose_object_info() calls
> would detect and barf by noticing that an error came from libz while
> it attempts to inflate and would not even call parse_sha1_header.
>
> But it is nevertheless bad to assume that whatever happens to
> inflate without an error must be formatted correctly to allow
> parsing (i.e. has ' ' and then NUL termination within the first 32
> bytes after inflation), which is exactly what the hdr[32] is saying.
Yeah. I also was surprised that we didn't barf on a zlib failure. But
based on previous debugging of corrupted zlib data, my recollection
is that there are a large number of weird corruptions that zlib will
happily pass back and only later complain about a checksum error. So
presumably "x" is one of those, and it might not hold for other
corruptions (but I didn't try).
> Note that this is totally unteseted and not thought through; I
> briefly thought about what unpack_sha1_header_to_strbuf() does with
> this change (it first lets unpack_sha1_header() to attempt with a
> small buffer but it seems to discard the error code from it before
> seeing if the returned buffer has NUL in it); there may be bad
> interactions with it.
Yeah, that seems wrong. I don't think it would involve an out of bounds
read, but we probably could fail to correctly report zlib corruption.
> diff --git a/sha1_file.c b/sha1_file.c
> index 60ff21f..dfcbd76 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1648,6 +1648,8 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
>
> int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
> {
> + int status;
> +
> /* Get the data stream */
> memset(stream, 0, sizeof(*stream));
> stream->next_in = map;
> @@ -1656,7 +1658,15 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
> stream->avail_out = bufsiz;
>
> git_inflate_init(stream);
> - return git_inflate(stream, 0);
> + status = git_inflate(stream, 0);
> + if (status)
> + return status;
> +
> + /* Make sure we got the terminating NUL for the object header */
> + if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
> + return -1;
> +
> + return 0;
This doesn't look too invasive as an approach, though I would have done
it differently. We're making the assumption that once there is a NUL,
the header-parser won't do anything stupid, which creates a coupling
between those two bits of code. My inclination would have been to just
treat the header as a ptr/len pair, and make sure the parser never reads
past the end.
But I implemented that, and it _is_ rather invasive. And it's not like
coupling unpack_sha1_header() and parse_sha1_header() is all that
terrible; they are meant to be paired.
I haven't read through your follow-up yet; I'll do that before posting
my version.
> static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
> @@ -1758,6 +1768,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
> char c = *hdr++;
> if (c == ' ')
> break;
> + if (!c)
> + die("invalid object header");
> type_len++;
> }
We keep reading from hdr after this, though I think those bits would all
bail correctly on seeing NUL.
-Peff
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Jeff King @ 2016-09-26 14:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <xmqqshsnuvvz.fsf_-_@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 09:29:04PM -0700, Junio C Hamano wrote:
> To correct this, do these three things:
>
> - rename unpack_sha1_header() to unpack_sha1_short_header() and
> have unpack_sha1_header_to_strbuf() keep calling that as its
> helper function. This will detect and report zlib errors, but is
> not aware of the format of a loose object (as before).
This step makes sense to me, and is a problem in the original you posted
(i.e., we may not see all of the header in the strbuf variant). Your
refactor looks good.
> - introduce unpack_sha1_header() that calls the same helper
> function, and when zlib reports it inflated OK into the buffer,
> check if the buffer has both SP and NUL in this order. This
> would ensure that parsing function will terminate within the
> buffer that holds the inflated header.
>
> - update unpack_sha1_header_to_strbuf() to check if the resulting
> buffer has both SP and NUL in this order for the same effect.
This part I don't understand, though. We clearly need to look for the
NUL. But why do we need to look for the space? The loop in
parse_sha1_header() can easily detect this as it looks for the end of
the type name (and if it hits the end-of-string, can bail as in your
original patch).
I.e., the root of the problem is that we pass parse_sha1_header() a the
"ptr" half of a ptr/len buffer, and it has no idea how much we read.
But once we get it that information (either by passing the length, or by
ensuring that the buffer is NUL-terminated, it should be easy for it to
do the right thing.
Anyway, here's my ptr/len version (which passes the length back out of
unpack_sha1_header via an in/out pointer). After thinking on it, though,
I'm of the opinion that we're better off just ensuring that "hdr" is
NUL-terminated. We end up assuming that anyway later, since we have to
know how much of the header buffer was consumed by parsing.
Do note the final call below in the streaming loose-open code, which
exhibits that, but also seems to call parse_sha1_header() without
checking its return value. I think that needs fixed regardless of the
approach.
---
diff --git a/cache.h b/cache.h
index d0494c8..e89dcff 100644
--- a/cache.h
+++ b/cache.h
@@ -1121,8 +1121,9 @@ extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned c
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
extern int git_open_noatime(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
-extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
-extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
+extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned
+ long mapsize, void *buffer, size_t *bufsiz);
+extern int parse_sha1_header(const char *hdr, size_t len, unsigned long *sizep);
/* global flag to enable extra checks when accessing packed objects */
extern int do_check_packed_object_crc;
diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..326593b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1646,41 +1646,50 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
return used;
}
-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
+int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize,
+ void *buffer, size_t *bufsiz)
{
+ int ret;
+
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
stream->next_in = map;
stream->avail_in = mapsize;
stream->next_out = buffer;
- stream->avail_out = bufsiz;
+ stream->avail_out = *bufsiz;
git_inflate_init(stream);
- return git_inflate(stream, 0);
+ ret = git_inflate(stream, 0);
+ *bufsiz -= stream->avail_out;
+ return ret;
}
-static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
- unsigned long mapsize, void *buffer,
- unsigned long bufsiz, struct strbuf *header)
+static int unpack_sha1_header_to_strbuf(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, size_t *bufsiz,
+ struct strbuf *header)
{
+ size_t initial_len = *bufsiz;
int status;
- status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+ status = unpack_sha1_header(stream, map, mapsize, buffer, &initial_len);
/*
* Check if entire header is unpacked in the first iteration.
*/
- if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+ if (memchr(buffer, '\0', initial_len)) {
+ *bufsiz = initial_len;
return 0;
+ }
/*
* buffer[0..bufsiz] was not large enough. Copy the partial
* result out to header, and then append the result of further
* reading the stream.
*/
- strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
+ strbuf_add(header, buffer, initial_len);
stream->next_out = buffer;
- stream->avail_out = bufsiz;
+ stream->avail_out = *bufsiz;
do {
status = git_inflate(stream, 0);
@@ -1688,7 +1697,7 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
return 0;
stream->next_out = buffer;
- stream->avail_out = bufsiz;
+ stream->avail_out = *bufsiz;
} while (status != Z_STREAM_END);
return -1;
}
@@ -1743,9 +1752,11 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
* too permissive for what we want to check. So do an anal
* object header parse by hand.
*/
-static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
- unsigned int flags)
+static int parse_sha1_header_extended(const char *hdr, size_t len,
+ struct object_info *oi,
+ unsigned int flags)
{
+ const char *end = hdr + len;
const char *type_buf = hdr;
unsigned long size;
int type, type_len = 0;
@@ -1754,12 +1765,14 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
* The type can be of any size but is followed by
* a space.
*/
- for (;;) {
+ while (hdr < end) {
char c = *hdr++;
if (c == ' ')
break;
type_len++;
}
+ if (hdr >= end)
+ return -1;
type = type_from_string_gently(type_buf, type_len, 1);
if (oi->typename)
@@ -1781,10 +1794,10 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
* decimal format (ie "010" is not valid).
*/
size = *hdr++ - '0';
- if (size > 9)
+ if (size > 9 || hdr >= end)
return -1;
if (size) {
- for (;;) {
+ while (hdr >= end) {
unsigned long c = *hdr - '0';
if (c > 9)
break;
@@ -1799,17 +1812,17 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
/*
* The length must be followed by a zero byte
*/
- return *hdr ? -1 : type;
+ return hdr >= end || *hdr ? -1 : type;
}
-int parse_sha1_header(const char *hdr, unsigned long *sizep)
+int parse_sha1_header(const char *hdr, size_t len, unsigned long *sizep)
{
struct object_info oi;
oi.sizep = sizep;
oi.typename = NULL;
oi.typep = NULL;
- return parse_sha1_header_extended(hdr, &oi, LOOKUP_REPLACE_OBJECT);
+ return parse_sha1_header_extended(hdr, len, &oi, LOOKUP_REPLACE_OBJECT);
}
static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type *type, unsigned long *size, const unsigned char *sha1)
@@ -1817,9 +1830,11 @@ static void *unpack_sha1_file(void *map, unsigned long mapsize, enum object_type
int ret;
git_zstream stream;
char hdr[8192];
+ size_t hdr_len = sizeof(hdr);
- ret = unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr));
- if (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)
+ ret = unpack_sha1_header(&stream, map, mapsize, hdr, &hdr_len);
+ if (ret < Z_OK ||
+ (*type = parse_sha1_header(hdr, hdr_len, size)) < 0)
return NULL;
return unpack_sha1_rest(&stream, hdr, *size, sha1);
@@ -2697,6 +2712,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
void *map;
git_zstream stream;
char hdr[32];
+ size_t hdr_len = sizeof(hdr);
struct strbuf hdrbuf = STRBUF_INIT;
if (oi->delta_base_sha1)
@@ -2725,19 +2741,19 @@ static int sha1_loose_object_info(const unsigned char *sha1,
if (oi->disk_sizep)
*oi->disk_sizep = mapsize;
if ((flags & LOOKUP_UNKNOWN_OBJECT)) {
- if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
+ if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, &hdr_len, &hdrbuf) < 0)
status = error("unable to unpack %s header with --allow-unknown-type",
sha1_to_hex(sha1));
- } else if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+ } else if (unpack_sha1_header(&stream, map, mapsize, hdr, &hdr_len) < 0)
status = error("unable to unpack %s header",
sha1_to_hex(sha1));
if (status < 0)
; /* Do nothing */
else if (hdrbuf.len) {
- if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
+ if ((status = parse_sha1_header_extended(hdrbuf.buf, hdrbuf.len, oi, flags)) < 0)
status = error("unable to parse %s header with --allow-unknown-type",
sha1_to_hex(sha1));
- } else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
+ } else if ((status = parse_sha1_header_extended(hdr, hdr_len, oi, flags)) < 0)
status = error("unable to parse %s header", sha1_to_hex(sha1));
git_inflate_end(&stream);
munmap(map, mapsize);
diff --git a/streaming.c b/streaming.c
index 3c48f04..ee73544 100644
--- a/streaming.c
+++ b/streaming.c
@@ -334,6 +334,7 @@ static struct stream_vtbl loose_vtbl = {
static open_method_decl(loose)
{
+ size_t len = sizeof(st->u.loose.hdr);
st->u.loose.mapped = map_sha1_file(sha1, &st->u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
@@ -341,13 +342,14 @@ static open_method_decl(loose)
st->u.loose.mapped,
st->u.loose.mapsize,
st->u.loose.hdr,
- sizeof(st->u.loose.hdr)) < 0) {
+ &len) < 0) {
git_inflate_end(&st->z);
munmap(st->u.loose.mapped, st->u.loose.mapsize);
return -1;
}
- parse_sha1_header(st->u.loose.hdr, &st->size);
+ if (parse_sha1_header(st->u.loose.hdr, len, &st->size) < 0)
+ return -1;
st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;
st->u.loose.hdr_avail = st->z.total_out;
st->z_state = z_used;
^ permalink raw reply related
* Re: git-gui, was Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Junio C Hamano @ 2016-09-26 14:21 UTC (permalink / raw)
To: Vasco Almeida; +Cc: git, Pat Thoyts
In-Reply-To: <xmqqlgyl86fe.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Vasco Almeida <vascomalmeida@sapo.pt> writes:
>
>> I have sent some git-gui patches on May this year and I think it will
>> add value to accepted them at some point:
>
> Yeah, they may be of value, but the thing is, I am not really in the
> position to review or apply them (I don't do git-gui).
>
> If Pat is not going to return, we would need to find volunteers to
> be maintainers of "git-gui" first.
>
> Thanks. I may get to these patches when/if I find time, but it is
> not likely to happen very soon.
This I just did (haven't pushed out the results, which will happen
after today's regular integration cycle in the afternoon US/Pacific
time).
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Philip Oakley @ 2016-09-26 13:00 UTC (permalink / raw)
To: Junio C Hamano, Vegard Nossum; +Cc: git, Santi Béjar, Kevin Bracey
In-Reply-To: <xmqq7f9zwl2q.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>>I use rev^..rev daily, and I'm surely not the only one.
>>
>> Not everyone knows the 'trick' and may not use it daily.
>>
>> Consider stating what it is useful for (e.g. "useful to get the
>> commits and all commits in the branches that were merged into commit"
>> - paraphrased from the doc text)
>>
>>> To save typing
>>> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch
>>> name)
>>> we can make rev^- a shorthand for that.
>>>
>>> The existing syntax rev^! seems like it should do the same, but it
>>> doesn't really do the right thing for merge commits (it gives only the
>>> merge itself).
>>
>> .. rather than the commit and those on side branches).
>>> As a natural generalisation, we also accept rev^-n where n excludes the
>>> nth parent of rev,
>>
>>> although this is expected to be generally less useful.
>>
>> Presumptious? for a two parent merge, surely(?) rev^-2 will give you
>> what has been going on on the main line while the branch was being
>> prepared... compare A^- and A^-2.
>
> All good comments. It often is a good strategy to avoid subjective
> "this is useful" and "this is not useful" assessment, and instead
> let the feature itself find its supporters in the reading public.
>
>>> +Parent Exclusion Notation
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
>>> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>>> +given. This is typically useful for merge commits where you
>>> +can just pass '<commit>{caret}-' to get all the commits in the branch
>>
>> s/get all the/get the commit and all the/ ?
>> It could be misread as a way of selecting just those commits that are
>> within the side branch without including the given commit itself.
>>
>>> +that was merged in merge commit '<commit>'.
>>> +
>>> Other <rev>{caret} Parent Shorthand Notations
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Two other shorthands exist, particularly useful for merge commits,
>
> Is it just me that this new thing belongs to this "other shorthand
> notations", making the total to three from two? It really is a
> closely related cousin of existing 'r1{caret}!'; instead of
> excluding all of its parents, it only excludes the specified one of
> its parents. IOW, this new one is better described as the third
> other shorthand in this "Other Notations" section, without creating
> a new "Parent Exclusion Notation" section.
>
True. It probably should be there.
>>> @@ -316,6 +324,10 @@ Revision Range Summary
>>> <rev2> but exclude those that are reachable from both. When
>>> either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>>>
>>> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
>
> Huh? Isn't the first example missing the necessary minus sign?
>
>>> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>>> + given.
>>> +
>>> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>>> A suffix '{caret}' followed by an at sign is the same as listing
>>> all parents of '<rev>' (meaning, include anything reachable from
>>> @@ -339,6 +351,8 @@ spelt out:
>>> C I J F C
>>> B..C = ^B C C
>>> B...C = B ^F C G H D E B C
>>> + B^- = B^..B
>>> + = B ^B^1 E I J F B
>
> Even though these are order independent, the second line should say
>
> = ^B^1 B E I J F B
>
> to be consistent with the expansion of B..C, I would think.
Agreed.
>
>>> diff --git builtin/rev-parse.c builtin/rev-parse.c
>>> index 76cf05e..ad5e6ac 100644
>>> --- builtin/rev-parse.c
>>> +++ builtin/rev-parse.c
>>> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>>> return 0;
>>> }
>>>
>>> +static int try_parent_exclusion(const char *arg)
>>> +{
>>> + int ret = 0;
>>> + char *to_rev = NULL;
>>> + char *from_rev = NULL;
>>> + unsigned char to_sha1[20];
>>> + unsigned char from_sha1[20];
>>> +
>>> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
>>> + goto out;
>>> + if (get_sha1_committish(to_rev, to_sha1))
>>> + goto out;
>>> + if (get_sha1_committish(from_rev, from_sha1))
>>> + goto out;
>>> +
>>> + show_rev(NORMAL, to_sha1, to_rev);
>>> + show_rev(REVERSED, from_sha1, from_rev);
>>> +
>>> + ret = 1;
>>> +
>>> +out:
>>> + free(to_rev);
>>> + free(from_rev);
>>> + return ret;
>>> +}
>>> +
>>> static int try_parent_shorthands(const char *arg)
>>> {
>>> char *dotdot;
>
> I did not expect that this needs an entirely new helper function,
> instead of being implemented as a new special case of existing
> try_parent_shorthands() function. You'd need to strstr "^-" and
> parse a sequence of digits that follow it, which may want a helper
> to make sure you can error out if fed "some^-12thing" saying that
> "12thing" is not an integer, extend the existing "parents-only"
> thing so that it can represent three cases (i.e. @? !? or -?), and
> need a new variable to denote which parent is to be excluded when it
> is the '-' kind. You'd need to temporarily *dotdot = '\0', parse
> what is before "^-" and revert *dotdot = '^' like existing helper
> function just the same.
>
> Exactly the same comment probably applies to the changes to the
> parser in revision.c, I would imagine, but I didn't read it ;-)
Sounds sensible. I hadn't double checked Vegard's implementation at this
point.
--
Philip
>
^ permalink raw reply
* Re: BUG: Git blame provides incorrect previous commit if the line is uncommitted
From: Junio C Hamano @ 2016-09-26 16:05 UTC (permalink / raw)
To: Eric Amodio; +Cc: git
In-Reply-To: <CAJxnqO6oMG2RvwP7y0Yt_xTrfeqqO6ZOUn5HWF7-h1hcjY+=bg@mail.gmail.com>
Eric Amodio <eamodio@gmail.com> writes:
> This is the first time I've reported a bug with Git so please forgive
> me if this isn't the right place, format, etc.
>
> If git blame --porcelain (or --line-porcelain or --incremental) is run
> on a file that has uncommitted changes any uncommitted lines have the
> wrong previous sha. Instead of the sha the last time that line was
> changed or even the last time the file was changed it seem to return
> the last commit in the repository.
This is not limited to the case where uncommitted changes getting
blamed to the working tree, I think. Replace C in the following
description with "a fictional commit C that would have made as a
direct child of HEAD if you were to commit all these uncommited
changes" and read on.
When the command finds that a line is attributed to commit C,
"previous" field in the internal data structure the command uses to
keeps track of the ancestry is shown there. What the field means is
this:
The command compared C (the final answer) with this "previous"
commit (typically a parent of it, but when you use -S or
--reverse option it may be different), and it was found that C
introduced this line.
So, no. "previous" is not "what would the result of running another
'git blame' on the state _before_ C to blame the general area?" It
is meant as a hint for _you_ (rather, whatever tool is reading the
incremental output) telling where to run another blame if you want
to dig further, and it does not waste cycles to compute another
blame on each and every output to show that before being asked.
^ permalink raw reply
* Re: [PATCH v8 03/11] run-command: move check_pipe() from write_or_die to run_command
From: Lars Schneider @ 2016-09-26 16:13 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <854ff387-57a4-4c27-4c27-b834f7797694@gmail.com>
> On 25 Sep 2016, at 00:12, Jakub Narębski <jnareb@gmail.com> wrote:
>
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> Move check_pipe() to run_command and make it public. This is necessary
>> to call the function from pkt-line in a subsequent patch.
>
> All right.
Does this mean I can add your "Acked-by: Jakub Narebski <jnareb@gmail.com>" ?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH] unpack_sha1_header(): detect malformed object header
From: Junio C Hamano @ 2016-09-26 16:15 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak, Gustavo Grieco
In-Reply-To: <20160926140309.l2h4b65gpqyutepn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This part I don't understand, though. We clearly need to look for the
> NUL. But why do we need to look for the space? The loop in
> parse_sha1_header() can easily detect this as it looks for the end of
> the type name (and if it hits the end-of-string, can bail as in your
> original patch).
> I.e., the root of the problem is that we pass parse_sha1_header() a the
> "ptr" half of a ptr/len buffer, and it has no idea how much we read.
> But once we get it that information (either by passing the length, or by
> ensuring that the buffer is NUL-terminated, it should be easy for it to
> do the right thing.
Yup.
> Anyway, here's my ptr/len version (which passes the length back out of
> unpack_sha1_header via an in/out pointer). After thinking on it, though,
> I'm of the opinion that we're better off just ensuring that "hdr" is
> NUL-terminated. We end up assuming that anyway later, since we have to
> know how much of the header buffer was consumed by parsing.
I'd agree, not because I didn't first go in this <ptr,len> route
myself, but because the attached change does look quite invasive.
Also, I think it is OK to ask unpack_*_header() to fail if what it
turns can no way be a header, e.g. lacks NUL termination.
> Do note the final call below in the streaming loose-open code, which
> exhibits that, but also seems to call parse_sha1_header() without
> checking its return value. I think that needs fixed regardless of the
> approach.
Good that your attempt to signature-changing change caught it. I'll
take a further look.
Thanks.
^ permalink raw reply
* Re: [PATCH v8 03/11] run-command: move check_pipe() from write_or_die to run_command
From: Jakub Narębski @ 2016-09-26 16:21 UTC (permalink / raw)
To: Lars Schneider
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <4B4255C0-4C0C-45F0-B37D-0C78C2AAFAE9@gmail.com>
On 26 September 2016 at 18:13, Lars Schneider <larsxschneider@gmail.com> wrote:
>> On 25 Sep 2016, at 00:12, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>
>>> Move check_pipe() to run_command and make it public. This is necessary
>>> to call the function from pkt-line in a subsequent patch.
>>
>> All right.
>
> Does this mean I can add your "Acked-by: Jakub Narebski <jnareb@gmail.com>" ?
Well, Acked-by makes sense if it is from subsystem maintainer. I can only
claim gitweb subsystem where my ACKs might make sense.
This "All right" is here to note that I have read this patch (and not
skipped it),
and I have't found anything to complain about or nitpick ;-P
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Linus Torvalds @ 2016-09-26 16:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20160926120036.mqs435a36njeihq6@sigill.intra.peff.net>
On Mon, Sep 26, 2016 at 5:00 AM, Jeff King <peff@peff.net> wrote:
>
> This patch teaches get_short_sha1() to list the sha1s of the
> objects it found, along with a few bits of information that
> may help the user decide which one they meant.
This looks very good to me, but I wonder if it couldn't be even more aggressive.
In particular, the only hashes that most people ever use in short form
are commit hashes. Those are the ones you'd use in normal human
interactions to point to something happening.
So when the disambiguation notices that there is ambiguity, but there
is only _one_ commit, maybe it should just have an aggressive mode
that says "use that as if it wasn't ambiguous".
And then have an explicit command (or flag) to do disambiguation for
when you explicitly want it.
Rationale: you'd never care about short forms for tags. You'd just use
the tag name. And while blob ID's certainly show up in short form in
diff output (in the "index" line), very few people will use them. And
tree hashes are basically never seen outside of any plumbing commands
and then seldom in shortened form.
So I think it would make sense to default to a mode that just picks
the commit hash if there is only one such hash. Sure, some command
might want a "treeish", but a commit is still more likely than a tree
or a tag.
But regardless, this series looks like a good thing.
Linus
^ permalink raw reply
* Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators
From: Junio C Hamano @ 2016-09-26 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115901.txmbr4e6xzwyfpmo@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The get_sha1() family of functions takes a flags field, but
> some of the flags are mutually exclusive. In particular, we
> can only handle one disambiguating function, and the flags
> quietly override each other. Let's instead detect these as
> programming bugs.
>
> Technically some of the flags are supersets of the others,
> so treating COMMITTISH|TREEISH as just COMMITTISH is not
> wrong, but it's a good sign the caller is confused. And
> certainly asking for BLOB|TREE does not work.
>
> We can do the check easily with some bit-twiddling, and as a
> bonus, the bit-mask of disambiguators will come in handy in
> a future patch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Other than your reinvention of HAS_MULTI_BITS(), which has been with
us since db7244bd ("parse-options new features.", 2007-11-07), this
looks like a reasonable thing to do.
;-)
> cache.h | 5 +++++
> sha1_name.c | 9 +++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index d0494c8..7bd78ca 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1203,6 +1203,11 @@ struct object_context {
> #define GET_SHA1_FOLLOW_SYMLINKS 0100
> #define GET_SHA1_ONLY_TO_DIE 04000
>
> +#define GET_SHA1_DISAMBIGUATORS \
> + (GET_SHA1_COMMIT | GET_SHA1_COMMITTISH | \
> + GET_SHA1_TREE | GET_SHA1_TREEISH | \
> + GET_SHA1_BLOB)
> +
> extern int get_sha1(const char *str, unsigned char *sha1);
> extern int get_sha1_commit(const char *str, unsigned char *sha1);
> extern int get_sha1_committish(const char *str, unsigned char *sha1);
> diff --git a/sha1_name.c b/sha1_name.c
> index faf873c..f9812ff 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -310,6 +310,11 @@ static int prepare_prefixes(const char *name, int len,
> return 0;
> }
>
> +static int multiple_bits_set(unsigned flags)
> +{
> + return !!(flags & (flags - 1));
> +}
> +
> static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> unsigned flags)
> {
> @@ -327,6 +332,10 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> prepare_alt_odb();
>
> memset(&ds, 0, sizeof(ds));
> +
> + if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
> + die("BUG: multiple get_short_sha1 disambiguator flags");
> +
> if (flags & GET_SHA1_COMMIT)
> ds.fn = disambiguate_commit_only;
> else if (flags & GET_SHA1_COMMITTISH)
^ permalink raw reply
* Re: [PATCH 04/10] get_short_sha1: peel tags when looking for treeish
From: Junio C Hamano @ 2016-09-26 16:55 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115947.hksmtkqp3i4tfftx@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The treeish disambiguation function tries to peel tags, but
> it does so by calling:
>
> deref_tag(lookup_object(sha1), ...);
>
> This will only work if we have previously looked at the tag
> and created a "struct tag" for it. Since parsing revision
> arguments typically happens before anything else, this is
> usually not the case, and we would fail to peel the tag (we
> are lucky that deref_tag() gracefully handles the NULL and
> does not segfault).
Makes perfect sense.
> Instead, we can use parse_object(). Note that this is the
> same fix done by 94d75d1 (get_short_sha1(): correctly
> disambiguate type-limited abbreviation, 2013-07-01), but
> that commit fixed only the committish disambiguator, and
> left the bug in the treeish one.
Can you share your secret tool you use to find this kind of thing?
Yes, the patch from that commit does look very similar to what we
see in this patch, but I'd love to see "I am fixing an incorrect
call to lookup-object by replacing it with parse-object; has there
been a similar fix?" automated ;-)
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> sha1_name.c | 2 +-
> t/t1512-rev-parse-disambiguation.sh | 7 +++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 38e51d9..432a308 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -269,7 +269,7 @@ static int disambiguate_treeish_only(const unsigned char *sha1, void *cb_data_un
> return 0;
>
> /* We need to do this the hard way... */
> - obj = deref_tag(lookup_object(sha1), NULL, 0);
> + obj = deref_tag(parse_object(sha1), NULL, 0);
> if (obj && (obj->type == OBJ_TREE || obj->type == OBJ_COMMIT))
> return 1;
> return 0;
> diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
> index 30e0b80..dfd3567 100755
> --- a/t/t1512-rev-parse-disambiguation.sh
> +++ b/t/t1512-rev-parse-disambiguation.sh
> @@ -264,6 +264,13 @@ test_expect_success 'ambiguous commit-ish' '
> test_must_fail git log 000000000...
> '
>
> +# There are three objects with this prefix: a blob, a tree, and a tag. We know
> +# the blob will not pass as a treeish, but the tree and tag should (and thus
> +# cause an error).
> +test_expect_success 'ambiguous tags peel to treeish' '
> + test_must_fail git rev-parse 0000000000f^{tree}
> +'
> +
> test_expect_success 'rev-parse --disambiguate' '
> # The test creates 16 objects that share the prefix and two
> # commits created by commit-tree in earlier tests share a
^ permalink raw reply
* Re: [PATCH 0/3] recursive support for ls-files
From: Brandon Williams @ 2016-09-26 17:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqzimvygdt.fsf@gitster.mtv.corp.google.com>
On 09/25, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > On 09/25, Jeff King wrote:
> >> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
> >>
> >> > After looking at the feedback I rerolled a few things, in particular the
> >> > --submodule_prefix option that existed to give a submodule context about where
> >> > it had been invoked from. People didn't seem to like the idea of exposing this
> >> > to the users (yet anyways) so I removed it as an option and instead have it
> >> > being passed to a child process via an environment variable
> >> > GIT_INTERNAL_SUBMODULE_PREFIX. This way we don't have to support anything to
> >> > external users at the moment.
> >>
> >> I think we can still have it as a command-line argument and declare it
> >> internal. It's not like environment variables cannot also be set by our
> >> callers. :)
> >>
> >> I don't mind it as an environment variable, though. In some ways it
> >> makes things easier. I just think "internal versus external" and the
> >> exact implementation are orthogonal.
> >
> > We may still want it to be an option at some point in the future. This
> > way we can revisit making it an option once we know more about the other
> > uses it could have (aside from just being for submodules as someone
> > suggested).
>
> I do not think it makes too much of a difference between environment
> and command line option. We need an update to the "git" potty to
> say "you told me to use the submodule-prefix feature, but this
> subcommand is not prepared to accept it (yet)" and cause it to error
> out either way, which would mean that a series that introduces the
> feature needs to touch "git.c" anyway, so I would have expected us
> to add command line option first, simply because "git.c" is where it
> happens, optionally with the support for the environment variable,
> not the other way around.
In a previous email you mentioned that this feature should be completely
hidden from users, which is why I removed the command line option for
this latest series. If that isn't what you intended that I can
definitely add the option to git.c. And you would rather we perform the
checking in git.c to see if a subcommand supports the prefix versus
silently ignoring it if it hasn't? I'm assuming this checking would
also be done in git.c?
>
> >> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> >> > King.
> >>
> >> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
> >> try not to have a broken state in the history. It's less important in
> >> this case, because the breakage is not a regression
> >> (--recurse-submodules is a new feature, so you could consider it "not
> >> working" until the 3rd patch). But I think it's still a good rule to
> >> follow, because it makes the commits easier to review, look at later,
> >> etc.
> >>
> >> For that matter, I do not understand why options like "-s" get enabled
> >> in patch 3. I do not mind them starting as disabled in patch 2, but it
> >> seems like "pass along some known-safe options" should be its own patch
> >> somewhere between patches 2 and 3.
>
> Yes, exactly.
>
> An obvious lazy way out to avoid breakage-in-the-middle and make
> incremental progress would be to squash everything into one patch,
> but we should and we should be able to do better.
>
> I'd imagine this three-patch series would be more pleasant for
> future readers if it were structured like:
>
> [1/3] introduces the submodule-prefix as a global feature; at the
> least it needs a way to invoke (either an environment, or an
> option to "git" potty, or both) and prevent mistakes by
> erroring out when it is attempted to call a subcommand that
> does not support the feature (yet).
>
> [2/3] adds the --recurse-submodule feature in a limited form to
> "ls-files". I'd suggest for this step to pass through all
> options and arguments that are safe and reasonably useful
> to pass through without needing anything more than "ah, this
> option was given, so let's stuff it to the argv-array". An
> attempt to give things that are not yet passed through until
> 3/3 to lead to an error that says it is not allowed (yet).
>
> [3-N] each of the remaining steps after 3/N adds support for one
> more thing to be passed that 2/3 refrained from doing, by
> doing more than just "pass it in argv-array", and then remove
> the "not yet supported" error that added by 2/3 for that one
> thing. The first of these "more things" would be to support
> pathspecs as the receiving side would need code changes for
> the matching logic. There may be more, or there may be
> nothing else that requires 4/N, 5/N, etc.
I can do another rework and structure the patch series more inline with
what you are suggesting here.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix
From: Junio C Hamano @ 2016-09-26 17:10 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926120007.eswpfrzs2ed66d2o@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We store the hex prefix in a 40-byte buffer with the prefix
> itself followed by 40-minus-len "x" characters. These x's
> serve no purpose, and the lack of NUL termination makes the
> prefix string annoying to use. Let's just terminate it.
> Note that this is in contrast to the binary prefix, which
> _must_ be zero-padded, because we look at the whole thing
> during a binary search to find the first potential match in
> each pack index.
Makes sense.
> The loose-object hex search cannot use the
> same trick because it has to do a linear walk through the
> unsorted results of readdir() (and even if it could, you'd
> want zeroes instead of x's).
OK.
> struct disambiguate_state {
> int len; /* length of prefix in hex chars */
> - char hex_pfx[GIT_SHA1_HEXSZ];
> + char hex_pfx[GIT_SHA1_HEXSZ + 1];
> unsigned char bin_pfx[GIT_SHA1_RAWSZ];
>
> disambiguate_hint_fn fn;
> @@ -291,7 +291,6 @@ static int init_object_disambiguation(const char *name, int len,
> return -1;
>
> memset(ds, 0, sizeof(*ds));
> - memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);
As the whole thing is cleared here...
>
> for (i = 0; i < len ;i++) {
> unsigned char c = name[i];
> @@ -313,6 +312,7 @@ static int init_object_disambiguation(const char *name, int len,
> }
>
> ds->len = len;
> + ds->hex_pfx[len] = '\0';
... do we even need this one? It would not hurt, though.
> @@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
> status = finish_object_disambiguation(&ds, sha1);
>
> if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
> - return error("short SHA1 %.*s is ambiguous.", ds.len, ds.hex_pfx);
> + return error("short SHA1 %s is ambiguous.", ds.hex_pfx);
Makes sense.
Thanks.
^ permalink raw reply
* Re: Request: Extra case for %G? format
From: Alex @ 2016-09-26 17:18 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <f056af47-ca98-b35c-e343-9f246c0c8f5b@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
>> Then currently %G? results in `N', the same as an unsigned commit.
>>
>> In this case, could %G? please result in a new character? Perhaps `M'
>> for "missing public key"?
>
> Yes, and no.
>
> Really, there are many different reasons why a signature couldn't be
> checked, but gpg itself has these status results:
>
> "For each signature only one of the three codes GOODSIG, BADSIG or
> ERRSIG will be emitted" (doc/DETAILS in gpg's source).
I see. It seems in GPG2 that got expanded to:
"For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
I don't suppose it's worthwhile to support the others? I'm not sure how
important the rest are.
> ERRSIG comes with additional info (RC) that could be parsed for the reason.
>
> Also, in addition to that line, there can be other lines with additional
> information. So there is a lot that could potentially be shown (and *is*
> shown with %GG). In the GOODSIG case, we parse the TRUST info to take
> the trust model into account (and return U for untrusted good).
>
> I wouldn't mind adding E to %G? in the ERRSIG case, even though one has
> to look at %GG in any case (N or E) if one wants to have more details.
That would be great. As long as %G? can tell between a signed but
uncheckable commit and an unsigned commit, then it's good for me.
>
> Cheers,
> Michael
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators
From: Jeff King @ 2016-09-26 17:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqbmzavcqx.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 09:37:10AM -0700, Junio C Hamano wrote:
> > We can do the check easily with some bit-twiddling, and as a
> > bonus, the bit-mask of disambiguators will come in handy in
> > a future patch.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
>
> Other than your reinvention of HAS_MULTI_BITS(), which has been with
> us since db7244bd ("parse-options new features.", 2007-11-07), this
> looks like a reasonable thing to do.
Heh, I _thought_ we had something like that but couldn't find it. I
grepped for "[^&]& .*-", which does match it, but stupidly did it only
in '*.c'. Definitely it should use the existing macro instead.
-Peff
^ permalink raw reply
* Re: [PATCH] git-gui: stop using deprecated merge syntax
From: Stefan Beller @ 2016-09-26 17:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, René Scharfe, Git List, Pat Thoyts,
Dennis Kaarsemaker
In-Reply-To: <xmqqvaxjygb2.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 11:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 24.09.2016 um 13:30 schrieb René Scharfe:
>>> Starting with v2.5.0 git merge can handle FETCH_HEAD internally and
>>> warns when it's called like 'git merge <message> HEAD <commit>' because
>>> that syntax is deprecated. Use this feature in git-gui and get rid of
>>> that warning.
>>>
>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>> ---
>>> Tested only _very_ lightly!
>>>
>>> git-gui/lib/merge.tcl | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/git-gui/lib/merge.tcl b/git-gui/lib/merge.tcl
>>> index 460d32f..5ab6f8f 100644
>>> --- a/git-gui/lib/merge.tcl
>>> +++ b/git-gui/lib/merge.tcl
>>> @@ -112,12 +112,7 @@ method _start {} {
>>> close $fh
>>> set _last_merged_branch $branch
>>>
>>> - set cmd [list git]
>>> - lappend cmd merge
>>> - lappend cmd --strategy=recursive
>>> - lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
>>> - lappend cmd HEAD
>>> - lappend cmd $name
>>> + set cmd [list git merge --strategy=recursive FETCH_HEAD]
>>>
>>> ui_status [mc "Merging %s and %s..." $current_branch $stitle]
>>> set cons [console::new [mc "Merge"] "merge $stitle"]
>>>
>>
>> Much better than my version. I had left fmt-merge-msg and added
>> --no-log to treat merge.log config suitably. But this works too, and
>> is much more obvious.
>>
>> Tested-by: Johannes Sixt <j6t@kdbg.org>
Reviewed-by: Stefan Beller <sbeller@google.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox