* Re: [PATCH v2 08/11] i18n: send-email: mark strings for translation
From: Junio C Hamano @ 2016-09-25 23:18 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-9-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> @@ -1403,7 +1412,7 @@ Message-Id: $message_id
> if ($quiet) {
> printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
> } else {
> - print (($dry_run ? "Dry-" : "")."OK. Log says:\n");
> + print (($dry_run ? "Dry-" : ""). __("OK. Log says:\n"));
I am not sure about this change. We either say Dry-Sent/Dry-OK
(under --dry-run) or "Sent/OK"; don't you want "dry-" part also
translated?
^ permalink raw reply
* Re: [PATCH v2 11/11] i18n: difftool: mark warnings for translation
From: Junio C Hamano @ 2016-09-25 23:21 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <1472646690-9699-12-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> - warn << 'EOF';
> + warn __ <<'EOF';
> Combined diff formats ('-c' and '--cc') are not supported in
> directory diff mode ('-d' and '--dir-diff').
> EOF
Wow, didn't imagine gettext would pick this up. Nice.
^ permalink raw reply
* Re: [PATCH v2 00/11] Mark strings in perl script for translation
From: Junio C Hamano @ 2016-09-25 23:31 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
David Aguilar
In-Reply-To: <xmqqtwe026p9.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Vasco Almeida <vascomalmeida@sapo.pt> writes:
>
>> Mark messages in some perl scripts for translation.
>>
>> Since v1, adds brackets so parameter grouping of sprintf parameters is easier
>> to see.
>>
>> Interdiff included below.
>
> Thanks; it is way too late for this cycle for i18n so I won't be
> picking the series up right now. Please ping me if you see me
> forget to pick it up in a week after 2.10 final gets tagged.
I'll queue this round on 'pu' (if there aren't too heavy conflicts,
that is) and let the interested parties continue reviewing and
discussing.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3 v3] submodules: make submodule-prefix option an envvar
From: Junio C Hamano @ 2016-09-25 23:34 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474676014-134568-2-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Add a submodule-prefix enviorment variable
environment?
> 'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
> --recurse-submodule options.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> cache.h | 1 +
> environment.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 3556326..ae88a35 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
> #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
> #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
> +#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
> #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
> #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
> #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
> diff --git a/environment.c b/environment.c
> index ca72464..7380815 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
> NO_REPLACE_OBJECTS_ENVIRONMENT,
> GIT_REPLACE_REF_BASE_ENVIRONMENT,
> GIT_PREFIX_ENVIRONMENT,
> + GIT_SUBMODULE_PREFIX_ENVIRONMENT,
> GIT_SHALLOW_FILE_ENVIRONMENT,
> GIT_COMMON_DIR_ENVIRONMENT,
> NULL
^ permalink raw reply
* Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
From: Junio C Hamano @ 2016-09-26 0:10 UTC (permalink / raw)
To: Gustavo Grieco; +Cc: git
In-Reply-To: <1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr>
Gustavo Grieco <gustavo.grieco@imag.fr> writes:
> We found a stack read out-of-bounds parsing object files using git 2.10.0. It was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN support and then execute:
>
> $ git init ; mkdir -p .git/objects/b2 ; printf 'x' > .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0
Interesting. If you prepare such a broken loose object file in your
local repository, I would expect that either unpack_sha1_header() or
unpack_sha1_header_to_strbuf() that sha1_loose_object_info() calls
would detect and barf by noticing that an error came from libz while
it attempts to inflate and would not even call parse_sha1_header.
But it is nevertheless bad to assume that whatever happens to
inflate without an error must be formatted correctly to allow
parsing (i.e. has ' ' and then NUL termination within the first 32
bytes after inflation), which is exactly what the hdr[32] is saying.
Perhaps we need something like the following to tighten the
codepath.
Note that this is totally unteseted and not thought through; I
briefly thought about what unpack_sha1_header_to_strbuf() does with
this change (it first lets unpack_sha1_header() to attempt with a
small buffer but it seems to discard the error code from it before
seeing if the returned buffer has NUL in it); there may be bad
interactions with it.
sha1_file.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/sha1_file.c b/sha1_file.c
index 60ff21f..dfcbd76 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1648,6 +1648,8 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
{
+ int status;
+
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
stream->next_in = map;
@@ -1656,7 +1658,15 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
stream->avail_out = bufsiz;
git_inflate_init(stream);
- return git_inflate(stream, 0);
+ status = git_inflate(stream, 0);
+ if (status)
+ return status;
+
+ /* Make sure we got the terminating NUL for the object header */
+ if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+ return -1;
+
+ return 0;
}
static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
@@ -1758,6 +1768,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
char c = *hdr++;
if (c == ' ')
break;
+ if (!c)
+ die("invalid object header");
type_len++;
}
^ permalink raw reply related
* Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-26 0:39 UTC (permalink / raw)
To: Philip Oakley; +Cc: Vegard Nossum, git, Santi Béjar, Kevin Bracey
In-Reply-To: <E7D489891E404823BF34F48E7B9E5618@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>I use rev^..rev daily, and I'm surely not the only one.
>
> Not everyone knows the 'trick' and may not use it daily.
>
> Consider stating what it is useful for (e.g. "useful to get the
> commits and all commits in the branches that were merged into commit"
> - paraphrased from the doc text)
>
>> To save typing
>> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
>> we can make rev^- a shorthand for that.
>>
>> The existing syntax rev^! seems like it should do the same, but it
>> doesn't really do the right thing for merge commits (it gives only the
>> merge itself).
>
> .. rather than the commit and those on side branches).
>> As a natural generalisation, we also accept rev^-n where n excludes the
>> nth parent of rev,
>
>> although this is expected to be generally less useful.
>
> Presumptious? for a two parent merge, surely(?) rev^-2 will give you
> what has been going on on the main line while the branch was being
> prepared... compare A^- and A^-2.
All good comments. It often is a good strategy to avoid subjective
"this is useful" and "this is not useful" assessment, and instead
let the feature itself find its supporters in the reading public.
>> +Parent Exclusion Notation
>> +~~~~~~~~~~~~~~~~~~~~~~~~~
>> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
>> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>> +given. This is typically useful for merge commits where you
>> +can just pass '<commit>{caret}-' to get all the commits in the branch
>
> s/get all the/get the commit and all the/ ?
> It could be misread as a way of selecting just those commits that are
> within the side branch without including the given commit itself.
>
>> +that was merged in merge commit '<commit>'.
>> +
>> Other <rev>{caret} Parent Shorthand Notations
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Two other shorthands exist, particularly useful for merge commits,
Is it just me that this new thing belongs to this "other shorthand
notations", making the total to three from two? It really is a
closely related cousin of existing 'r1{caret}!'; instead of
excluding all of its parents, it only excludes the specified one of
its parents. IOW, this new one is better described as the third
other shorthand in this "Other Notations" section, without creating
a new "Parent Exclusion Notation" section.
>> @@ -316,6 +324,10 @@ Revision Range Summary
>> <rev2> but exclude those that are reachable from both. When
>> either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>>
>> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
Huh? Isn't the first example missing the necessary minus sign?
>> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>> + given.
>> +
>> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>> A suffix '{caret}' followed by an at sign is the same as listing
>> all parents of '<rev>' (meaning, include anything reachable from
>> @@ -339,6 +351,8 @@ spelt out:
>> C I J F C
>> B..C = ^B C C
>> B...C = B ^F C G H D E B C
>> + B^- = B^..B
>> + = B ^B^1 E I J F B
Even though these are order independent, the second line should say
= ^B^1 B E I J F B
to be consistent with the expansion of B..C, I would think.
>> diff --git builtin/rev-parse.c builtin/rev-parse.c
>> index 76cf05e..ad5e6ac 100644
>> --- builtin/rev-parse.c
>> +++ builtin/rev-parse.c
>> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>> return 0;
>> }
>>
>> +static int try_parent_exclusion(const char *arg)
>> +{
>> + int ret = 0;
>> + char *to_rev = NULL;
>> + char *from_rev = NULL;
>> + unsigned char to_sha1[20];
>> + unsigned char from_sha1[20];
>> +
>> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
>> + goto out;
>> + if (get_sha1_committish(to_rev, to_sha1))
>> + goto out;
>> + if (get_sha1_committish(from_rev, from_sha1))
>> + goto out;
>> +
>> + show_rev(NORMAL, to_sha1, to_rev);
>> + show_rev(REVERSED, from_sha1, from_rev);
>> +
>> + ret = 1;
>> +
>> +out:
>> + free(to_rev);
>> + free(from_rev);
>> + return ret;
>> +}
>> +
>> static int try_parent_shorthands(const char *arg)
>> {
>> char *dotdot;
I did not expect that this needs an entirely new helper function,
instead of being implemented as a new special case of existing
try_parent_shorthands() function. You'd need to strstr "^-" and
parse a sequence of digits that follow it, which may want a helper
to make sure you can error out if fed "some^-12thing" saying that
"12thing" is not an integer, extend the existing "parents-only"
thing so that it can represent three cases (i.e. @? !? or -?), and
need a new variable to denote which parent is to be excluded when it
is the '-' kind. You'd need to temporarily *dotdot = '\0', parse
what is before "^-" and revert *dotdot = '^' like existing helper
function just the same.
Exactly the same comment probably applies to the changes to the
parser in revision.c, I would imagine, but I didn't read it ;-)
^ permalink raw reply
* Changing the default for "core.abbrev"?
From: Linus Torvalds @ 2016-09-26 1:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
The default value for commit abbreviation (environment.c: 19) is seven:
int minimum_abbrev = 4, default_abbrev = 7;
which back in the dark early days of git was fairly reasonable.
It's probably still a perfectly fine default for lots of projects,
since 7 hex digits is a few hundred million unique values, and you
won't really start to get very many collisions in that until you get
closer to a million objects.
The kernel, these days, is at roughly 5 million objects, and while the
seven hex digits are still often enough for uniqueness (and git will
always add digits *until* it is unique), it's long been at the point
where I tell people to do
git config --global core.abbrev 12
because even though git will extend the seven hex digits until the
object name is unique, that only reflects the *current* situation in
the repository. With 5 million objects and a very healthy growth rate,
a 7-8 hex digit number that is unique today is not necessarily unique
a month or two from now, and then it gets annoying when a commit
message has a short git ID that is no longer unique when you go back
and try to figure out what went wrong in that commit.
I can just keep reminding kernel maintainers and developers to update
their git config, but maybe it would be a good idea to just admit that
the defaults picked in 2005 weren't necessarily the best ones
possible, and those could be bumped up a bit?
I think I mentioned this some time ago, and it's not a huge deal, but
I thought I'd just mention it again because it came up again today for
me..
Thanks,
Linus
^ permalink raw reply
* Re: Changing the default for "core.abbrev"?
From: Junio C Hamano @ 2016-09-26 3:46 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <CA+55aFy0_pwtFOYS1Tmnxipw9ZkRNCQHmoYyegO00pjMiZQfbg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> I can just keep reminding kernel maintainers and developers to update
> their git config, but maybe it would be a good idea to just admit that
> the defaults picked in 2005 weren't necessarily the best ones
> possible, and those could be bumped up a bit?
>
> I think I mentioned this some time ago, and it's not a huge deal, but
> I thought I'd just mention it again because it came up again today for
> me..
I am not quite sure how good any new default would be, though. Just
like any timeout is not long enough for somebody, growing projects
will eventually hit whatever abbreviation length they start with.
Even if we bump it to 12 for everybody, majority of projects at
GitHub would probably be just wasting 5 more hexdigits in addition
to whatever they are already wasting. The kernel folks will keep
having the problem of having harder time looking up objects referred
to by ancient commits no matter what the new default is anyway, and
then they will again regret we didn't bump it to 16 in year 2016 in
several decades; by that time both of us are probably retired so it
may no longer be our problems, though ;-)
I am not opposed to bump the default to 12 or whatever, but I
suspect any lengthening today may need to be accompanied by a tool
support that finds the set of objects that are reachable from a
commit whose names begin with non-unique abbreviations that appear
in the commit log message. Assuming that it is very hard to refer to
future objects in the log message you write today, such a tool may
find a single object that used to be the unique instance of that
abbreviation back then, and with reachability bitmap support, it may
not be too expensive to run.
^ permalink raw reply
* [PATCH] unpack_sha1_header(): detect malformed object header
From: Junio C Hamano @ 2016-09-26 4:29 UTC (permalink / raw)
To: git, Karthik Nayak, Jeff King; +Cc: Gustavo Grieco
In-Reply-To: <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com>
When opening a loose object file, we often do this sequence:
- prepare a short buffer for the object header (on stack)
- call unpack_sha1_header() and have early part of the object data
inflated, enough to fill the buffer
- parse that data in the short buffer, assuming that the first part
of the object is <type> SP <length> NUL
Nobody in this sequence however actually verifies that the loop that
tries to find SP that must come after the typename or NUL that must
come after the length exist in the inflated data. Because the
parsing function parse_sha1_header_extended() is not even given the
number of bytes inflated into the header buffer, it can easily read
past it, looking for the SP byte that may not even exist.
A variant recently introduced to support "--allow-unknown-type"
option of "git cat-file -t" changes the second step to use
unpack_sha1_header_to_strbuf(), but the story is essentially the
same. It did check to see if it saw enough to include NUL, but
nobody checked for SP before calling the parsing function.
To correct this, do these three things:
- rename unpack_sha1_header() to unpack_sha1_short_header() and
have unpack_sha1_header_to_strbuf() keep calling that as its
helper function. This will detect and report zlib errors, but is
not aware of the format of a loose object (as before).
- introduce unpack_sha1_header() that calls the same helper
function, and when zlib reports it inflated OK into the buffer,
check if the buffer has both SP and NUL in this order. This
would ensure that parsing function will terminate within the
buffer that holds the inflated header.
- update unpack_sha1_header_to_strbuf() to check if the resulting
buffer has both SP and NUL in this order for the same effect.
Reported-by: Gustavo Grieco <gustavo.grieco@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Unlike the "something like this" version, this does the "we got
some data, does it look like an object header, safely parseable
by our parser?" check in the unpack code, without touching the
parser, as I think that division of labor between the unpacker
and the parser makes more sense.
The strbuf codepath came in 46f03448 ("sha1_file: support reading
from a loose object of unknown type", 2015-05-03) by Karthik,
whose log says it was written by me, and helped by Peff, so I'm
asking these two to lend their eyes.
sha1_file.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..445e763 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1646,7 +1646,9 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
return used;
}
-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
+static int unpack_sha1_short_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
{
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
@@ -1659,13 +1661,37 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
return git_inflate(stream, 0);
}
+int unpack_sha1_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
+{
+ const char *eoh;
+ int status = unpack_sha1_short_header(stream, map, mapsize,
+ buffer, bufsiz);
+
+ if (status < Z_OK)
+ return status;
+
+ /* Make sure we have the terminating NUL */
+ eoh = memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer);
+ if (!eoh)
+ return -1;
+ /* Make sure we have ' ' at the end of type */
+ if (!memchr(buffer, ' ', eoh - (const char *)buffer))
+ return -1;
+ return 0;
+}
+
static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
unsigned long mapsize, void *buffer,
unsigned long bufsiz, struct strbuf *header)
{
+ const char *eoh;
int status;
- status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+ status = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);
+ if (status < Z_OK)
+ return -1;
/*
* Check if entire header is unpacked in the first iteration.
@@ -1686,11 +1712,19 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
status = git_inflate(stream, 0);
strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
- return 0;
+ goto enough;
stream->next_out = buffer;
stream->avail_out = bufsiz;
} while (status != Z_STREAM_END);
return -1;
+
+enough:
+ eoh = memchr(header->buf, '\0', header->len);
+ if (!eoh)
+ die("BUG: the NUL we earlier saw is gone???");
+ if (!memchr(header->buf, ' ', eoh - header->buf))
+ return -1;
+ return 0;
}
static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
^ permalink raw reply related
* Re: Changing the default for "core.abbrev"?
From: Jeff King @ 2016-09-26 4:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq37knwcf4.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 08:46:39PM -0700, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > I can just keep reminding kernel maintainers and developers to update
> > their git config, but maybe it would be a good idea to just admit that
> > the defaults picked in 2005 weren't necessarily the best ones
> > possible, and those could be bumped up a bit?
> >
> > I think I mentioned this some time ago, and it's not a huge deal, but
> > I thought I'd just mention it again because it came up again today for
> > me..
>
> I am not quite sure how good any new default would be, though. Just
> like any timeout is not long enough for somebody, growing projects
> will eventually hit whatever abbreviation length they start with.
I actually think "12" might be sane for a long time. That's 48 bits of
sha1, so we'd expect a 50% change of a _single_ collision at 2^24, or 16
million. The biggest repository I know about (in number of objects) is
the one holding all of the objects for all of the forks of
torvalds/linux on GitHub. It's at about 15 million objects.
Which _seems_ close, but remember that's the size where we expect to see
a single collision. They don't become common until much later (I didn't
compute an exact number, but Linus's 16x sounds about right). I know
that the growth of the kernel isn't really linear, but I think the need
to bump to "13" might not just be decades, but possibly a century or
more.
So 12 seems reasonable, and the only downside for it (or for "13", for
that matter) is a few extra bytes. I dunno, maybe people will really
hate that, but I have a feeling these are mostly cut-and-pasted anyway.
> I am not opposed to bump the default to 12 or whatever, but I
> suspect any lengthening today may need to be accompanied by a tool
> support that finds the set of objects that are reachable from a
> commit whose names begin with non-unique abbreviations that appear
> in the commit log message. Assuming that it is very hard to refer to
> future objects in the log message you write today, such a tool may
> find a single object that used to be the unique instance of that
> abbreviation back then, and with reachability bitmap support, it may
> not be too expensive to run.
I had a similar thought, but I think it's not just reachability. You
might refer to a short sha1 on an alternate branch that isn't reachable
from you. Or you may even use the short sha1 in an email message or a
bug tracker. So I think the extra context you want is probably a
timestamp: at time t, what was a reasonable guess for this sha1?
That's easy to answer for commits and tags (cull the ones that are too
new), but harder for blobs and trees (you'd want to know the earliest
commit which contains them).
An easier (but less automatic) tool would be to improve our error
message for the ambiguous case, and actually report details of the
candidates. I'm working up a patch now.
-Peff
^ permalink raw reply
* Re: Changing the default for "core.abbrev"?
From: Junio C Hamano @ 2016-09-26 4:45 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926043442.3pz7ccawdcsn2kzb@sigill.intra.peff.net>
On Sun, Sep 25, 2016 at 9:34 PM, Jeff King <peff@peff.net> wrote:
>
> An easier (but less automatic) tool would be to improve our error
> message for the ambiguous case, and actually report details of the
> candidates. I'm working up a patch now.
That sounds like a fun little lunch-break project. Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-26 5:02 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Jeff King, Junio C Hamano, git, Swift Geek
In-Reply-To: <0b6cbc53-e058-f064-59e8-b73203f3e400@gmail.com>
On Mon, Sep 26, 2016 at 12:38:42AM +0200, Jakub Narębski wrote:
> W dniu 25.09.2016 o 22:17, Kevin Daudt pisze:
> > On Fri, Sep 23, 2016 at 12:15:41AM -0400, Jeff King wrote:
>
> >> Oops, yes. It is beginning to make the "strbuf_swap()" look less
> >> convoluted. :)
> >>
> >
> > I've switched to strbuf_swap now, much better. I've implemented
> > recursive parsing without looking at what you provided, just to see what
> > I'd came up with. Though I've not implemented a recursive descent
> > parser, but it might suffice.
>
> I think you can implement a parser handling proper nesting of parens
> without recursion.
>
> Though... what is the definition in the RFC?
This part describes comments.
ccontent = ctext / quoted-pair / comment
comment = "(" *([FWS] ccontent) [FWS] ")"
CFWS = *([FWS] comment) (([FWS] comment) / FWS)
So each comment can itself also contain a comment.
This could be done without recursion by keeping a count of how many open
parens we have encountered.
Kevin
^ permalink raw reply
* Re: Changing the default for "core.abbrev"?
From: Matthieu Moy @ 2016-09-26 6:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq37knwcf4.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I am not opposed to bump the default to 12 or whatever, but I
> suspect any lengthening today may need to be accompanied by a tool
> support that finds the set of objects that are reachable from a
> commit whose names begin with non-unique abbreviations that appear
> in the commit log message.
Something much simpler would be to set core.abbrev at clone time,
depending on the size of the project just cloned. So, when cloning a
hello-world, we'd keep the 7 but when cloning a big project we'd get a
larger value.
This doesn't cover the case of someone growing his own project without
cloning, and isn't as clever as actually looking for colision, but it
would probably provide a sane default in 99% cases, and wouldn't be
worse than hardcoding 7 in the 1% remaining cases.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* git-upload-pack hangs
From: Jason Pyeron @ 2016-09-26 5:51 UTC (permalink / raw)
To: git
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?
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-work-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: Changing the default for "core.abbrev"?
From: Christian Couder @ 2016-09-26 7:13 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Git Mailing List,
Ævar Arnfjörð Bjarmason
In-Reply-To: <CA+55aFy0_pwtFOYS1Tmnxipw9ZkRNCQHmoYyegO00pjMiZQfbg@mail.gmail.com>
On Mon, Sep 26, 2016 at 3:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The kernel, these days, is at roughly 5 million objects, and while the
> seven hex digits are still often enough for uniqueness (and git will
> always add digits *until* it is unique), it's long been at the point
> where I tell people to do
>
> git config --global core.abbrev 12
>
> because even though git will extend the seven hex digits until the
> object name is unique, that only reflects the *current* situation in
> the repository. With 5 million objects and a very healthy growth rate,
> a 7-8 hex digit number that is unique today is not necessarily unique
> a month or two from now, and then it gets annoying when a commit
> message has a short git ID that is no longer unique when you go back
> and try to figure out what went wrong in that commit.
AEvar sent a patch recently
(https://public-inbox.org/git/20160921114428.28664-3-avarab@gmail.com/)
to have gitweb link to "git describe"'d commits in log messages, and
this makes me wonder if it woudn't be better for the kernel to also
use the output of a command like `git describe --verylong` or `git
describe --long=12` instead of a regular git ID in commit messages.
^ permalink raw reply
* Re: Request: Extra case for %G? format
From: Michael J Gruber @ 2016-09-26 11:53 UTC (permalink / raw)
To: Alex, git
In-Reply-To: <87d1js1pl1.fsf@gmail.com>
Alex venit, vidit, dixit 25.09.2016 08:05:
> Hello all,
>
> Could the %G? format differentiate between an unsigned commit and a
> signed commit that you're missing a public key for?
>
> If `git show --format=%GG --no-patch <commit>' produces an output like
> the following:
>
> gpg: Signature made <date> using RSA key ID <id>
> gpg: Can't check signature: public key not found
That is the "raw verification message from GPG for a signed commit" as
per git-log(1).
> Then currently %G? results in `N', the same as an unsigned commit.
>
> In this case, could %G? please result in a new character? Perhaps `M'
> for "missing public key"?
Yes, and no.
Really, there are many different reasons why a signature couldn't be
checked, but gpg itself has these status results:
"For each signature only one of the three codes GOODSIG, BADSIG or
ERRSIG will be emitted" (doc/DETAILS in gpg's source).
ERRSIG comes with additional info (RC) that could be parsed for the reason.
Also, in addition to that line, there can be other lines with additional
information. So there is a lot that could potentially be shown (and *is*
shown with %GG). In the GOODSIG case, we parse the TRUST info to take
the trust model into account (and return U for untrusted good).
I wouldn't mind adding E to %G? in the ERRSIG case, even though one has
to look at %GG in any case (N or E) if one wants to have more details.
Cheers,
Michael
^ permalink raw reply
* [PATCH 0/10] helping people resolve ambiguous sha1s
From: Jeff King @ 2016-09-26 11:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <CAPc5daV1YJaEqH5eZCej3nkg8htHVDWQu0V0uoC4gVmPYpDL9Q@mail.gmail.com>
On Sun, Sep 25, 2016 at 09:45:18PM -0700, Junio C Hamano wrote:
> On Sun, Sep 25, 2016 at 9:34 PM, Jeff King <peff@peff.net> wrote:
> >
> > An easier (but less automatic) tool would be to improve our error
> > message for the ambiguous case, and actually report details of the
> > candidates. I'm working up a patch now.
>
> That sounds like a fun little lunch-break project. Thanks.
That's what I thought, but it turned out to be quite involved. :)
I started by trying to teach get_short_sha1() to remember all of the
candidates it sees, but it turns out to be surprisingly complicated. I
did have something working, but I scrapped it in favor of just looking
at the object database again. It's the error code path, so it's OK to be
slower (especially if it keeps the non-error code path much simpler).
But then being the diligent programmer that I am, I added a tests.
And that failed because of an unrelated bug. Fixing that revealed
another bug. And so on.
The good news is that I think I've finally cleared up all of the
long-standing bugs where git will print the same error message twice.
Those have been annoying me for yours (and apparently others[1]).
Patches 2-4 and 9 are all bugfixes. Patch 10 is the interesting part.
The rest are just cleanups and refactoring.
[01/10]: get_sha1: detect buggy calls with multiple disambiguators
[02/10]: get_sha1: avoid repeating ourselves via ONLY_TO_DIE
[03/10]: get_sha1: propagate flags to child functions
[04/10]: get_short_sha1: peel tags when looking for treeish
[05/10]: get_short_sha1: refactor init of disambiguation code
[06/10]: get_short_sha1: NUL-terminate hex prefix
[07/10]: get_short_sha1: mark ambiguity error for translation
[08/10]: sha1_array: let callbacks interrupt iteration
[09/10]: for_each_abbrev: drop duplicate objects
[10/10]: get_short_sha1: list ambiguous objects on error
Of course this is all totally orthogonal to Linus's original question. I
hope it will make things more pleasant when somebody does end up having
to look up a too-short sha1, but it's probably still a good idea to
bump the default.
-Peff
[1] http://public-inbox.org/git/504B91B7.1000406@avtalion.name/
^ permalink raw reply
* [PATCH 01/10] get_sha1: detect buggy calls with multiple disambiguators
From: Jeff King @ 2016-09-26 11:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The get_sha1() family of functions takes a flags field, but
some of the flags are mutually exclusive. In particular, we
can only handle one disambiguating function, and the flags
quietly override each other. Let's instead detect these as
programming bugs.
Technically some of the flags are supersets of the others,
so treating COMMITTISH|TREEISH as just COMMITTISH is not
wrong, but it's a good sign the caller is confused. And
certainly asking for BLOB|TREE does not work.
We can do the check easily with some bit-twiddling, and as a
bonus, the bit-mask of disambiguators will come in handy in
a future patch.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 5 +++++
sha1_name.c | 9 +++++++++
2 files changed, 14 insertions(+)
diff --git a/cache.h b/cache.h
index d0494c8..7bd78ca 100644
--- a/cache.h
+++ b/cache.h
@@ -1203,6 +1203,11 @@ struct object_context {
#define GET_SHA1_FOLLOW_SYMLINKS 0100
#define GET_SHA1_ONLY_TO_DIE 04000
+#define GET_SHA1_DISAMBIGUATORS \
+ (GET_SHA1_COMMIT | GET_SHA1_COMMITTISH | \
+ GET_SHA1_TREE | GET_SHA1_TREEISH | \
+ GET_SHA1_BLOB)
+
extern int get_sha1(const char *str, unsigned char *sha1);
extern int get_sha1_commit(const char *str, unsigned char *sha1);
extern int get_sha1_committish(const char *str, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index faf873c..f9812ff 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -310,6 +310,11 @@ static int prepare_prefixes(const char *name, int len,
return 0;
}
+static int multiple_bits_set(unsigned flags)
+{
+ return !!(flags & (flags - 1));
+}
+
static int get_short_sha1(const char *name, int len, unsigned char *sha1,
unsigned flags)
{
@@ -327,6 +332,10 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
prepare_alt_odb();
memset(&ds, 0, sizeof(ds));
+
+ if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
+ die("BUG: multiple get_short_sha1 disambiguator flags");
+
if (flags & GET_SHA1_COMMIT)
ds.fn = disambiguate_commit_only;
else if (flags & GET_SHA1_COMMITTISH)
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 02/10] get_sha1: avoid repeating ourselves via ONLY_TO_DIE
From: Jeff King @ 2016-09-26 11:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
When the revision code cannot parse an argument like
"HEAD:foo", it will call maybe_die_on_misspelt_object_name(),
which re-runs get_sha1() with an extra ONLY_TO_DIE flag. We
then spend more effort to generate a better error message.
Unfortunately, a side effect is that our second call may
repeat the same error messages from the original get_sha1()
call. You can see this with:
$ git show 0017
error: short SHA1 0017 is ambiguous.
error: short SHA1 0017 is ambiguous.
fatal: ambiguous argument '0017': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
where the second "error:" line comes from the ONLY_TO_DIE
call.
To fix this, we can make ONLY_TO_DIE imply QUIETLY. This is
a little odd, because the whole point of ONLY_TO_DIE is to
output error messages. But what we want to do is tell the
rest of the get_sha1() code (particularly get_sha1_1()) that
the _regular_ messages should be quiet, but the only-to-die
ones should not.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 3 +++
t/t1512-rev-parse-disambiguation.sh | 6 ++++++
2 files changed, 9 insertions(+)
diff --git a/sha1_name.c b/sha1_name.c
index f9812ff..fe05ba0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1391,6 +1391,9 @@ static int get_sha1_with_context_1(const char *name,
const char *cp;
int only_to_die = flags & GET_SHA1_ONLY_TO_DIE;
+ if (only_to_die)
+ flags |= GET_SHA1_QUIETLY;
+
memset(oc, 0, sizeof(*oc));
oc->mode = S_IFINVALID;
ret = get_sha1_1(name, namelen, sha1, flags);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index e221167..16f9709 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -291,4 +291,10 @@ test_expect_success 'ambiguous short sha1 ref' '
grep "refname.*${REF}.*ambiguous" err
'
+test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated' '
+ test_must_fail git rev-parse 00000 2>stderr &&
+ grep "is ambiguous" stderr >errors &&
+ test_line_count = 1 errors
+'
+
test_done
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 03/10] get_sha1: propagate flags to child functions
From: Jeff King @ 2016-09-26 11:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The get_sha1() function is actually implementation by many
sub-functions, but we do not always pass our flags around to
all of those functions. As a result, we may forget that our
caller asked us to resolve with GET_SHA1_QUIETLY and output
messages. The two triggerable cases are:
1. Resolving treeish:path will resolve the "treeish"
portion using GET_SHA1_TREEISH, dropping all other
flags.
2. The peel_onion() function did not take flags at all
but recurses to get_sha1_1(), which does.
The solution for both is to bitwise-OR their new flags with
the existing ones (after dropping any mutually exclusive
disambiguation flags).
This bug can trigger with "git rev-parse --quiet", which
asks for quiet resolution. But it can also happen in a more
vanilla code path when we do a follow-up ONLY_TO_DIE
invocation of get_sha1(), and that's what the tests check.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 18 ++++++++++++------
t/t1512-rev-parse-disambiguation.sh | 14 +++++++++++++-
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index fe05ba0..38e51d9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -686,12 +686,12 @@ struct object *peel_to_type(const char *name, int namelen,
}
}
-static int peel_onion(const char *name, int len, unsigned char *sha1)
+static int peel_onion(const char *name, int len, unsigned char *sha1,
+ unsigned lookup_flags)
{
unsigned char outer[20];
const char *sp;
unsigned int expected_type = 0;
- unsigned lookup_flags = 0;
struct object *o;
/*
@@ -731,10 +731,11 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
else
return -1;
+ lookup_flags &= ~GET_SHA1_DISAMBIGUATORS;
if (expected_type == OBJ_COMMIT)
- lookup_flags = GET_SHA1_COMMITTISH;
+ lookup_flags |= GET_SHA1_COMMITTISH;
else if (expected_type == OBJ_TREE)
- lookup_flags = GET_SHA1_TREEISH;
+ lookup_flags |= GET_SHA1_TREEISH;
if (get_sha1_1(name, sp - name - 2, outer, lookup_flags))
return -1;
@@ -835,7 +836,7 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
return get_nth_ancestor(name, len1, sha1, num);
}
- ret = peel_onion(name, len, sha1);
+ ret = peel_onion(name, len, sha1, lookup_flags);
if (!ret)
return 0;
@@ -1470,7 +1471,12 @@ static int get_sha1_with_context_1(const char *name,
if (*cp == ':') {
unsigned char tree_sha1[20];
int len = cp - name;
- if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {
+ unsigned sub_flags = flags;
+
+ sub_flags &= ~GET_SHA1_DISAMBIGUATORS;
+ sub_flags |= GET_SHA1_TREEISH;
+
+ if (!get_sha1_1(name, len, tree_sha1, sub_flags)) {
const char *filename = cp+1;
char *new_filename = NULL;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 16f9709..30e0b80 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -291,10 +291,22 @@ test_expect_success 'ambiguous short sha1 ref' '
grep "refname.*${REF}.*ambiguous" err
'
-test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated' '
+test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (raw)' '
test_must_fail git rev-parse 00000 2>stderr &&
grep "is ambiguous" stderr >errors &&
test_line_count = 1 errors
'
+test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (treeish)' '
+ test_must_fail git rev-parse 00000:foo 2>stderr &&
+ grep "is ambiguous" stderr >errors &&
+ test_line_count = 1 errors
+'
+
+test_expect_success C_LOCALE_OUTPUT 'ambiguity errors are not repeated (peel)' '
+ test_must_fail git rev-parse 00000^{commit} 2>stderr &&
+ grep "is ambiguous" stderr >errors &&
+ test_line_count = 1 errors
+'
+
test_done
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 04/10] get_short_sha1: peel tags when looking for treeish
From: Jeff King @ 2016-09-26 11:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The treeish disambiguation function tries to peel tags, but
it does so by calling:
deref_tag(lookup_object(sha1), ...);
This will only work if we have previously looked at the tag
and created a "struct tag" for it. Since parsing revision
arguments typically happens before anything else, this is
usually not the case, and we would fail to peel the tag (we
are lucky that deref_tag() gracefully handles the NULL and
does not segfault).
Instead, we can use parse_object(). Note that this is the
same fix done by 94d75d1 (get_short_sha1(): correctly
disambiguate type-limited abbreviation, 2013-07-01), but
that commit fixed only the committish disambiguator, and
left the bug in the treeish one.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 2 +-
t/t1512-rev-parse-disambiguation.sh | 7 +++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 38e51d9..432a308 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -269,7 +269,7 @@ static int disambiguate_treeish_only(const unsigned char *sha1, void *cb_data_un
return 0;
/* We need to do this the hard way... */
- obj = deref_tag(lookup_object(sha1), NULL, 0);
+ obj = deref_tag(parse_object(sha1), NULL, 0);
if (obj && (obj->type == OBJ_TREE || obj->type == OBJ_COMMIT))
return 1;
return 0;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 30e0b80..dfd3567 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -264,6 +264,13 @@ test_expect_success 'ambiguous commit-ish' '
test_must_fail git log 000000000...
'
+# There are three objects with this prefix: a blob, a tree, and a tag. We know
+# the blob will not pass as a treeish, but the tree and tag should (and thus
+# cause an error).
+test_expect_success 'ambiguous tags peel to treeish' '
+ test_must_fail git rev-parse 0000000000f^{tree}
+'
+
test_expect_success 'rev-parse --disambiguate' '
# The test creates 16 objects that share the prefix and two
# commits created by commit-tree in earlier tests share a
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 05/10] get_short_sha1: refactor init of disambiguation code
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The disambiguation machinery has two callers: get_short_sha1
and for_each_abbrev. Both need to repeat much of the same
setup: declaring buffers, sanity-checking lengths, preparing
the prefixes, etc. Let's pull that into a single init
function so we can avoid repeating ourselves.
Pulling the buffers into the "struct disambiguate_state"
isn't strictly necessary, but it does make things simpler
for the callers, who no longer have to worry about sizing
them correctly (i.e., it's an implicit requirement that
the caller provide 20- and 40-byte buffers).
And while we're touching this code, we can convert any
magic-number sizes to the more modern GIT_SHA1_* constants.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 79 +++++++++++++++++++++++++++----------------------------------
1 file changed, 35 insertions(+), 44 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 432a308..79eb1ee 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -13,9 +13,13 @@ static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *)
typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
+ int len; /* length of prefix in hex chars */
+ char hex_pfx[GIT_SHA1_HEXSZ];
+ unsigned char bin_pfx[GIT_SHA1_RAWSZ];
+
disambiguate_hint_fn fn;
void *cb_data;
- unsigned char candidate[20];
+ unsigned char candidate[GIT_SHA1_RAWSZ];
unsigned candidate_exists:1;
unsigned candidate_checked:1;
unsigned candidate_ok:1;
@@ -72,10 +76,10 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
/* otherwise, current can be discarded and candidate is still good */
}
-static void find_short_object_filename(int len, const char *hex_pfx, struct disambiguate_state *ds)
+static void find_short_object_filename(struct disambiguate_state *ds)
{
struct alternate_object_database *alt;
- char hex[40];
+ char hex[GIT_SHA1_HEXSZ];
static struct alternate_object_database *fakeent;
if (!fakeent) {
@@ -95,7 +99,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa
}
fakeent->next = alt_odb_list;
- xsnprintf(hex, sizeof(hex), "%.2s", hex_pfx);
+ xsnprintf(hex, sizeof(hex), "%.2s", ds->hex_pfx);
for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {
struct dirent *de;
DIR *dir;
@@ -103,7 +107,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa
* every alt_odb struct has 42 extra bytes after the base
* for exactly this purpose
*/
- xsnprintf(alt->name, 42, "%.2s/", hex_pfx);
+ xsnprintf(alt->name, 42, "%.2s/", ds->hex_pfx);
dir = opendir(alt->base);
if (!dir)
continue;
@@ -113,7 +117,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa
if (strlen(de->d_name) != 38)
continue;
- if (memcmp(de->d_name, hex_pfx + 2, len - 2))
+ if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
continue;
memcpy(hex + 2, de->d_name, 38);
if (!get_sha1_hex(hex, sha1))
@@ -138,9 +142,7 @@ static int match_sha(unsigned len, const unsigned char *a, const unsigned char *
return 1;
}
-static void unique_in_pack(int len,
- const unsigned char *bin_pfx,
- struct packed_git *p,
+static void unique_in_pack(struct packed_git *p,
struct disambiguate_state *ds)
{
uint32_t num, last, i, first = 0;
@@ -155,7 +157,7 @@ static void unique_in_pack(int len,
int cmp;
current = nth_packed_object_sha1(p, mid);
- cmp = hashcmp(bin_pfx, current);
+ cmp = hashcmp(ds->bin_pfx, current);
if (!cmp) {
first = mid;
break;
@@ -174,20 +176,19 @@ static void unique_in_pack(int len,
*/
for (i = first; i < num && !ds->ambiguous; i++) {
current = nth_packed_object_sha1(p, i);
- if (!match_sha(len, bin_pfx, current))
+ if (!match_sha(ds->len, ds->bin_pfx, current))
break;
update_candidates(ds, current);
}
}
-static void find_short_packed_object(int len, const unsigned char *bin_pfx,
- struct disambiguate_state *ds)
+static void find_short_packed_object(struct disambiguate_state *ds)
{
struct packed_git *p;
prepare_packed_git();
for (p = packed_git; p && !ds->ambiguous; p = p->next)
- unique_in_pack(len, bin_pfx, p, ds);
+ unique_in_pack(p, ds);
}
#define SHORT_NAME_NOT_FOUND (-1)
@@ -281,14 +282,17 @@ static int disambiguate_blob_only(const unsigned char *sha1, void *cb_data_unuse
return kind == OBJ_BLOB;
}
-static int prepare_prefixes(const char *name, int len,
- unsigned char *bin_pfx,
- char *hex_pfx)
+static int init_object_disambiguation(const char *name, int len,
+ struct disambiguate_state *ds)
{
int i;
- hashclr(bin_pfx);
- memset(hex_pfx, 'x', 40);
+ if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
+ return -1;
+
+ memset(ds, 0, sizeof(*ds));
+ memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);
+
for (i = 0; i < len ;i++) {
unsigned char c = name[i];
unsigned char val;
@@ -302,11 +306,14 @@ static int prepare_prefixes(const char *name, int len,
}
else
return -1;
- hex_pfx[i] = c;
+ ds->hex_pfx[i] = c;
if (!(i & 1))
val <<= 4;
- bin_pfx[i >> 1] |= val;
+ ds->bin_pfx[i >> 1] |= val;
}
+
+ ds->len = len;
+ prepare_alt_odb();
return 0;
}
@@ -319,20 +326,12 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
unsigned flags)
{
int status;
- char hex_pfx[40];
- unsigned char bin_pfx[20];
struct disambiguate_state ds;
int quietly = !!(flags & GET_SHA1_QUIETLY);
- if (len < MINIMUM_ABBREV || len > 40)
- return -1;
- if (prepare_prefixes(name, len, bin_pfx, hex_pfx) < 0)
+ if (init_object_disambiguation(name, len, &ds) < 0)
return -1;
- prepare_alt_odb();
-
- memset(&ds, 0, sizeof(ds));
-
if (multiple_bits_set(flags & GET_SHA1_DISAMBIGUATORS))
die("BUG: multiple get_short_sha1 disambiguator flags");
@@ -347,36 +346,28 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
else if (flags & GET_SHA1_BLOB)
ds.fn = disambiguate_blob_only;
- find_short_object_filename(len, hex_pfx, &ds);
- find_short_packed_object(len, bin_pfx, &ds);
+ find_short_object_filename(&ds);
+ find_short_packed_object(&ds);
status = finish_object_disambiguation(&ds, sha1);
if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error("short SHA1 %.*s is ambiguous.", len, hex_pfx);
+ return error("short SHA1 %.*s is ambiguous.", ds.len, ds.hex_pfx);
return status;
}
int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
{
- char hex_pfx[40];
- unsigned char bin_pfx[20];
struct disambiguate_state ds;
- int len = strlen(prefix);
- if (len < MINIMUM_ABBREV || len > 40)
+ if (init_object_disambiguation(prefix, strlen(prefix), &ds) < 0)
return -1;
- if (prepare_prefixes(prefix, len, bin_pfx, hex_pfx) < 0)
- return -1;
-
- prepare_alt_odb();
- memset(&ds, 0, sizeof(ds));
ds.always_call_fn = 1;
ds.cb_data = cb_data;
ds.fn = fn;
- find_short_object_filename(len, hex_pfx, &ds);
- find_short_packed_object(len, bin_pfx, &ds);
+ find_short_object_filename(&ds);
+ find_short_packed_object(&ds);
return ds.ambiguous;
}
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
We store the hex prefix in a 40-byte buffer with the prefix
itself followed by 40-minus-len "x" characters. These x's
serve no purpose, and the lack of NUL termination makes the
prefix string annoying to use. Let's just terminate it.
Note that this is in contrast to the binary prefix, which
_must_ be zero-padded, because we look at the whole thing
during a binary search to find the first potential match in
each pack index. The loose-object hex search cannot use the
same trick because it has to do a linear walk through the
unsorted results of readdir() (and even if it could, you'd
want zeroes instead of x's).
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 79eb1ee..549ef3f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -14,7 +14,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
int len; /* length of prefix in hex chars */
- char hex_pfx[GIT_SHA1_HEXSZ];
+ char hex_pfx[GIT_SHA1_HEXSZ + 1];
unsigned char bin_pfx[GIT_SHA1_RAWSZ];
disambiguate_hint_fn fn;
@@ -291,7 +291,6 @@ static int init_object_disambiguation(const char *name, int len,
return -1;
memset(ds, 0, sizeof(*ds));
- memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);
for (i = 0; i < len ;i++) {
unsigned char c = name[i];
@@ -313,6 +312,7 @@ static int init_object_disambiguation(const char *name, int len,
}
ds->len = len;
+ ds->hex_pfx[len] = '\0';
prepare_alt_odb();
return 0;
}
@@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
status = finish_object_disambiguation(&ds, sha1);
if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error("short SHA1 %.*s is ambiguous.", ds.len, ds.hex_pfx);
+ return error("short SHA1 %s is ambiguous.", ds.hex_pfx);
return status;
}
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 07/10] get_short_sha1: mark ambiguity error for translation
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
This is a human-readable message, and there's no reason it
should not be translated. While we're at it, let's drop the
period from the end, which is not our usual style.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 549ef3f..d4c7e26 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
status = finish_object_disambiguation(&ds, sha1);
if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
- return error("short SHA1 %s is ambiguous.", ds.hex_pfx);
+ return error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
return status;
}
--
2.10.0.492.g14f803f
^ permalink raw reply related
* [PATCH 08/10] sha1_array: let callbacks interrupt iteration
From: Jeff King @ 2016-09-26 12:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <20160926115720.p2yb22lcq37gboon@sigill.intra.peff.net>
The callbacks for iterating a sha1_array must have a void
return. This is unlike our usual for_each semantics, where
a callback may interrupt iteration and have its value
propagated. Let's switch it to the usual form, which will
enable its use in more places (e.g., where we are replacing
an existing iteration with a different data structure).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-sha1-array.txt | 8 ++++++--
builtin/cat-file.c | 3 ++-
builtin/receive-pack.c | 3 ++-
sha1-array.c | 8 ++++++--
sha1-array.h | 8 ++++----
submodule.c | 3 ++-
t/helper/test-sha1-array.c | 3 ++-
7 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/Documentation/technical/api-sha1-array.txt b/Documentation/technical/api-sha1-array.txt
index 3e75497..dcc5294 100644
--- a/Documentation/technical/api-sha1-array.txt
+++ b/Documentation/technical/api-sha1-array.txt
@@ -38,16 +38,20 @@ Functions
`sha1_array_for_each_unique`::
Efficiently iterate over each unique element of the list,
executing the callback function for each one. If the array is
- not sorted, this function has the side effect of sorting it.
+ not sorted, this function has the side effect of sorting it. If
+ the callback returns a non-zero value, the iteration ends
+ immediately and the callback's return is propagated; otherwise,
+ 0 is returned.
Examples
--------
-----------------------------------------
-void print_callback(const unsigned char sha1[20],
+int print_callback(const unsigned char sha1[20],
void *data)
{
printf("%s\n", sha1_to_hex(sha1));
+ return 0; /* always continue */
}
void some_func(void)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 94e67eb..cca97a8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -401,11 +401,12 @@ struct object_cb_data {
struct expand_data *expand;
};
-static void batch_object_cb(const unsigned char sha1[20], void *vdata)
+static int batch_object_cb(const unsigned char sha1[20], void *vdata)
{
struct object_cb_data *data = vdata;
hashcpy(data->expand->oid.hash, sha1);
batch_object_write(NULL, data->opt, data->expand);
+ return 0;
}
static int batch_loose_object(const unsigned char *sha1,
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 896b16f..f7cd180 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -268,9 +268,10 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
return 0;
}
-static void show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
+static int show_one_alternate_sha1(const unsigned char sha1[20], void *unused)
{
show_ref(".have", sha1);
+ return 0;
}
static void collect_one_alternate_ref(const struct ref *ref, void *data)
diff --git a/sha1-array.c b/sha1-array.c
index 6f4a224..af1d7d5 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -42,7 +42,7 @@ void sha1_array_clear(struct sha1_array *array)
array->sorted = 0;
}
-void sha1_array_for_each_unique(struct sha1_array *array,
+int sha1_array_for_each_unique(struct sha1_array *array,
for_each_sha1_fn fn,
void *data)
{
@@ -52,8 +52,12 @@ void sha1_array_for_each_unique(struct sha1_array *array,
sha1_array_sort(array);
for (i = 0; i < array->nr; i++) {
+ int ret;
if (i > 0 && !hashcmp(array->sha1[i], array->sha1[i-1]))
continue;
- fn(array->sha1[i], data);
+ ret = fn(array->sha1[i], data);
+ if (ret)
+ return ret;
}
+ return 0;
}
diff --git a/sha1-array.h b/sha1-array.h
index 72bb33b..b3230be 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -14,10 +14,10 @@ void sha1_array_append(struct sha1_array *array, const unsigned char *sha1);
int sha1_array_lookup(struct sha1_array *array, const unsigned char *sha1);
void sha1_array_clear(struct sha1_array *array);
-typedef void (*for_each_sha1_fn)(const unsigned char sha1[20],
- void *data);
-void sha1_array_for_each_unique(struct sha1_array *array,
- for_each_sha1_fn fn,
+typedef int (*for_each_sha1_fn)(const unsigned char sha1[20],
void *data);
+int sha1_array_for_each_unique(struct sha1_array *array,
+ for_each_sha1_fn fn,
+ void *data);
#endif /* SHA1_ARRAY_H */
diff --git a/submodule.c b/submodule.c
index 0ef2ff4..aba94dd 100644
--- a/submodule.c
+++ b/submodule.c
@@ -728,9 +728,10 @@ void check_for_new_submodule_commits(unsigned char new_sha1[20])
sha1_array_append(&ref_tips_after_fetch, new_sha1);
}
-static void add_sha1_to_argv(const unsigned char sha1[20], void *data)
+static int add_sha1_to_argv(const unsigned char sha1[20], void *data)
{
argv_array_push(data, sha1_to_hex(sha1));
+ return 0;
}
static void calculate_changed_submodule_paths(void)
diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c
index 09f7790..f7a53c4 100644
--- a/t/helper/test-sha1-array.c
+++ b/t/helper/test-sha1-array.c
@@ -1,9 +1,10 @@
#include "cache.h"
#include "sha1-array.h"
-static void print_sha1(const unsigned char sha1[20], void *data)
+static int print_sha1(const unsigned char sha1[20], void *data)
{
puts(sha1_to_hex(sha1));
+ return 0;
}
int cmd_main(int argc, const char **argv)
--
2.10.0.492.g14f803f
^ permalink raw reply related
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