* Re: [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Junio C Hamano @ 2016-09-27 15:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160927055202.4ucddki3xkns45om@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But once we introduce other fallbacks, then "utf8 -> latin1" may become
> "UTF-8 -> iso8859-1". A system that knows only "utf8" and "iso8859-1"
> _could_ work if we turned the knobs individually, but won't if we turn
> them both at once. Worse, a system that knows only "UTF-8" and "latin1"
> works now, but would break with your patches.
>
> I'm not convinced it's worth worrying about, though. The existence of
> such a system is theoretical at this point. I'm not even sure how common
> the "know about utf8 but not UTF-8" thing is, or if we were merely being
> overly cautious.
Yeah, I did consider having to try the permutations until it works,
but suspecting that somebody takes "utf8" without taking "UTF-8" is
to pretty much invalidate the basic premise of the existing code,
i.e. spelling it as "UTF-8" is the most likely to work anywhere as
long as UTF-8 is supported, so I stopped worrying about it at that
point.
I'd actually welcome a more generic suggestions we can put in our
documentation so that we can _lose_ the fallback entirely (e.g. "if
your contributor spelled 'utf8' and your system, which does take
'UTF-8', does not like it, then here is what you can do to your
/etc/locale.alias").
^ permalink raw reply
* [PATCH v3 3/3] add David Turner's Two Sigma address
From: David Turner @ 2016-09-27 15:23 UTC (permalink / raw)
To: git, peff; +Cc: David Turner
In-Reply-To: <1474989806-5002-1-git-send-email-dturner@twosigma.com>
From: David Turner <novalis@novalis.org>
Signed-off-by: David Turner <novalis@novalis.org>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)
diff --git a/.mailmap b/.mailmap
index 9441a54..9cc33e9 100644
--- a/.mailmap
+++ b/.mailmap
@@ -48,6 +48,7 @@ David Kågedal <davidk@lysator.liu.se>
David Reiss <dreiss@facebook.com> <dreiss@dreiss-vmware.(none)>
David S. Miller <davem@davemloft.net>
David Turner <novalis@novalis.org> <dturner@twopensource.com>
+David Turner <novalis@novalis.org> <dturner@twosigma.com>
Deskin Miller <deskinm@umich.edu>
Dirk Süsserott <newsletter@dirk.my1.cc>
Eric Blake <eblake@redhat.com> <ebb9@byu.net>
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH v3 2/3] fsck: handle bad trees like other errors
From: David Turner @ 2016-09-27 15:23 UTC (permalink / raw)
To: git, peff; +Cc: David Turner
In-Reply-To: <1474989806-5002-1-git-send-email-dturner@twosigma.com>
Instead of dying when fsck hits a malformed tree object, log the error
like any other and continue. Now fsck can tell the user which tree is
bad, too.
Signed-off-by: David Turner <dturner@twosigma.com>
---
fsck.c | 18 ++++++++-----
t/t1450-fsck.sh | 16 +++++++++--
tree-walk.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++--------
tree-walk.h | 8 ++++++
4 files changed, 106 insertions(+), 19 deletions(-)
diff --git a/fsck.c b/fsck.c
index c9cf3de..4a3069e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
return -1;
name = get_object_name(options, &tree->object);
- init_tree_desc(&desc, tree->buffer, tree->size);
- while (tree_entry(&desc, &entry)) {
+ if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+ return -1;
+ while (tree_entry_gently(&desc, &entry)) {
struct object *obj;
int result;
@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
static int fsck_tree(struct tree *item, struct fsck_options *options)
{
- int retval;
+ int retval = 0;
int has_null_sha1 = 0;
int has_full_path = 0;
int has_empty_name = 0;
@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
unsigned o_mode;
const char *o_name;
- init_tree_desc(&desc, item->buffer, item->size);
+ if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ return retval;
+ }
o_mode = 0;
o_name = NULL;
@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
is_hfs_dotgit(name) ||
is_ntfs_dotgit(name));
has_zero_pad |= *(char *)desc.buffer == '0';
- update_tree_entry(&desc);
+ if (update_tree_entry_gently(&desc)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ break;
+ }
switch (mode) {
/*
@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
o_name = name;
}
- retval = 0;
if (has_null_sha1)
retval += report(options, &item->object, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
if (has_full_path)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2..0fcd38a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '
grep "error in commit $new.*unterminated header: NUL at offset" out
'
-test_expect_success 'malformatted tree object' '
- test_when_finished "git update-ref -d refs/tags/wrong" &&
+test_expect_success 'tree object with duplicate entries' '
test_when_finished "remove_object \$T" &&
T=$(
GIT_INDEX_FILE=test-index &&
@@ -208,6 +207,19 @@ test_expect_success 'malformatted tree object' '
grep "error in tree .*contains duplicate file entries" out
'
+test_expect_success 'unparseable tree object' '
+ test_when_finished "git update-ref -d refs/heads/wrong" &&
+ test_when_finished "remove_object \$tree_sha1" &&
+ test_when_finished "remove_object \$commit_sha1" &&
+ tree_sha1=$(printf "100644 \0twenty-bytes-of-junk" | git hash-object -t tree --stdin -w --literally) &&
+ commit_sha1=$(git commit-tree $tree_sha1) &&
+ git update-ref refs/heads/wrong $commit_sha1 &&
+ test_must_fail git fsck 2>out &&
+ test_i18ngrep "error: empty filename in tree entry" out &&
+ test_i18ngrep "$tree_sha1" out &&
+ ! test_i18ngrep "fatal: empty filename in tree entry" out
+'
+
test_expect_success 'tag pointing to nonexistent' '
cat >invalid-tag <<-\EOF &&
object ffffffffffffffffffffffffffffffffffffffff
diff --git a/tree-walk.c b/tree-walk.c
index 24f9a0f..828f435 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)
return str;
}
-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
{
const char *path;
unsigned int mode, len;
- if (size < 23 || buf[size - 21])
- die(_("too-short tree object"));
+ if (size < 23 || buf[size - 21]) {
+ strbuf_addstr(err, _("too-short tree object"));
+ return -1;
+ }
path = get_mode(buf, &mode);
- if (!path)
- die(_("malformed mode in tree entry for tree"));
- if (!*path)
- die(_("empty filename in tree entry for tree"));
+ if (!path) {
+ strbuf_addstr(err, _("malformed mode in tree entry"));
+ return -1;
+ }
+ if (!*path) {
+ strbuf_addstr(err, _("empty filename in tree entry"));
+ return -1;
+ }
len = strlen(path) + 1;
/* Initialize the descriptor entry */
desc->entry.path = path;
desc->entry.mode = canon_mode(mode);
desc->entry.oid = (const struct object_id *)(path + len);
+
+ return 0;
}
-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
{
desc->buffer = buffer;
desc->size = size;
if (size)
- decode_tree_entry(desc, buffer, size);
+ return decode_tree_entry(desc, buffer, size, err);
+ return 0;
+}
+
+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (init_tree_desc_internal(desc, buffer, size, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ int result = init_tree_desc_internal(desc, buffer, size, &err);
+ if (result)
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return result;
}
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
*a = t->entry;
}
-void update_tree_entry(struct tree_desc *desc)
+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
{
const void *buf = desc->buffer;
const unsigned char *end = desc->entry.oid->hash + 20;
@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)
desc->buffer = buf;
desc->size = size;
if (size)
- decode_tree_entry(desc, buf, size);
+ return decode_tree_entry(desc, buf, size, err);
+ return 0;
+}
+
+void update_tree_entry(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int update_tree_entry_gently(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err)) {
+ error("%s", err.buf);
+ strbuf_release(&err);
+ /* Stop processing this tree after error */
+ desc->size = 0;
+ return -1;
+ }
+ strbuf_release(&err);
+ return 0;
}
int tree_entry(struct tree_desc *desc, struct name_entry *entry)
@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)
return 1;
}
+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
+{
+ if (!desc->size)
+ return 0;
+
+ *entry = desc->entry;
+ if (update_tree_entry_gently(desc))
+ return 0;
+ return 1;
+}
+
void setup_traverse_info(struct traverse_info *info, const char *base)
{
int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index 97a7d69..68bb78b 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)
return (const char *)ne->oid - ne->path - 1;
}
+/*
+ * The _gently versions of these functions warn and return false on a
+ * corrupt tree entry rather than dying,
+ */
+
void update_tree_entry(struct tree_desc *);
+int update_tree_entry_gently(struct tree_desc *);
void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
/*
* Helper function that does both tree_entry_extract() and update_tree_entry()
* and returns true for success
*/
int tree_entry(struct tree_desc *, struct name_entry *);
+int tree_entry_gently(struct tree_desc *, struct name_entry *);
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors
From: David Turner @ 2016-09-27 15:23 UTC (permalink / raw)
To: git, peff; +Cc: David Turner
From: Jeff King <peff@peff.net>
When the tree-walker runs into an error, it just calls
die(), and the message is always "corrupt tree file".
However, we are actually covering several cases here; let's
give the user a hint about what happened.
Let's also avoid using the word "corrupt", which makes it
seem like the data bit-rotted on disk. Our sha1 check would
already have found that. These errors are ones of data that
is malformed in the first place.
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Jeff King <peff@peff.net>
---
t/t1007-hash-object.sh | 21 +++++++++++++++++++--
tree-walk.c | 12 +++++++-----
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index acca9ac..0691b88 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -183,9 +183,26 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
pop_repo
done
-test_expect_success 'corrupt tree' '
+test_expect_success 'too-short tree' '
echo abc >malformed-tree &&
- test_must_fail git hash-object -t tree malformed-tree
+ test_must_fail git hash-object -t tree malformed-tree 2>err &&
+ test_i18ngrep "too-short tree object" err
+'
+
+test_expect_success 'malformed mode in tree' '
+ hex_sha1=$(echo foo | git hash-object --stdin -w) &&
+ bin_sha1=$(echo $hex_sha1 | perl -ne "printf \"\\\\%03o\", ord for /../g") &&
+ printf "9100644 \0$bin_sha1" >tree-with-malformed-mode &&
+ test_must_fail git hash-object -t tree tree-with-malformed-mode 2>err &&
+ test_i18ngrep "malformed mode in tree entry" err
+'
+
+test_expect_success 'empty filename in tree' '
+ hex_sha1=$(echo foo | git hash-object --stdin -w) &&
+ bin_sha1=$(echo $hex_sha1 | perl -ne "printf \"\\\\%03o\", ord for /../g") &&
+ printf "100644 \0$bin_sha1" >tree-with-empty-filename &&
+ test_must_fail git hash-object -t tree tree-with-empty-filename 2>err &&
+ test_i18ngrep "empty filename in tree entry" err
'
test_expect_success 'corrupt commit' '
diff --git a/tree-walk.c b/tree-walk.c
index ce27842..24f9a0f 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
const char *path;
unsigned int mode, len;
- if (size < 24 || buf[size - 21])
- die("corrupt tree file");
+ if (size < 23 || buf[size - 21])
+ die(_("too-short tree object"));
path = get_mode(buf, &mode);
- if (!path || !*path)
- die("corrupt tree file");
+ if (!path)
+ die(_("malformed mode in tree entry for tree"));
+ if (!*path)
+ die(_("empty filename in tree entry for tree"));
len = strlen(path) + 1;
/* Initialize the descriptor entry */
@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)
unsigned long len = end - (const unsigned char *)buf;
if (size < len)
- die("corrupt tree file");
+ die(_("too-short tree file"));
buf = end;
size -= len;
desc->buffer = buf;
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors
From: David Turner @ 2016-09-27 15:21 UTC (permalink / raw)
To: Jeff King; +Cc: git, mhagger, David Turner
In-Reply-To: <20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net>
On Tue, 2016-09-27 at 01:14 -0400, Jeff King wrote:
> I also wonder if $bin_sha1 should actually be more like:
>
> hex_sha1=$(echo foo | git hash-object --stdin -w)
> bin_sha1=$(echo $hex_sha1 | perl -ne 'printf "\\%3o", ord for /./g')
>
> so that it's a real sha1 (or maybe it is in your original, from an
> object that happens to be in the repo; it's hard to tell). I wouldn't
> expect the code to actually get to the point of looking at the sha1, but
> it's perhaps a more realistic test.
I'm going to do this for this patch, but for the second patch, I don't
think it matters, and it's a bit simpler to avoid it. I guess it's
probably better to test both ways anyway.
^ permalink raw reply
* Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: David Turner @ 2016-09-27 15:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, mhagger, David Turner
In-Reply-To: <20160927052754.bs4frcfy4y7fey62@sigill.intra.peff.net>
On Tue, 2016-09-27 at 01:27 -0400, Jeff King wrote:
> > -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
> > +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
> > {
>
> I know we used the "err" strbuf pattern in the ref code, and it makes
> sense there where we have a lot of different functions with public
> interfaces. But here, we literally just feed the result to die() or
> warning(). I wonder if a nicer interface would be:
>
> typedef void (*err_fn)(const char *, ...);
>
> static int decode_tree_entry(struct tree_desc *desc,
> const char *buf, unsigned long size,
> err_fn err)
> {
> ...
> if (size < 23 || buf[size - 21]) {
> err("too-short tree object");
> return -1;
> }
> }
>
> I dunno. Maybe that is overengineering. I guess we only hit the strbufs
> in the error path (which used to die!), so nobody really cares that much
> about the extra allocation.
I don't really like err_fn because:
(a) without a baton, it's somewhat less general (or less thread-safe)
than the strbuf approach and
(b) with a baton, it's two arguments instead of one.
Thanks for all of the rest of the commentary; I've incorporated it and
will re-roll shortly.
^ permalink raw reply
* Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag
From: Santiago Torres @ 2016-09-27 14:38 UTC (permalink / raw)
To: Philip Oakley; +Cc: git, gitster, peff, sunshine, walters
In-Reply-To: <D5DC2D99E1F84C308DDB99096B0E3BCD@PhilipOakley>
[-- Attachment #1: Type: text/plain, Size: 342 bytes --]
> > static const char * const verify_tag_usage[] = {
> > - N_("git verify-tag [-v | --verbose] <tag>..."),
> > + N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
>
> Does this require a corresponding documentation change? (also 5/5)
>
Yes, I'll work on this while I wait for more reviews.
Thanks!
-Santiago.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* [PATCH] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-09-27 14:31 UTC (permalink / raw)
To: git; +Cc: Alex
In-Reply-To: <87y42ey3z4.fsf@gmail.com>
According to gpg2's doc/DETAILS:
"For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted."
gpg1 ("classic") behaves the same (although doc/DETAILS
differs).
Currently, we parse gpg's status output for GOODSIG, BADSIG and trust
information and translate that into status codes G, B, U, N for the %G?
format specifier.
git-verify-* returns success in the GOODSIG case only. This is somewhat in
disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG,
but we err on the very safe side.
Introduce additional status codes E, X, R for ERRSIG, EXP*SIG, REVKEYSIG
so that a user of %G? gets more information about the absence of a 'G'
on first glance.
Reported-by: Alex <agrambot@gmail.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I'd be happy to learn are more portable/safer/cooler way to make gpg forget
that key in the added test...
Documentation/pretty-formats.txt | 9 +++++++--
gpg-interface.c | 4 ++++
pretty.c | 3 +++
t/t7510-signed-commit.sh | 11 ++++++++++-
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a942d57..806b47f 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,13 @@ ifndef::git-rev-list[]
- '%N': commit notes
endif::git-rev-list[]
- '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
- "U" for a good signature with unknown validity and "N" for no signature
+- '%G?': show "G" for a good (rather: valid) signature,
+ "B" for a bad signature,
+ "U" for a good signature with unknown validity,
+ "X" for a good expired signature, or good signature made by an expired key,
+ "R" for a good signature made by a revoked key,
+ "E" if the signature cannot be checked (e.g. missing key)
+ 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}` or
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..8a3e245 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -33,6 +33,10 @@ static struct {
{ 'B', "\n[GNUPG:] BADSIG " },
{ 'U', "\n[GNUPG:] TRUST_NEVER" },
{ 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
+ { 'E', "\n[GNUPG:] ERRSIG "},
+ { 'X', "\n[GNUPG:] EXPSIG "},
+ { 'X', "\n[GNUPG:] EXPKEYSIG "},
+ { 'R', "\n[GNUPG:] REVKEYSIG "},
};
void parse_gpg_output(struct signature_check *sigc)
diff --git a/pretty.c b/pretty.c
index 493edb0..39a36cd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1232,8 +1232,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
switch (c->signature_check.result) {
case 'G':
case 'B':
+ case 'E':
case 'U':
case 'N':
+ case 'X':
+ case 'R':
strbuf_addch(sb, c->signature_check.result);
}
break;
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e839f5..fd22742 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -190,7 +190,7 @@ test_expect_success GPG 'show bad signature with custom format' '
test_cmp expect actual
'
-test_expect_success GPG 'show unknown signature with custom format' '
+test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
61092E85B7227189
@@ -200,6 +200,15 @@ test_expect_success GPG 'show unknown signature with custom format' '
test_cmp expect actual
'
+test_expect_success GPG 'show unknown signature with custom format' '
+ cat >expect <<-\EOF &&
+ E
+ 61092E85B7227189
+ EOF
+ GNUPGHOME=/dev/null git log -1 --format="%G?%n%GK" eighth-signed-alt >actual &&
+ test_cmp expect actual
+'
+
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
--
2.10.0.527.gbcb6904
^ permalink raw reply related
* [PATCH] rev-list-options: clarify the usage of -n/--max-number
From: Pranit Bauva @ 2016-09-27 14:10 UTC (permalink / raw)
To: git
In-Reply-To: <201609271240.19759.sweet_f_a@gmx.de>
-n=<number>, -<number>, --max-number=<number> shows the last n commits
specified in <number> irrespective of whether --reverse is used or not.
With --reverse, it just shows the last n commits in reverse order.
Reported-by: Ruediger Meier <sweet_f_a@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
Hey Ruegiger,
The description is a bit inappropriate for --max-count and thus this
patch.
I cannot comment whether --max-count=-n would be a good choice or not
because personally I never left the need of it. I normally use --reverse
so as to review my patches in a branch serially. So for me the current
usage of --reverse seems more appropriate.
---
Documentation/rev-list-options.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 7e462d3..6b7c2e5 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -18,7 +18,7 @@ ordering and formatting options, such as `--reverse`.
-<number>::
-n <number>::
--max-count=<number>::
- Limit the number of commits to output.
+ Limit to last n number of commits to output specified in <number>.
--skip=<number>::
Skip 'number' commits before starting to show the commit output.
--
https://github.com/git/git/pull/296
^ permalink raw reply related
* Re: git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Paul Smith @ 2016-09-27 13:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <CAPc5daU_nnHRjtC02bxqRaoU+0Rgi7pS6e912Fqk-Xy=qdKWFA@mail.gmail.com>
On Mon, 2016-09-26 at 14:57 -0700, Junio C Hamano wrote:
> On Mon, Sep 26, 2016 at 2:32 PM, Paul Smith <paul@mad-scientist.net> wrote:
> >
> > Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
> > so I can copy it anywhere and it continues to run without any problem.
> > This is on GNU/Linux systems, FWIW.
>
> I had an impression that the setting was only to support MS Windows.
Hm. You may be right. If so that's too bad, because a relocatable Git
is very handy even on UNIX systems. Is there a reason for invoking the
subcommands by providing the plain command ("fetch", "merge-base") as
argv[0], rather than giving the fully-qualified path to a Git command?
^ permalink raw reply
* Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"
From: Michael Haggerty @ 2016-09-27 13:18 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160927043733.u3emlanbipu2cn5h@sigill.intra.peff.net>
On 09/27/2016 06:37 AM, Jeff King wrote:
> Commit e8adf23 (xdl_change_compact(): introduce the concept
> of a change group, 2016-08-22) added a "struct group" type
> to xdiff/xdiffi.c. But the POSIX system header "grp.h"
> already defines "struct group" (it is part of the getgrnam
> interface). This happens to work because the new type is
> local to xdiffi.c, and the xdiff code includes a relatively
> small set of system headers. But it will break compilation
> if xdiff ever switches to using git-compat-util.h. It can
> also probably cause confusion with tools that look at the
> whole code base, like coccinelle or ctags.
>
> Let's resolve by giving the xdiff variant a scoped name,
> which is closer to other xdiff types anyway (e.g.,
> xdlfile_t, though note that xdiff is fond if typedefs when
> Git usually is not).
Makes sense to me. I didn't try to adhere to xdiff conventions too
tightly because I don't think that project is alive anymore, so I don't
expect we'll be upstreaming anything [1]. But this change definitely
makes sense.
Thanks,
Michael
[1] Though I've since learned that libgit2 also bases their diff code on
xdiff, so if we avoid changing things gratuitously there is more chance
that our two projects can benefit from each other's improvements
whenever they are also licensed compatibly.
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jeff King @ 2016-09-27 12:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CA+55aFyfvvqq1c=hZcuL-yPavp2tjzx8r3bFJnMY7DAE7YcB=Q@mail.gmail.com>
On Mon, Sep 26, 2016 at 09:36:23AM -0700, Linus Torvalds wrote:
> 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".
You can basically get that by using "1234^{commit}" all the time, as
that turns on the committish disambiguator function (though it's not
quite the same, as it would pick a tag, too; you really want the
commit-only disambiguator). But presumably you'd want it on all the
time. See the patch below, which lets you do:
git config --global core.disambiguate commit
and I think should do what you want. I'm up in the air on whether it is
a good idea or not, but then I do not usually run into ambiguous sha1s.
> And then have an explicit command (or flag) to do disambiguation for
> when you explicitly want it.
In my patch you can tweak the config variable off, though it might make
sense to also have some per-short-sha1 syntax.
> 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.
I think I do sometimes "git show $blob_sha1" based on a diff index line.
OTOH, I don't think of though as "long-term" references. I'm usually
trying to apply the patch at the time, so it's fairly fresh (it's true
that the short-sha1 may have been generated on the sender's side, who
has fewer objects, but I doubt that's a big problem in general; the real
issue is that it was unique at one point, and isn't a few years later).
But more importantly, any fallback like this should take a backseat to
context provided by the rest of git. So for instance, the index-building
in "am -3" uses the blob disambiguator, and should continue to do so
(and does with my patch).
> 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.
By the same rule I just mentioned above, if you use the short sha1 in a
treeish context, it will look for any treeish (so "1234:foo" would
continue to look for any treeish, not just a commit). So that might not
be as desirable, but I think it does make sense (and of course it will
still tell you immediately what the options are, and you can decide what
to do).
-- >8 --
Subject: [PATCH] get_short_sha1: make default disambiguation configurable
When we find ambiguous short sha1s, we may get a
disambiguation rule from our caller's context. But if we
don't, we fall back to treating all sha1s the same, even
though most projects will tend to refer only to commits by
their short sha1s.
This patch introduces a configuration option that lets the
user pick a different fallback (e.g., only commits). It's
possible that we may want to make this the default, but it's
a good idea to start as a config option for two reasons:
1. It lets people experiment with this and see if it's a
good idea (i.e., the "tend to" above is an assumption;
we don't really know if this will break some obscure
cases).
2. Even if we do flip the default, it gives people an
escape hatch if it causes problems (you can sometimes
override it by asking for "1234^{tree}", but not all
combinations are possible).
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 2 ++
config.c | 3 +++
sha1_name.c | 32 ++++++++++++++++++++++++++++++++
t/t1512-rev-parse-disambiguation.sh | 14 ++++++++++++++
4 files changed, 51 insertions(+)
diff --git a/cache.h b/cache.h
index 5df0f33..b9583c4 100644
--- a/cache.h
+++ b/cache.h
@@ -1224,6 +1224,8 @@ extern int get_oid(const char *str, struct object_id *oid);
typedef int each_abbrev_fn(const unsigned char *sha1, void *);
extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
+extern int set_disambiguate_hint_config(const char *var, const char *value);
+
/*
* Try to read a SHA1 in hexadecimal format from the 40 characters
* starting at hex. Write the 20-byte result to sha1 in binary form.
diff --git a/config.c b/config.c
index 1e4b617..83fdecb 100644
--- a/config.c
+++ b/config.c
@@ -841,6 +841,9 @@ static int git_default_core_config(const char *var, const char *value)
return 0;
}
+ if (!strcmp(var, "core.disambiguate"))
+ return set_disambiguate_hint_config(var, value);
+
if (!strcmp(var, "core.loosecompression")) {
int level = git_config_int(var, value);
if (level == -1)
diff --git a/sha1_name.c b/sha1_name.c
index 0513f14..3b647fd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -283,6 +283,36 @@ static int disambiguate_blob_only(const unsigned char *sha1, void *cb_data_unuse
return kind == OBJ_BLOB;
}
+static disambiguate_hint_fn default_disambiguate_hint;
+
+int set_disambiguate_hint_config(const char *var, const char *value)
+{
+ static const struct {
+ const char *name;
+ disambiguate_hint_fn fn;
+ } hints[] = {
+ { "none", NULL },
+ { "commit", disambiguate_commit_only },
+ { "committish", disambiguate_committish_only },
+ { "tree", disambiguate_tree_only },
+ { "treeish", disambiguate_treeish_only },
+ { "blob", disambiguate_blob_only }
+ };
+ int i;
+
+ if (!value)
+ return config_error_nonbool(var);
+
+ for (i = 0; i < ARRAY_SIZE(hints); i++) {
+ if (!strcasecmp(value, hints[i].name)) {
+ default_disambiguate_hint = hints[i].fn;
+ return 0;
+ }
+ }
+
+ return error("unknown hint type for '%s': %s", var, value);
+}
+
static int init_object_disambiguation(const char *name, int len,
struct disambiguate_state *ds)
{
@@ -373,6 +403,8 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
ds.fn = disambiguate_treeish_only;
else if (flags & GET_SHA1_BLOB)
ds.fn = disambiguate_blob_only;
+ else
+ ds.fn = default_disambiguate_hint;
find_short_object_filename(&ds);
find_short_packed_object(&ds);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index c5447ef..7c659eb 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -347,4 +347,18 @@ test_expect_success C_LOCALE_OUTPUT 'failed type-selector still shows hint' '
test_line_count = 3 hints
'
+test_expect_success 'core.disambiguate config can prefer types' '
+ # ambiguous between tree and tag
+ sha1=0000000000f &&
+ test_must_fail git rev-parse $sha1 &&
+ git rev-parse $sha1^{commit} &&
+ git -c core.disambiguate=committish rev-parse $sha1
+'
+
+test_expect_success 'core.disambiguate does not override context' '
+ # treeish ambiguous between tag and tree
+ test_must_fail \
+ git -c core.disambiguate=committish rev-parse $sha1^{tree}
+'
+
test_done
--
2.10.0.564.g318c4ae
^ permalink raw reply related
* Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Duy Nguyen @ 2016-09-27 12:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git Mailing List, Thomas Braun
In-Reply-To: <CACsJy8A_CWgcS5za-Dha6Khgd6HqHk9UWHq7qqAeR-kns-syXg@mail.gmail.com>
On Tue, Sep 27, 2016 at 5:58 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> but then you also have to change the type of xdl_opts
>> to uint64_t, which in turn means that you will have to change the
>> definition of xpparam_t's "flags" field from unsigned long to uint64_t.
>
> I miss a connection here. This new flag is intended to be used in
> "flags" field in struct diff_options. Is there any chance it can be
> set on xdl_opts (of the same struct, I assume)?
>
>> Maybe this can be avoided?
>
> I don't see a good way to avoid it. We normally enable or disable diff
> features as bit flags and now we run out of bits. Adding something
> like "flags2" works, but not pretty. Any suggestion is welcome.
Never mind. I think I found some way that does not look particularly bad.
--
Duy
^ permalink raw reply
* Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Jeff King @ 2016-09-27 12:13 UTC (permalink / raw)
To: Lars Schneider
Cc: Jakub Narębski, git, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <AF15F1E5-4DA0-459F-84EE-EDCB5FA23EF7@gmail.com>
On Tue, Sep 27, 2016 at 02:10:50PM +0200, Lars Schneider wrote:
> > That being said, why don't you just use LARGE_PACKET_MAX here? It is
> > already the accepted size for feeding to packet_read(), and we know it
> > has enough space to hold a NUL terminator. Yes, we may over-allocate by
> > 4 bytes, but that isn't really relevant. Strbufs over-allocate anyway.
>
> TBH in that case I would prefer the "PKTLINE_DATA_MAXLEN+1" solution with
> an additional comment explaining "+1".
>
> Would that be OK for you?
>
> I am not worried about the extra 4 bytes. I am worried that we make it harder
> to see what is going on if we use LARGE_PACKET_MAX.
I guess I don't feel to strongly either way. My interest in
LARGE_PACKET_MAX is mostly that this is how all the rest of the
packet_read() callers behave.
-Peff
^ permalink raw reply
* Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Lars Schneider @ 2016-09-27 12:10 UTC (permalink / raw)
To: Jeff King
Cc: Jakub Narębski, git, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160927090031.jjb3dmhspbbnizja@sigill.intra.peff.net>
> On 27 Sep 2016, at 11:00, Jeff King <peff@peff.net> wrote:
>
> On Tue, Sep 27, 2016 at 10:14:16AM +0200, Lars Schneider wrote:
>
>>>>> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
>>>>> + paket_len = packet_read(fd_in, NULL, NULL,
>>>>> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
>> [...]
>> After looking at it with fresh eyes I think the existing code is probably correct,
>> but maybe a bit confusing.
>>
>> packet_read() adds a '\0' at the end of the destination buffer:
>> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206
>>
>> That is why the destination buffer needs to be one byte larger than the expected content.
>>
>> However, in this particular case that wouldn't be necessary because the destination
>> buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we are not
>> supposed to write to this extra byte:
>> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31
>
> Right. The allocation happens as part of strbuf_grow(), but whatever
> fills the buffer is expected to write the actual NUL (either manually,
> or by calling strbuf_setlen().
>
> I see the bit you quoted warns not to touch the extra byte yourself,
> though I wonder if that is a bit heavy-handed (I guess it would matter
> if we moved the extra 1-byte growth into strbuf_setlen(), but I find
> that a rather unlikely change).
>
> That being said, why don't you just use LARGE_PACKET_MAX here? It is
> already the accepted size for feeding to packet_read(), and we know it
> has enough space to hold a NUL terminator. Yes, we may over-allocate by
> 4 bytes, but that isn't really relevant. Strbufs over-allocate anyway.
TBH in that case I would prefer the "PKTLINE_DATA_MAXLEN+1" solution with
an additional comment explaining "+1".
Would that be OK for you?
I am not worried about the extra 4 bytes. I am worried that we make it harder
to see what is going on if we use LARGE_PACKET_MAX.
> So just:
>
> for (;;) {
> strbuf_grow(sb_out, LARGE_PACKET_MAX);
> packet_len = packet_read(fd_in, NULL, NULL,
> sb_out->buf + sb_out->len, LARGE_PACKET_MAX,
> options);
> if (packet_len <= 0)
> break;
> /*
> * no need for strbuf_setlen() here; packet_read always adds a
> * NUL terminator.
> */
> sb_out->len += packet_len;
> }
>
> You _could_ make the final line of the loop use strbuf_setlen(); it
> would just overwrite something we already know is a NUL (and we know
> that no extra allocation is necessary).
>
> Also, using LARGE_PACKET_MAX fixes the fact that this patch is using
> PKTLINE_DATA_MAXLEN before it is actually defined. :)
Yeah, I noticed that too and fixed it in v9 :-) Thanks for the reminder!
> You might want to occasionally run:
>
> git rebase -x make
>
> to make sure all of your incremental steps are valid (or even "make
> test" if you are extremely patient; I often do that once after a big
> round of complex interactive-rebase reordering).
That is a good suggestion. I'll add that to my "tool-belt" :-)
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 2/4] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
From: Duy Nguyen @ 2016-09-27 10:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git Mailing List, Thomas Braun
In-Reply-To: <alpine.DEB.2.20.1606091815310.2680@virtualbox>
(sorry for a very late reply, I'm just picking this series up again)
On Thu, Jun 9, 2016 at 11:18 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Duy,
>
> On Mon, 6 Jun 2016, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/diff.h b/diff.h
>> index b497078..9e42556 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -92,6 +92,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>> #define DIFF_OPT_FUNCCONTEXT (1 << 29)
>> #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
>> #define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
>> +#define DIFF_OPT_SHIFT_INTENT_TO_ADD (1UL << 32)
>
> I am afraid that this is not enough. On Windows, sizeof(unsigned long) is
> 4 (and it is perfectly legal). That means that you would have to use at
> least (1ULL << 32),
OK.
> but then you also have to change the type of xdl_opts
> to uint64_t, which in turn means that you will have to change the
> definition of xpparam_t's "flags" field from unsigned long to uint64_t.
I miss a connection here. This new flag is intended to be used in
"flags" field in struct diff_options. Is there any chance it can be
set on xdl_opts (of the same struct, I assume)?
> Maybe this can be avoided?
I don't see a good way to avoid it. We normally enable or disable diff
features as bit flags and now we run out of bits. Adding something
like "flags2" works, but not pretty. Any suggestion is welcome.
--
Duy
^ permalink raw reply
* question about git rev-list --max-count=n
From: Ruediger Meier @ 2016-09-27 10:40 UTC (permalink / raw)
To: git
Hi,
git rev-list --max-count=n
seems to always list the _last_ (newest) n commits. Is there any
functionality to list the _first_ n commits?
I've tried to add --reverse hoping that this would do it but it does
not.
The manual could be a bit more clear about that:
--max-count=<number> Limit the number of commits to output.
If it really would only limit the _output_ (like head -n), then
IMO --reverse should do what I want.
Regarding my initial question. Maybe we could support
--max-count=-n
to list the first n commits.
cu,
Rudi
^ permalink raw reply
* Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-27 10:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Swift Geek, Jeff King
In-Reply-To: <xmqq7f9ypag4.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 03:23:23PM -0700, Junio C Hamano wrote:
> Kevin Daudt <me@ikke.info> writes:
>
> > On Mon, Sep 26, 2016 at 12:26:13PM -0700, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > Don't these also need to be downcased if you prefer $data over
> >> > $DATA, though?
> >>
> >> For now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to
> >> 1/2 between your 1/2 and 2/2.
> >
> > Ugh, thanks. I'd replaced it in the first patch, but forgot it in the
> > second.
>
> Heh, I already guessed that much that these were sent without even
> be in the tree, after editing only the patch files. Don't do that
> ;-)
That was just my poor wording. I did a proper rebase, but only changed
it for the first commit.
^ permalink raw reply
* RE: git-upload-pack hangs
From: Jason Pyeron @ 2016-09-27 10:27 UTC (permalink / raw)
To: git
In-Reply-To: <CAPc5daVdnuEfgNaeaGCymd3QWX7kfO3JQutVWmPOv1iMMzqCcA@mail.gmail.com>
Thanks, I wrote this while on a train to get the site up 1st, I did not even review it yet!
I think I am going to spend the time this weekend to clean it up, and submit a patch for master and the version cygwin uses.
-Jason
> -----Original Message-----
> From: jch2355@gmail.com [mailto:jch2355@gmail.com] On Behalf
> Of Junio C Hamano
> Sent: Tuesday, September 27, 2016 00:18
> To: Jason Pyeron
> Subject: Re: git-upload-pack hangs
>
> Not a review, but size_t is an unsigned type, so specifying -1 as a
> fallback value,
> or comparing it with -1, does not make much sense. Perhaps
> use ssize_t?
>
> On Mon, Sep 26, 2016 at 8:45 PM, Jason Pyeron
> <jpyeron@pdinc.us> wrote:
> > This is a very, very first draft.
> >
> > It is allowing IIS to work right now.
> >
> > I still need to address chunked issues, where there is no
> content length (see
> http://www.gossamer-threads.com/lists/apache/users/373042)
> >
> > Any comments, sugestions?
> >
> > -Jason
> >
> > --- ./origsrc/git-v2.8.3/http-backend.c 2016-05-18
> 18:32:41.000000000 -0400
> > +++ ./src/git-v2.8.3/http-backend.c 2016-09-26
> 22:52:02.636135000 -0400
> > @@ -279,14 +279,17 @@
> > {
> > size_t len = 0, alloc = 8192;
> > unsigned char *buf = xmalloc(alloc);
> > + /* get request size */
> > + size_t req_len = git_env_ulong("CONTENT_LENGTH", -1);
> >
> > if (max_request_buffer < alloc)
> > max_request_buffer = alloc;
> >
> > - while (1) {
> > + while (req_len>0 || req_len==-1 ) {
> > + ssize_t maxread=alloc>req_len &&
> req_len!=-1?req_len:alloc;
> > ssize_t cnt;
> >
> > - cnt = read_in_full(fd, buf + len, alloc - len);
> > + cnt = read_in_full(fd, buf + len, maxread - len);
> > if (cnt < 0) {
> > free(buf);
> > return -1;
> > @@ -294,13 +297,19 @@
> >
> > /* partial read from read_in_full means we
> hit EOF */
> > len += cnt;
> > - if (len < alloc) {
> > + if (len < maxread) {
> > *out = buf;
> > return len;
> > }
> >
> > + if (req_len>0) {
> > + req_len -= cnt;
> > + if (req_len<0)
> > + req_len=0;
> > + }
> > +
> > /* otherwise, grow and try again (if we can) */
> > - if (alloc == max_request_buffer)
> > + if (alloc == max_request_buffer && maxread == alloc)
> > die("request was larger than our
> maximum size (%lu);"
> > " try setting
> GIT_HTTP_MAX_REQUEST_BUFFER",
> > max_request_buffer);
> > @@ -310,6 +319,10 @@
> > alloc = max_request_buffer;
> > REALLOC_ARRAY(buf, alloc);
> > }
> > +
> > + free(buf);
> > +
> > + return len;
> > }
> >
> > static void inflate_request(const char *prog_name, int
> out, int buffer_input)
> >
> >
> >> -----Original Message-----
> >> From: git-owner@vger.kernel.org
> >> [mailto:git-owner@vger.kernel.org] On Behalf Of Jason Pyeron
> >> Sent: Monday, September 26, 2016 09:26
> >> To: git@vger.kernel.org
> >> Subject: RE: git-upload-pack hangs
> >>
> >> > -----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: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Jeff King @ 2016-09-27 9:00 UTC (permalink / raw)
To: Lars Schneider
Cc: Jakub Narębski, git, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <6D8B988C-8E54-4AA6-980C-A6BA40983D88@gmail.com>
On Tue, Sep 27, 2016 at 10:14:16AM +0200, Lars Schneider wrote:
> >>> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
> >>> + paket_len = packet_read(fd_in, NULL, NULL,
> >>> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
> [...]
> After looking at it with fresh eyes I think the existing code is probably correct,
> but maybe a bit confusing.
>
> packet_read() adds a '\0' at the end of the destination buffer:
> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206
>
> That is why the destination buffer needs to be one byte larger than the expected content.
>
> However, in this particular case that wouldn't be necessary because the destination
> buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we are not
> supposed to write to this extra byte:
> https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31
Right. The allocation happens as part of strbuf_grow(), but whatever
fills the buffer is expected to write the actual NUL (either manually,
or by calling strbuf_setlen().
I see the bit you quoted warns not to touch the extra byte yourself,
though I wonder if that is a bit heavy-handed (I guess it would matter
if we moved the extra 1-byte growth into strbuf_setlen(), but I find
that a rather unlikely change).
That being said, why don't you just use LARGE_PACKET_MAX here? It is
already the accepted size for feeding to packet_read(), and we know it
has enough space to hold a NUL terminator. Yes, we may over-allocate by
4 bytes, but that isn't really relevant. Strbufs over-allocate anyway.
So just:
for (;;) {
strbuf_grow(sb_out, LARGE_PACKET_MAX);
packet_len = packet_read(fd_in, NULL, NULL,
sb_out->buf + sb_out->len, LARGE_PACKET_MAX,
options);
if (packet_len <= 0)
break;
/*
* no need for strbuf_setlen() here; packet_read always adds a
* NUL terminator.
*/
sb_out->len += packet_len;
}
You _could_ make the final line of the loop use strbuf_setlen(); it
would just overwrite something we already know is a NUL (and we know
that no extra allocation is necessary).
Also, using LARGE_PACKET_MAX fixes the fact that this patch is using
PKTLINE_DATA_MAXLEN before it is actually defined. :)
You might want to occasionally run:
git rebase -x make
to make sure all of your incremental steps are valid (or even "make
test" if you are extremely patient; I often do that once after a big
round of complex interactive-rebase reordering).
> I see two options:
>
>
> (1) I leave the +1 as is and add a comment why the extra byte is necessary.
>
> Pro: No change in existing code necessary
> Con: The destination buffer has two '\0' at the end.
>
>
> (2) I add an option PACKET_READ_DISABLE_NUL_TERMINATION. If the option is
> set then no '\0' byte is added to the end.
>
> Pro: Correct solution, no byte wasted.
> Con: Change in existing code required.
>
>
> Any preference?
Of the two, I prefer (1), though I like what I suggested above even more
(big surprise, I know).
-Peff
^ permalink raw reply
* Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()
From: Jeff King @ 2016-09-27 8:39 UTC (permalink / raw)
To: Lars Schneider
Cc: Jakub Narębski, git, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <4CF26590-7B88-4DB6-B6D4-A6CFE0FD6ABF@gmail.com>
On Mon, Sep 26, 2016 at 09:21:10PM +0200, Lars Schneider wrote:
> On 25 Sep 2016, at 13:26, 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>
> >> ...
> >>
> >> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> >
> > I'm not sure what naming convention the rest of Git uses, but isn't
> > it more like '*data' rather than '*buf' here?
>
> pkt-line seems to use 'buf' or 'buffer' for everything else.
I do not think we have definite rules, but I would generally expect to
see "data" as an opaque thing (e.g., passing "void *data" to callbacks).
"buf" or "buffer" makes sense here, but I don't think it really matters
that much either way.
> >> + static char packet_write_buffer[LARGE_PACKET_MAX];
> >
> > I think there should be warning (as a comment before function
> > declaration, or before function definition), that packet_write_gently()
> > is not thread-safe (nor reentrant, but the latter does not matter here,
> > I think).
> >
> > Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
> >
> > This is not something terribly important; I guess git code has tons
> > of functions not marked as thread-unsafe...
>
> I agree that the function is not thread-safe. However, I can't find
> an example in the Git source that marks a function as not thread-safe.
> Unless is it explicitly stated in the coding guidelines I would prefer
> not to start way to mark functions.
I'd agree. A large number of functions in git are not reentrant, and I
would not want to give the impression that those missing a warning are
safe to use.
> >> + if (size > sizeof(packet_write_buffer) - 4) {
> >
> > First, wouldn't the following be more readable:
> >
> > + if (size + 4 > LARGE_PACKET_MAX) {
>
> Peff suggested that here:
> http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy@sigill.intra.peff.net/
There is a good reason to do size checks as a subtraction from a known
quantity: you can be sure that you are not introducing an overflow
(e.g., Jakub's suggestion does the wrong thing when "size" is within 4
bytes of its maximum value). That's unlikely in this case, but then so
is the size exceeding LARGE_PACKET_MAX in the first place (arguably this
should be a die("BUG"), because it is the caller's responsibility to
split their packets.
-Peff
^ permalink raw reply
* [PATCH v5] revision: new rev^-n shorthand for rev^n..rev
From: Vegard Nossum @ 2016-09-27 8:32 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Santi Béjar, Kevin Bracey, Philip Oakley,
Matthieu Moy, Ramsay Jones, Jakub Narębski, Jeff King,
Vegard Nossum
"git log rev^..rev" is commonly used to show all work done on and merged
from a side branch. This patch introduces a shorthand "rev^-" for this
and additionally allows "rev^-$n" to mean "reachable from rev, excluding
what is reachable from the nth parent of rev". For example, for a
two-parent merge, you can use rev^-2 to get the set of commits which were
made to the main branch while the topic branch was prepared.
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
[v2: Use ^- instead of % as suggested by Junio Hamano and use some
common helper functions for parsing.]
[v3: Use 'struct object_id' instead of 'char[20]' and add some tests as
suggested by Matthieu Moy; fix missing '-' in Documentation/revisions.txt
as suggested by Ramsay Jones; misc changelog + documentation fixes as
suggested by Philip Oakley.]
[v4: Documentation fixes and parsing rework suggested by Junio Hamano
and add some more tests.]
[v5: count parents before showing anything, misc testing changes, and
changelog shortening as suggested by Junio Hamano, parent counting
changes suggested by Jeff King.]
---
Documentation/revisions.txt | 17 +++++++-
builtin/rev-parse.c | 54 +++++++++++++++++++------
revision.c | 34 ++++++++++++++--
t/t6101-rev-parse-parents.sh | 94 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 180 insertions(+), 19 deletions(-)
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 4bed5b1..ba11b9c 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -283,7 +283,7 @@ empty range that is both reachable and unreachable from HEAD.
Other <rev>{caret} Parent Shorthand Notations
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Two other shorthands exist, particularly useful for merge commits,
+Three other shorthands exist, particularly useful for merge commits,
for naming a set that is formed by a commit and its parent commits.
The 'r1{caret}@' notation means all parents of 'r1'.
@@ -291,8 +291,15 @@ The 'r1{caret}@' notation means all parents of 'r1'.
The 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents.
By itself, this notation denotes the single commit 'r1'.
+The '<rev>{caret}-{<n>}' notation includes '<rev>' but excludes the <n>th
+parent (i.e. a 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
+that was merged in merge commit '<commit>' (including '<commit>'
+itself).
+
While '<rev>{caret}<n>' was about specifying a single commit parent, these
-two notations consider all its parents. For example you can say
+three notations also consider its parents. For example you can say
'HEAD{caret}2{caret}@', however you cannot say 'HEAD{caret}@{caret}2'.
Revision Range Summary
@@ -326,6 +333,10 @@ Revision Range Summary
as giving commit '<rev>' and then all its parents prefixed with
'{caret}' to exclude them (and their ancestors).
+'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
+ Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
+ given.
+
Here are a handful of examples using the Loeliger illustration above,
with each step in the notation's expansion and selection carefully
spelt out:
@@ -339,6 +350,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^1 B E I J F B
C^@ = C^1
= F I J F
B^@ = B^1 B^2 B^3
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 76cf05e..4da1f1d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)
unsigned char sha1[20];
struct commit *commit;
struct commit_list *parents;
- int parents_only;
-
- if ((dotdot = strstr(arg, "^!")))
- parents_only = 0;
- else if ((dotdot = strstr(arg, "^@")))
- parents_only = 1;
-
- if (!dotdot || dotdot[2])
+ int parent_number;
+ int include_rev = 0;
+ int include_parents = 0;
+ int exclude_parent = 0;
+
+ if ((dotdot = strstr(arg, "^!"))) {
+ include_rev = 1;
+ if (dotdot[2])
+ return 0;
+ } else if ((dotdot = strstr(arg, "^@"))) {
+ include_parents = 1;
+ if (dotdot[2])
+ return 0;
+ } else if ((dotdot = strstr(arg, "^-"))) {
+ include_rev = 1;
+ exclude_parent = 1;
+
+ if (dotdot[2]) {
+ char *end;
+ exclude_parent = strtoul(dotdot + 2, &end, 10);
+ if (*end != '\0' || !exclude_parent)
+ return 0;
+ }
+ } else
return 0;
*dotdot = 0;
@@ -314,12 +330,24 @@ static int try_parent_shorthands(const char *arg)
return 0;
}
- if (!parents_only)
- show_rev(NORMAL, sha1, arg);
commit = lookup_commit_reference(sha1);
- for (parents = commit->parents; parents; parents = parents->next)
- show_rev(parents_only ? NORMAL : REVERSED,
- parents->item->object.oid.hash, arg);
+ if (exclude_parent &&
+ exclude_parent > commit_list_count(commit->parents)) {
+ *dotdot = '^';
+ return 0;
+ }
+
+ if (include_rev)
+ show_rev(NORMAL, sha1, arg);
+ for (parents = commit->parents, parent_number = 1;
+ parents;
+ parents = parents->next, parent_number++) {
+ if (exclude_parent && parent_number != exclude_parent)
+ continue;
+
+ show_rev(include_parents ? NORMAL : REVERSED,
+ parents->item->object.oid.hash, arg);
+ }
*dotdot = '^';
return 1;
diff --git a/revision.c b/revision.c
index 969b3d1..b37dbec 100644
--- a/revision.c
+++ b/revision.c
@@ -1289,12 +1289,14 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
}
}
-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
+ int exclude_parent)
{
unsigned char sha1[20];
struct object *it;
struct commit *commit;
struct commit_list *parents;
+ int parent_number;
const char *arg = arg_;
if (*arg == '^') {
@@ -1316,7 +1318,15 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
if (it->type != OBJ_COMMIT)
return 0;
commit = (struct commit *)it;
- for (parents = commit->parents; parents; parents = parents->next) {
+ if (exclude_parent &&
+ exclude_parent > commit_list_count(commit->parents))
+ return 0;
+ for (parents = commit->parents, parent_number = 1;
+ parents;
+ parents = parents->next, parent_number++) {
+ if (exclude_parent && parent_number != exclude_parent)
+ continue;
+
it = &parents->item->object;
it->flags |= flags;
add_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);
@@ -1519,17 +1529,33 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
}
*dotdot = '.';
}
+
dotdot = strstr(arg, "^@");
if (dotdot && !dotdot[2]) {
*dotdot = 0;
- if (add_parents_only(revs, arg, flags))
+ if (add_parents_only(revs, arg, flags, 0))
return 0;
*dotdot = '^';
}
dotdot = strstr(arg, "^!");
if (dotdot && !dotdot[2]) {
*dotdot = 0;
- if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM)))
+ if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))
+ *dotdot = '^';
+ }
+ dotdot = strstr(arg, "^-");
+ if (dotdot) {
+ int exclude_parent = 1;
+
+ if (dotdot[2]) {
+ char *end;
+ exclude_parent = strtoul(dotdot + 2, &end, 10);
+ if (*end != '\0' || !exclude_parent)
+ return -1;
+ }
+
+ *dotdot = 0;
+ if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
*dotdot = '^';
}
diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
index 1c6952d..64a9850 100755
--- a/t/t6101-rev-parse-parents.sh
+++ b/t/t6101-rev-parse-parents.sh
@@ -102,4 +102,98 @@ test_expect_success 'short SHA-1 works' '
test_cmp_rev_output start "git rev-parse ${start%?}"
'
+# rev^- tests; we can use a simpler setup for these
+
+test_expect_success 'setup for rev^- tests' '
+ test_commit one &&
+ test_commit two &&
+ test_commit three &&
+
+ # Merge in a branch for testing rev^-
+ git checkout -b branch &&
+ git checkout HEAD^^ &&
+ git merge -m merge --no-edit --no-ff branch &&
+ git checkout -b merge
+'
+
+# The merged branch has 2 commits + the merge
+test_expect_success 'rev-list --count merge^- = merge^..merge' '
+ git rev-list --count merge^..merge >expect &&
+ echo 3 >actual &&
+ test_cmp expect actual
+'
+
+# All rev^- rev-parse tests
+
+test_expect_success 'rev-parse merge^- = merge^..merge' '
+ git rev-parse merge^..merge >expect &&
+ git rev-parse merge^- >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse merge^-1 = merge^..merge' '
+ git rev-parse merge^1..merge >expect &&
+ git rev-parse merge^-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse merge^-2 = merge^2..merge' '
+ git rev-parse merge^2..merge >expect &&
+ git rev-parse merge^-2 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse merge^-0 (invalid parent)' '
+ test_must_fail git rev-parse merge^-0
+'
+
+test_expect_success 'rev-parse merge^-3 (invalid parent)' '
+ test_must_fail git rev-parse merge^-3
+'
+
+test_expect_success 'rev-parse merge^-^ (garbage after ^-)' '
+ test_must_fail git rev-parse merge^-^
+'
+
+test_expect_success 'rev-parse merge^-1x (garbage after ^-1)' '
+ test_must_fail git rev-parse merge^-1x
+'
+
+# All rev^- rev-list tests (should be mostly the same as rev-parse; the reason
+# for the duplication is that rev-parse and rev-list use different parsers).
+
+test_expect_success 'rev-list merge^- = merge^..merge' '
+ git rev-list merge^..merge >expect &&
+ git rev-list merge^- >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-list merge^-1 = merge^1..merge' '
+ git rev-list merge^1..merge >expect &&
+ git rev-list merge^-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-list merge^-2 = merge^2..merge' '
+ git rev-list merge^2..merge >expect &&
+ git rev-list merge^-2 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-list merge^-0 (invalid parent)' '
+ test_must_fail git rev-list merge^-0
+'
+
+test_expect_success 'rev-list merge^-3 (invalid parent)' '
+ test_must_fail git rev-list merge^-3
+'
+
+test_expect_success 'rev-list merge^-^ (garbage after ^-)' '
+ test_must_fail git rev-list merge^-^
+'
+
+test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '
+ test_must_fail git rev-list merge^-1x
+'
+
test_done
--
2.10.0.rc0.1.g07c9292
^ permalink raw reply related
* Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams
From: Lars Schneider @ 2016-09-27 8:14 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: <77315FC2-47F3-433A-8D70-5497FB04CBBE@gmail.com>
On 26 Sep 2016, at 22:23, Lars Schneider <larsxschneider@gmail.com> wrote:
>
> On 25 Sep 2016, at 15:46, 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>
>
>
>>> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
>>> + paket_len = packet_read(fd_in, NULL, NULL,
>>> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
>>
>> A question (which perhaps was answered during the development of this
>> patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here?
>
> Nice catch. I think this is wrong:
> https://github.com/git/git/blob/6fe1b1407ed91823daa5d487abe457ff37463349/pkt-line.c#L196
>
> It should be "if (len > size)" ... then we don't need the "+1" here.
> (but I need to think a bit more about this)
After looking at it with fresh eyes I think the existing code is probably correct,
but maybe a bit confusing.
packet_read() adds a '\0' at the end of the destination buffer:
https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206
That is why the destination buffer needs to be one byte larger than the expected content.
However, in this particular case that wouldn't be necessary because the destination
buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we are not
supposed to write to this extra byte:
https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31
I see two options:
(1) I leave the +1 as is and add a comment why the extra byte is necessary.
Pro: No change in existing code necessary
Con: The destination buffer has two '\0' at the end.
(2) I add an option PACKET_READ_DISABLE_NUL_TERMINATION. If the option is
set then no '\0' byte is added to the end.
Pro: Correct solution, no byte wasted.
Con: Change in existing code required.
Any preference?
Thanks,
Lars
^ permalink raw reply
* Re: Possible integer overflow parsing malformed objects in git 2.10.0
From: Jeff King @ 2016-09-27 8:07 UTC (permalink / raw)
To: Gustavo Grieco; +Cc: git
In-Reply-To: <381383122.8376940.1474943423005.JavaMail.zimbra@imag.fr>
On Tue, Sep 27, 2016 at 04:30:23AM +0200, Gustavo Grieco wrote:
> We found a malformed object file that triggers an allocation with a
> negative size when parsed in git 2.10.0. It can be caused by an
> integer overflow somewhere, so it is better to verify how the code got
> such value.
Are you sure this is triggering a negative integer?
The zlib-inflated contents for the object in your example look like:
(gdb) print hdr
$2 = "tree 18446744073709551460\000..."
IOW, this really _is_ a gigantic number, but still within 2^64. So when
we feed it to malloc, that really is correct. And we'd expect malloc to
return NULL, at which point we'll call die, which should look like this
(which I get when running without ASAN):
$ git fsck
fatal: Out of memory, malloc failed (tried to allocate 18446744073709551461 bytes)
You'll note that's 1 more than the value in the object; that addition
happens via xmallocz() and _is_ checked for integer overflow.
> The ASAN report is here:
>
> ==24709==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes
> ==24709==AddressSanitizer's allocator is terminating the process instead of returning 0
> ==24709==If you don't like this behavior set allocator_may_return_null=1
I don't think this is an overflow at all. This is just ASAN having
really conservative debugging defaults. A real malloc would return NULL,
and git would notice and abort.
If you follow its suggestion, you get:
$ ASAN_OPTIONS=allocator_may_return_null=1 git fsck
==19380==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes
==19380==WARNING: AddressSanitizer failed to allocate 0xffffffffffffff65 bytes
fatal: Out of memory, malloc failed (tried to allocate 18446744073709551461 bytes)
as expected. So I don't think there is any bug at all in the example
you gave, only a silly-sized object that we cannot handle.
That being said, the parse_sha1_header() function clearly does not
detect overflow at all when parsing the size. So on a 32-bit system, you
end up with:
$ git fsck
fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)
which is not correct, but I'm not sure it's a security problem. Integer
overflows are an issue if they cause us to under-allocate, and then to
write more bytes than we allocated. In this case, I would expect
unpack_sha1_rest() to never write more bytes than the "size" we parsed
and allocated (and to complain if the number of bytes we get from the
zlib sequence do not exactly match the claimed size).
So a more interesting example is more like "ULONG_MAX + 5", where we
would overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not
ever write more than 5 bytes. From my reading and a few tests with gdb,
it does not. However, it also does not notice that there were more bytes
that we didn't use.
So I think there's room for improved diagnosis of bogus situations
(including integer overflows), but I don't see any actual security bugs.
-Peff
^ permalink raw reply
* Re: [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag
From: Philip Oakley @ 2016-09-27 7:44 UTC (permalink / raw)
To: santiago, git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20160926224233.32702-5-santiago@nyu.edu>
From: <santiago@nyu.edu>
> From: Santiago Torres <santiago@nyu.edu>
>
> Callers of verify-tag may want to cross-check the tagname from refs/tags
> with the tagname from the tag object header upon GPG verification. This
> is to avoid tag refs that point to an incorrect object.
>
> Add a --format parameter to git verify-tag to print the formatted tag
> object header in addition to or instead of the --verbose or --raw GPG
> verification output.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> builtin/verify-tag.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index de10198..a941053 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -12,12 +12,15 @@
> #include <signal.h>
> #include "parse-options.h"
> #include "gpg-interface.h"
> +#include "ref-filter.h"
>
> static const char * const verify_tag_usage[] = {
> - N_("git verify-tag [-v | --verbose] <tag>..."),
> + N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
Does this require a corresponding documentation change? (also 5/5)
> NULL
> };
>
> +static char *fmt_pretty;
> +
> static int git_verify_tag_config(const char *var, const char *value, void
> *cb)
> {
> int status = git_gpg_config(var, value, cb);
> @@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const
> char *prefix)
> const struct option verify_tag_options[] = {
> OPT__VERBOSE(&verbose, N_("print tag contents")),
> OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"),
> GPG_VERIFY_RAW),
> + OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use
> for the output")),
> OPT_END()
> };
>
> @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const
> char *prefix)
> if (verbose)
> flags |= GPG_VERIFY_VERBOSE;
>
> + if (fmt_pretty) {
> + verify_ref_format(fmt_pretty);
> + flags |= GPG_VERIFY_QUIET;
> + }
> +
> while (i < argc) {
> unsigned char sha1[20];
> const char *name = argv[i++];
> if (get_sha1(name, sha1))
> had_error = !!error("tag '%s' not found.", name);
> - else if (verify_and_format_tag(sha1, name, NULL, flags))
> + else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> had_error = 1;
> }
> return had_error;
> --
> 2.10.0
>
--
Philip
^ 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