* [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
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Orgad Shaneh @ 2016-09-27 7:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Thoyts, git
In-Reply-To: <xmqqmviupcpx.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 27, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Orgad Shaneh <orgads@gmail.com> writes:
>
>> On Sun, Jul 10, 2016 at 7:36 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>>
>>> On Wed, May 18, 2016 at 9:12 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>>>> ping?
>>>>
>>> It's been over 2 months. Can anyone please review and merge it?
>>>
>> 4.5 months and counting... :(
>>>
>>>> On Thu, May 5, 2016 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Pat, we haven't heard from you for a long time. Are you still
>>>>> around and interested in helping us by maintaining git-gui?
>>>>>
>>>>> Otherwise we may have to start recruiting a volunteer or two to take
>>>>> this over.
>
> Sorry about that. No volunteers materialized yet X-<, and I really
> really do not want to apply anything other than trivial patches to
> it myself, as I am not a git-gui user.
>
This patch has been in use in Git for Windows for a decent period of time.
I actually see that there is a problem with it:
https://github.com/git-for-windows/git/issues/761
I'll try to revise it and resubmit.
- Orgad
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Jeff King @ 2016-09-27 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Gustavo Grieco, git
In-Reply-To: <xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 11:10:54AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am inclined to say that it has no security implications. You have
> > to be able to write a bogus loose object in an object store you
> > already have write access to in the first place, in order to cause
> > this ...
>
> Note that you could social-engineer others to fetch from you and
> feed a small enough update that results in loose objects created in
> their repositories, without you having a direct write access to the
> repository.
>
> The codepath under discussion in this thread however cannot be used
> as an attack vector via that route, because the "fetch from
> elsewhere" codepath runs verification of the incoming data stream
> before storing the results (either in loose object files, or in a
> packfile) on disk.
I don't think it could be used at all for anything that speaks the git
protocol, because the object header is not present at all in a packfile.
So even if you hit unpack-objects, it would be writing the (correct)
loose object header itself.
But when we grab loose objects _directly_ from a remote, as in dumb-http
fetch, I'd suspect that the code doing the verification calls
unpack_sha1_header() as part of it. So I didn't test, but I'd strongly
suspect that's a viable attack vector.
I'm not sure what the actual attack would look like, though, aside from
locally accessing memory in a read-only way.
-Peff
^ permalink raw reply
* [PATCH] worktree: honor configuration variables
From: Junio C Hamano @ 2016-09-27 6:49 UTC (permalink / raw)
To: git
The command accesses default_abbrev (defined in environment.c and is
updated via core.abbrev configuration), but never makes any call to
git_config(). The output from "worktree list" ignores the abbrev
setting for this reason.
Make a call to git_config() to read the default set of configuration
variables at the beginning of the command.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/worktree.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6dcf7bd..5c4854d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -528,6 +528,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
OPT_END()
};
+ git_config(git_default_config, NULL);
+
if (ac < 2)
usage_with_options(worktree_usage, options);
if (!prefix)
--
2.10.0-561-g98a6b79
^ permalink raw reply related
* Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Jeff King @ 2016-09-27 6:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Vegard Nossum, git, Santi Béjar, Kevin Bracey, Philip Oakley,
Matthieu Moy, Ramsay Jones, Jakub Narębski
In-Reply-To: <xmqqh992pbq6.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 02:55:45PM -0700, Junio C Hamano wrote:
> Taking these two together, perhaps squashing this in may be
> sufficient.
> [...]
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 2c3da19..9474c37 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)
> if (include_rev)
> show_rev(NORMAL, sha1, arg);
> commit = lookup_commit_reference(sha1);
> +
> + if (exclude_parent) {
> + /* do we have enough parents? */
> + for (parent_number = 0, parents = commit->parents;
> + parents;
> + parents = parents->next)
> + parent_number++;
> + if (parent_number < exclude_parent) {
> + *dotdot = '^';
> + return 0;
> + }
> + }
I think you can use commit_list_count() to make this a bit shorter,
like:
if (exclude_parent &&
commit_list_count(commit->parents) < exclude_parent) {
*dotdot = '^';
return 0;
}
Technically you can drop the first half of the &&, but it is probably a
good idea to avoid the traversal when exclude_parent is not in use.
Also technically, you can stop counting when you hit exclude_parent
(which is only possible with a custom traversal), but it is unlikely
enough that it is probably not worth caring about.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Junio C Hamano @ 2016-09-27 6:08 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20160927055744.el2jbxzdqfhjl6qt@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I have to admit that I don't care too deeply about performance for
> somebody who wants to convert "latin1" to "ISO-8859-1". If one of your
> encodings is not UTF-8, you are probably Doing It Wrong. :)
Exactly. Note that the "you" in the above are usually plural,
collectively referring to both the sender and the receiver. I
usually am on the poor receiving end ;-)
^ permalink raw reply
* Re: [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Jeff King @ 2016-09-27 5:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20160927012211.9378-3-gitster@pobox.com>
On Mon, Sep 26, 2016 at 06:22:11PM -0700, Junio C Hamano wrote:
> Even though latin-1 is still seen in e-mail headers, some platforms
> only install ISO-8859-1. "iconv -f ISO-8859-1" succeeds, while
> "iconv -f latin-1" fails on such a system.
>
> Using the same fallback_encoding() mechanism factored out in the
> previous step, teach ourselves that "ISO-8859-1" has a better chance
> of being accepted than "latin-1".
I was curious if this was the most official or accepted spelling.
Grepping a few hundred thousand messages from my mail archives, it does
seem to be the most common.
> diff --git a/utf8.c b/utf8.c
> index 550e785..0c8e011 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -501,6 +501,13 @@ static const char *fallback_encoding(const char *name)
> if (is_encoding_utf8(name))
> return "UTF-8";
>
> + /*
> + * Even though latin-1 is still seen in e-mail
> + * headers, some platforms only install ISO-8859-1.
> + */
> + if (!strcasecmp(name, "latin-1"))
> + return "ISO-8859-1";
> +
For the UTF-8 fallbacks, we actually detect their equivalence via
same_encoding() before even hitting iconv. Is it worth doing the same
here?
I have to admit that I don't care too deeply about performance for
somebody who wants to convert "latin1" to "ISO-8859-1". If one of your
encodings is not UTF-8, you are probably Doing It Wrong. :)
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Jeff King @ 2016-09-27 5:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20160927012211.9378-2-gitster@pobox.com>
On Mon, Sep 26, 2016 at 06:22:10PM -0700, Junio C Hamano wrote:
> @@ -501,17 +516,9 @@ char *reencode_string_len(const char *in, int insz,
>
> conv = iconv_open(out_encoding, in_encoding);
> if (conv == (iconv_t) -1) {
> - /*
> - * Some platforms do not have the variously spelled variants of
> - * UTF-8, so let's fall back to trying the most official
> - * spelling. We do so only as a fallback in case the platform
> - * does understand the user's spelling, but not our official
> - * one.
> - */
> - if (is_encoding_utf8(in_encoding))
> - in_encoding = "UTF-8";
> - if (is_encoding_utf8(out_encoding))
> - out_encoding = "UTF-8";
> + in_encoding = fallback_encoding(in_encoding);
> + out_encoding = fallback_encoding(out_encoding);
> +
This comment is interesting. We're concerned about a platform knowing
"utf8" but not "UTF-8". When we fallback, we do it for both the input
and output encodings, because we don't know which may have caused the
problem. So is it possible that we improve one case but break the other?
With just UTF-8, I don't think so. That could only be the case with
something like "utf8 -> utf-8" because they both become "UTF-8". So
either it improves the situation or not (because we either understand
UTF-8 or not).
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.
-Peff
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jacob Keller @ 2016-09-27 5:42 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Junio C Hamano, Git Mailing List
In-Reply-To: <CA+55aFyfvvqq1c=hZcuL-yPavp2tjzx8r3bFJnMY7DAE7YcB=Q@mail.gmail.com>
On Mon, Sep 26, 2016 at 9:36 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> 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.
>
I'd think we would want to phase this in over a few releases if we do
this? Maybe at least sort commits first in the list so that they are
faster to spot.
I am trying to think of what problems we'd cause by having the
behavior be this aggressive...
Thanks,
Jake
> But regardless, this series looks like a good thing.
>
> Linus
^ permalink raw reply
* Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 5:35 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, mhagger, David Turner
In-Reply-To: <20160927051453.yuvrnao5ldjpzhcj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> +test_expect_success 'malformed mode in tree' '
>> + test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
>> + grep "malformed mode in tree entry for tree" err
>> +'
>
> This ".." will break when the test is run with "--root". You should use
>
> "$TEST_DIRECTORY"/t1007/...
>
> instead. And ditto in the second test, of course.
Ahh, that explains the breakage I saw.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: Jeff King @ 2016-09-27 5:27 UTC (permalink / raw)
To: David Turner; +Cc: git, mhagger, David Turner
In-Reply-To: <1474918365-10937-3-git-send-email-novalis@novalis.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4422 bytes --]
On Mon, Sep 26, 2016 at 03:32:45PM -0400, David Turner wrote:
> 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.
Cool. I think the lack of this is what made me drag my feet on the first
patch. Thanks for finishing it off.
> 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)) {
I wondered if other callers would be happy with init_tree_desc_gently().
Grepping for init_tree_desc(), it seems like it would be a fairly
trivial conversion for most of them, because they almost invariably run
unpack_trees() right afterwards, and so have to deal with errors from
it.
So perhaps in the long run we can convert them all. But certainly that
does not need to be part of this series.
> +test_expect_success 'unparseable tree object' '
> + test_when_finished "git update-ref -d refs/heads/wrong" &&
> + test_when_finished "remove_object 307e300745b82417cc1a903f875c7d22e45ef907" &&
> + test_when_finished "remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd" &&
> + mkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&
> + cp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&
> + cp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&
This needs the same $TEST_DIRECTORY treatment as t1007.
> + git update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&
> + test_must_fail git fsck 2>out &&
> + grep "warning: empty filename in tree entry" out &&
> + grep "f506a346749bb96f52d8605ffba9fb93d46b5ffd" out &&
> + ! grep "fatal: empty filename in tree entry" out
> +'
I'd also expect these to be test_i18ngrep, but I see that t1450 is quite
bad about this in general. I'm OK with adding them as greps and leaving
a conversion of the whole script until later.
> diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> new file mode 100644
> index 0000000..6e23d62
> --- /dev/null
> +++ b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> @@ -0,0 +1,4 @@
> +x\x01ŽA\x0e \x10E]sŠ¹€f°@Ä\x18\x17\x1eÁ\v0\x05Z\x12[\x12
> +õú¢é \ýüÅ{ÿ\x0fižc\x01IòP²÷\x104\x1aÛ)Ó+b&\x13ôÙ]\fê\x10ØR`êœ2Üš\x13¶–)exØ-:xÖ¼ø\f×%mö\x15×ûž§”Ç^[HÕd\x12{-áˆ
> +Q\f¿ÍÒ€\x7fè\x1d‡w,\x13p\x1aë
> +ßçâ\x03&ë?Þ
> \ No newline at end of file
Yikes. :)
I wonder if some printfs, similar to what I showed in the last patch,
combined with "hash-object --literally", could make these tests more
readable and avoid the binary goo.
> -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.
> +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)
> + warning("%s", err.buf);
> + strbuf_release(&err);
> + return result;
> }
I also wonder if this ought to be "error()" and not "warning()". I think
it's pretty common for fsck to spit out errors from sub-code but keep going.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors
From: Jeff King @ 2016-09-27 5:14 UTC (permalink / raw)
To: David Turner; +Cc: git, mhagger, David Turner
In-Reply-To: <1474918365-10937-2-git-send-email-novalis@novalis.org>
On Mon, Sep 26, 2016 at 03:32:44PM -0400, David Turner wrote:
> 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>
Yay. This has been on my "to look at and repost" list for literally 2
years. Thanks for picking it up (see kids, procrastination _does_ pay
off).
> t/t1007-hash-object.sh | 15 +++++++++++++--
> t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
> t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
Ooh, and tests. Exciting.
> -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 &&
> + grep "too-short tree object" err
> +'
Should this be test_i18ngrep? Even if the message is not translated now,
it seems like a good proactive measure (and probably it _should_ be
translated).
> +test_expect_success 'malformed mode in tree' '
> + test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
> + grep "malformed mode in tree entry for tree" err
> +'
This ".." will break when the test is run with "--root". You should use
"$TEST_DIRECTORY"/t1007/...
instead. And ditto in the second test, of course.
> diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename
> new file mode 100644
> index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe
> GIT binary patch
> literal 28
> kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3
>
> literal 0
> HcmV?d00001
This is rather opaque, of course. :)
I wonder if it would be possible to generate the test vector with
something like:
# any 20 bytes will do
bin_sha1='\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0'
printf "100644 \0$bin_sha1" >tree-with-empty-filename
I know that is longer and possibly more error-prone to run, but I think
it makes the test much easier to read and modify later.
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 also think it would be nice if hash-object had a "--binary-sha1"
option to avoid the perl grossness. :)
> diff --git a/tree-walk.c b/tree-walk.c
> index ce27842..ba544cf 100644
The code change itself looks brilliant, naturally. :)
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: Jeff King @ 2016-09-27 4:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Turner, git
In-Reply-To: <xmqqtwd2ng8k.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 09:01:15PM -0700, Junio C Hamano wrote:
> > 5 files changed, 21 insertions(+), 7 deletions(-)
> > create mode 100644 t/t1007/.gitattributes
> > create mode 100644 t/t1007/tree-with-empty-filename
> > create mode 100644 t/t1007/tree-with-malformed-mode
>
> I hate to report this, but this alone, or together with 2/2, when
> merged to 'pu', I cannot get them to pass the tests in my automated
> integration tests, even though they seem to pass when the problematic
> tests are run manually. I do not see offhand anything suspicious
> (like something that may be racy) in these two patches but I haven't
> figured out where it goes wrong.
>
> If somebody manages to find breakages in today's 'pu', please (1) do
> not be too alarmed, and (2) help figure out where things are broken.
I think the problem is just that they refer to t/t1450 (and t1007) from
the trash directory as "../t1450". That breaks when the test is run with
"--root" (and I imagine that like me, you have --root as part of your
automated tests but do not bother with it when doing a one-off run).
-Peff
^ permalink raw reply
* [PATCH] xdiff: rename "struct group" to "struct xdlgroup"
From: Jeff King @ 2016-09-27 4:37 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
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).
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't rename the functions, which have no conflict, but that would
also be closer to xdiff's usual style. I don't know how far it is worth
going; maybe this patch is even already too far.
I noticed because I have a patch series which switches xdiff
to git-compat-util, to try to use the st_* macros there.
xdiff/xdiffi.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 67c1ccc..760fbb6 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -708,7 +708,7 @@ static int score_cmp(struct split_score *s1, struct split_score *s2)
* Note that loops that are testing for changed lines in xdf->rchg do not need
* index bounding since the array is prepared with a zero at position -1 and N.
*/
-struct group {
+struct xdlgroup {
/*
* The index of the first changed line in the group, or the index of
* the unchanged line above which the (empty) group is located.
@@ -725,7 +725,7 @@ struct group {
/*
* Initialize g to point at the first group in xdf.
*/
-static void group_init(xdfile_t *xdf, struct group *g)
+static void group_init(xdfile_t *xdf, struct xdlgroup *g)
{
g->start = g->end = 0;
while (xdf->rchg[g->end])
@@ -736,7 +736,7 @@ static void group_init(xdfile_t *xdf, struct group *g)
* Move g to describe the next (possibly empty) group in xdf and return 0. If g
* is already at the end of the file, do nothing and return -1.
*/
-static inline int group_next(xdfile_t *xdf, struct group *g)
+static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->end == xdf->nrec)
return -1;
@@ -752,7 +752,7 @@ static inline int group_next(xdfile_t *xdf, struct group *g)
* Move g to describe the previous (possibly empty) group in xdf and return 0.
* If g is already at the beginning of the file, do nothing and return -1.
*/
-static inline int group_previous(xdfile_t *xdf, struct group *g)
+static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
{
if (g->start == 0)
return -1;
@@ -769,7 +769,7 @@ static inline int group_previous(xdfile_t *xdf, struct group *g)
* following group, expand this group to include it. Return 0 on success or -1
* if g cannot be slid down.
*/
-static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
+static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g, long flags)
{
if (g->end < xdf->nrec &&
recs_match(xdf->recs[g->start], xdf->recs[g->end], flags)) {
@@ -790,7 +790,7 @@ static int group_slide_down(xdfile_t *xdf, struct group *g, long flags)
* into a previous group, expand this group to include it. Return 0 on success
* or -1 if g cannot be slid up.
*/
-static int group_slide_up(xdfile_t *xdf, struct group *g, long flags)
+static int group_slide_up(xdfile_t *xdf, struct xdlgroup *g, long flags)
{
if (g->start > 0 &&
recs_match(xdf->recs[g->start - 1], xdf->recs[g->end - 1], flags)) {
@@ -818,7 +818,7 @@ static void xdl_bug(const char *msg)
* size.
*/
int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
- struct group g, go;
+ struct xdlgroup g, go;
long earliest_end, end_matching_other;
long groupsize;
unsigned int blank_lines;
--
2.10.0.492.g14f803f
^ permalink raw reply related
* Re: [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: Junio C Hamano @ 2016-09-27 4:01 UTC (permalink / raw)
To: David Turner; +Cc: git, Jeff King
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> 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 | 15 +++++++++++++--
> t/t1007/.gitattributes | 1 +
> t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
> t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
> tree-walk.c | 12 +++++++-----
> 5 files changed, 21 insertions(+), 7 deletions(-)
> create mode 100644 t/t1007/.gitattributes
> create mode 100644 t/t1007/tree-with-empty-filename
> create mode 100644 t/t1007/tree-with-malformed-mode
I hate to report this, but this alone, or together with 2/2, when
merged to 'pu', I cannot get them to pass the tests in my automated
integration tests, even though they seem to pass when the problematic
tests are run manually. I do not see offhand anything suspicious
(like something that may be racy) in these two patches but I haven't
figured out where it goes wrong.
If somebody manages to find breakages in today's 'pu', please (1) do
not be too alarmed, and (2) help figure out where things are broken.
Thanks.
^ permalink raw reply
* RE: git-upload-pack hangs
From: Jason Pyeron @ 2016-09-27 3:45 UTC (permalink / raw)
To: git
In-Reply-To: <62E3FC352BE4428A90D7E4E9B137A9FB@black7>
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
* Possible integer overflow parsing malformed objects in git 2.10.0
From: Gustavo Grieco @ 2016-09-27 2:30 UTC (permalink / raw)
To: git
In-Reply-To: <1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr>
Hi,
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. 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 'eJyVT8ERAjEIXKiEBpyBHJdcCroGHAvQjyX49m1ZtmADQjL68uMnZFnYZU/HfRfb3Gtz17Y07etqXhX6ul9uAnCJh6DCAKxUCWABok9J2PN8jYn42iwqYA2OYoKRzVAY67mYgIOfQP8WOthUKubNt6V6/yn5YSPEowsxKGPk0Jdq6ZLKxJYX2LTjYTNi52WTAN4RVyPd' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Finally you can trigger the bug using several commands from git (other commands that parses all objects will work too), for instance:
$ git fsck
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
==24709==AddressSanitizer CHECK failed: /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145 "((0)) != (0)" (0x0, 0x0)
#0 0x7f571ae467aa in AsanCheckFailed /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_rtl.cc:65
#1 0x7f571ae4d163 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_common.cc:157
#2 0x7f571ae4b326 in __sanitizer::ReportAllocatorCannotReturnNull() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145
#3 0x7f571ad9b2f4 in __sanitizer::CombinedAllocator<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback>, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator64<105553116266496ul, 4398046511104ul, 0ul, __sanitizer::SizeClassMap<17ul, 128ul, 16ul>, __asan::AsanMapUnmapCallback> >, __sanitizer::LargeMmapAllocator<__asan::AsanMapUnmapCallback> >::ReturnNullOrDie() /build/gcc-multilib/src/gcc/libsanitizer/sanitizer_common/sanitizer_allocator.h:1315
#4 0x7f571ad9b2f4 in __asan::Allocator::Allocate(unsigned long, unsigned long, __sanitizer::BufferedStackTrace*, __asan::AllocType, bool) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:357
#5 0x7f571ad9b2f4 in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_allocator.cc:716
#6 0x7f571ae3ce24 in __interceptor_malloc /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:63
#7 0x767816 in do_xmalloc /home/g/Work/Code/git-2.10.0/wrapper.c:59
#8 0x76794c in do_xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:99
#9 0x7679bd in xmallocz /home/g/Work/Code/git-2.10.0/wrapper.c:107
#10 0x6fe36c in unpack_sha1_rest /home/g/Work/Code/git-2.10.0/sha1_file.c:1625
#11 0x6feb40 in unpack_sha1_file /home/g/Work/Code/git-2.10.0/sha1_file.c:1751
#12 0x703fe0 in read_object /home/g/Work/Code/git-2.10.0/sha1_file.c:2811
#13 0x70410a in read_sha1_file_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:2834
#14 0x647676 in read_sha1_file /home/g/Work/Code/git-2.10.0/cache.h:1056
#15 0x648545 in parse_object /home/g/Work/Code/git-2.10.0/object.c:269
#16 0x48d46d in fsck_sha1 builtin/fsck.c:367
#17 0x48da47 in fsck_loose builtin/fsck.c:493
#18 0x707514 in for_each_file_in_obj_subdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3477
#19 0x70775b in for_each_loose_file_in_objdir_buf /home/g/Work/Code/git-2.10.0/sha1_file.c:3512
#20 0x707885 in for_each_loose_file_in_objdir /home/g/Work/Code/git-2.10.0/sha1_file.c:3532
#21 0x48dc1d in fsck_object_dir builtin/fsck.c:521
#22 0x48e2e6 in cmd_fsck builtin/fsck.c:644
#23 0x407a8f in run_builtin /home/g/Work/Code/git-2.10.0/git.c:352
#24 0x407e35 in handle_builtin /home/g/Work/Code/git-2.10.0/git.c:539
#25 0x408175 in run_argv /home/g/Work/Code/git-2.10.0/git.c:593
#26 0x408458 in cmd_main /home/g/Work/Code/git-2.10.0/git.c:665
#27 0x53fc70 in main /home/g/Work/Code/git-2.10.0/common-main.c:40
#28 0x7f5719f46290 in __libc_start_main (/usr/lib/libc.so.6+0x20290)
#29 0x405209 in _start (/home/g/Work/Code/git-2.10.0/git+0x405209)
This test case was found using QuickFuzz.
Regards,
Gustavo.
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Gustavo Grieco @ 2016-09-27 2:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwd2sf9t.fsf@gitster.mtv.corp.google.com>
Btw, this other test case will trigger a similar issue, but in another line of code:
To reproduce:
$ git init ; mkdir -p .git/objects/b2 ; printf 'eJwNwoENgDAIBECkDsII5Z8CHagLGPePXu59zjHGRIOZG3OzI/lnRc4KemXDPdYSml6iQ+4ATIZ+nAEK4g==' | base64 -d > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Then:
$ git fsck
notice: HEAD points to an unborn branch (master)
=================================================================
==24569==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe7645fda0 at pc 0x0000006fe799 bp 0x7ffe7645fc40 sp 0x7ffe7645fc30
READ of size 1 at 0x7ffe7645fda0 thread T0
#0 0x6fe798 in parse_sha1_header_extended /home/g/Work/Code/git-2.10.0/sha1_file.c:1714
...
It will be nice to test the current patch.
----- Original Message -----
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am inclined to say that it has no security implications. You have
> > to be able to write a bogus loose object in an object store you
> > already have write access to in the first place, in order to cause
> > this ...
>
> Note that you could social-engineer others to fetch from you and
> feed a small enough update that results in loose objects created in
> their repositories, without you having a direct write access to the
> repository.
>
> The codepath under discussion in this thread however cannot be used
> as an attack vector via that route, because the "fetch from
> elsewhere" codepath runs verification of the incoming data stream
> before storing the results (either in loose object files, or in a
> packfile) on disk.
>
>
^ permalink raw reply
* [PATCH 2/2] utf8: accept "latin-1" as ISO-8859-1
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
In-Reply-To: <20160927012211.9378-1-gitster@pobox.com>
Even though latin-1 is still seen in e-mail headers, some platforms
only install ISO-8859-1. "iconv -f ISO-8859-1" succeeds, while
"iconv -f latin-1" fails on such a system.
Using the same fallback_encoding() mechanism factored out in the
previous step, teach ourselves that "ISO-8859-1" has a better chance
of being accepted than "latin-1".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
utf8.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/utf8.c b/utf8.c
index 550e785..0c8e011 100644
--- a/utf8.c
+++ b/utf8.c
@@ -501,6 +501,13 @@ static const char *fallback_encoding(const char *name)
if (is_encoding_utf8(name))
return "UTF-8";
+ /*
+ * Even though latin-1 is still seen in e-mail
+ * headers, some platforms only install ISO-8859-1.
+ */
+ if (!strcasecmp(name, "latin-1"))
+ return "ISO-8859-1";
+
return name;
}
--
2.10.0-556-g5bbc40b
^ permalink raw reply related
* [PATCH 1/2] utf8: refactor code to decide fallback encoding
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
In-Reply-To: <20160927012211.9378-1-gitster@pobox.com>
The codepath we use to call iconv_open() has a provision to use a
fallback encoding when it fails, hoping that "UTF-8" being spelled
differently could be the reason why the library function did not
like the encoding names we gave it. Essentially, we turn what we
have observed to be used as variants of "UTF-8" (e.g. "utf8") into
the most official spelling and use that as a fallback.
We do the same thing for input and output encoding. Introduce a
helper function to do just one side and call that twice.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
utf8.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/utf8.c b/utf8.c
index 00e10c8..550e785 100644
--- a/utf8.c
+++ b/utf8.c
@@ -489,6 +489,21 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs
return out;
}
+static const char *fallback_encoding(const char *name)
+{
+ /*
+ * Some platforms do not have the variously spelled variants of
+ * UTF-8, so let's fall back to trying the most official
+ * spelling. We do so only as a fallback in case the platform
+ * does understand the user's spelling, but not our official
+ * one.
+ */
+ if (is_encoding_utf8(name))
+ return "UTF-8";
+
+ return name;
+}
+
char *reencode_string_len(const char *in, int insz,
const char *out_encoding, const char *in_encoding,
int *outsz)
@@ -501,17 +516,9 @@ char *reencode_string_len(const char *in, int insz,
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1) {
- /*
- * Some platforms do not have the variously spelled variants of
- * UTF-8, so let's fall back to trying the most official
- * spelling. We do so only as a fallback in case the platform
- * does understand the user's spelling, but not our official
- * one.
- */
- if (is_encoding_utf8(in_encoding))
- in_encoding = "UTF-8";
- if (is_encoding_utf8(out_encoding))
- out_encoding = "UTF-8";
+ in_encoding = fallback_encoding(in_encoding);
+ out_encoding = fallback_encoding(out_encoding);
+
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1)
return NULL;
--
2.10.0-556-g5bbc40b
^ permalink raw reply related
* [PATCH 0/2] Locally alias "latin-1" to "ISO-8859-1"
From: Junio C Hamano @ 2016-09-27 1:22 UTC (permalink / raw)
To: git
Some systems do not seem to ship "latin-1" as a valid locale, even
though they happilly accept more modern official name "ISO-8859-1".
Naturally, "iconv -f iso-8859-1" succeeds while "iconv -f latin-1"
fails on such a system.
We already have in utf8.c to accomodate overly strict iconv_open()
that does not like various spellings of UTF-8 when our users spell
it differently from the most official "UTF-8" form. Piggyback on
the mechanism and teach outselves that "latin-1" used to be the way
to say "ISO-8859-1".
I feel dirty for doing it this way, but I found it the easiest
workaround to apply recent patches we saw on the mailing list.
Junio C Hamano (2):
utf8: refactor code to decide fallback encoding
utf8: accept "latin-1" as ISO-8859-1
utf8.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
--
2.10.0-556-g5bbc40b
^ permalink raw reply
* Re: [PATCH 0/2] tree-walk improvements
From: Junio C Hamano @ 2016-09-27 0:35 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, mhagger
In-Reply-To: <1474921343.13374.1.camel@frank>
David Turner <novalis@novalis.org> writes:
> Because truncated, to me, means "something that has been cut off". Here,
> the recorded length is too short, so it's probably not the case that
> something was cut off -- it was never right to begin with.
That's perfectly sensible. Thanks.
^ 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