* Re: [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Junio C Hamano @ 2016-09-27 20:35 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20160927202925.GA32565@google.com>
Brandon Williams <bmwill@google.com> writes:
>> s/submodules/submodule-prefix/ at least.
>
> So should the #define be SUPPORT_SUBMODULE_PREFIX instead? That may be
> too narrow minded and not looking toward future submodule options
> support but I'm not sure.
I am not convinced that this prefix needs to be tied/limited to
submodule, at least not yet, though. I view it as a prefix that
points from above the repository's top, of which submodule support
may be a good sample user, but the caller may not necessarily be
doing or interested in "submodule support".
That's also part of figuring out how we want define the semantics of
this thing and how we want to present it to the end-users, I guess,
so we may have to rename it when we know more, but that's OK.
^ permalink raw reply
* Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules
From: Brandon Williams @ 2016-09-27 20:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlgydnqlh.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> > +static const char *submodule_prefix;
>
> I would have expected this to added to environment.c in the previous
> step, but it is OK--I'd imagine you'd grab this from the environment
> and carrying a piece of information from git.c to here by setenv()
> followed by getenv() feels somewhat roundabout, though.
If it would make sense to do the caching of prefix string in
environment.c I can move it there and add a get_submodule_prefix()
function which either reads the envvar or the cached value if its
already been read. Would that be a better route?
>
> > static const char *prefix;
> > static int max_prefix_len;
> > @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> > static void write_name(const char *name)
> > {
> > /*
> > + * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> > + * by the caller.
>
> As Peff mentioned in his review in another thread, a large number of
> functions in git are not reentrant, and I do not think we would want
> to give the impression that those missing a warning are safe to use.
>
> Other than that, this step looks OK. 3/4 and later would be a lot
> more fun to review ;-)
Oh yes, I can remove the comment. Seemed to miss that bit while
rerolling the series.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] rev-list-options: clarify the usage of -n/--max-number
From: Pranit Bauva @ 2016-09-27 20:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqq1t05qoad.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Tue, Sep 27, 2016 at 10:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> -n=<number>, -<number>, --max-number=<number> shows the last n commits
>> specified in <number> irrespective of whether --reverse is used or not.
>> With --reverse, it just shows the last n commits in reverse order.
>
> I think it is easier to understand if you updated the description of
> "--reverse", rather than "-<n>". "rev-list -n $N" that stops after
> showing $N commits is something everybody understands. What often
> dissapoints some users is that "--reverse" kicks in _after_ what
> commits are to be shown are decided.
True.
>> Documentation/rev-list-options.txt | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index 7e462d3..6b7c2e5 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -18,7 +18,7 @@ ordering and formatting options, such as `--reverse`.
>> -<number>::
>> -n <number>::
>> --max-count=<number>::
>> - Limit the number of commits to output.
>> + Limit to last n number of commits to output specified in <number>.
>
> These essentially say the same thing. The original does not mention
> where and how <number> is used, but "Limit the number of commits" as
> a description for "-<number>" would be understood by anybody halfway
> intelligent that the given number is used as that limit, so I do not
> think an updated description is making it easier to understand.
To clear out that confused I used the word "last" but I can now
understand that it can be easily misunderstood.
> There is a paragraph of interest in an earlier part of "Commit
> Limiting" section (which is the section "-n" appears in, among other
> options):
>
> Note that these are applied before commit
> ordering and formatting options, such as `--reverse`.
>
> So the documentation already makes an attempt to avoid confusion
> Ruediger saw, i.e. "rev-list traverses, limits the output to N, and
> then shows these N commits in reverse" is what it expects readers to
> understand, and that it also expects it would lead naturally to
> "these N commits are still from the newest part of the history,
> hence 'rev-list --reverse -n N' is not how you grab the earliest N".
It surely does :)
> But apparently the attempt by the current documentation is not
> enough. Let's see how it describes the '--reverse' option:
>
> Commit Ordering
> ~~~~~~~~~~~~~~~
>
> By default, the commits are shown in reverse chronological order.
> ...
>
> --reverse::
> Output the commits in reverse order.
> Cannot be combined with `--walk-reflogs`.
>
> Perhaps "Output the commits chosen to be shown (see Commit Limiting
> section above) in reverse order." would make it clearer?
That would be a much better edit. Thanks! Will send out a re-roll.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Brandon Williams @ 2016-09-27 20:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwd1nr56.fsf@gitster.mtv.corp.google.com>
On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > +--submodule-prefix=<path>::
> > + Set a prefix which gives submodules context about the superproject that
> > + invoked it. Only allowed for commands which support submodules.
>
> This, and also the message in die(), uses a phrase "support
> submodules", but it is unclear what it exactly means to the end
> users and readers.
>
> A "ls-files" that is recursively run as an implementation detail of
> the "grep --recurse-submodules" would be taught to support this
> option with this series. Who is supporting submodules in that
> context?
>
> I'd imagine (close to) 100% of the people would say it is "grep"
> that is supporting submodules, not "ls-files", but what this
> paragraph and die() message want to express by the phrase "support
> submodules" is the fact that "ls-files" knows how to react to
> "--submodule-prefix" option.
>
> I'd suggest not to worry too much about this phrasing at this point,
> until we figure out exactly how we want to present these to end
> users. For now, perhaps drop the second sentence and replace it
> with "The end-users are not expected to use this option" or
> something like that?
K can do. The intention is that each command has to do whatever
internal rework needed so that it understands how to interact with
subomodules (be that calling another command or internally supporting
it).
> > + die("%s doesn't support submodules", p->cmd);
>
> s/submodules/submodule-prefix/ at least.
So should the #define be SUPPORT_SUBMODULE_PREFIX instead? That may be
too narrow minded and not looking toward future submodule options
support but I'm not sure.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
From: Junio C Hamano @ 2016-09-27 20:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <29e75b7b-6dd0-8c52-e444-cad1ba613cd0@web.de>
René Scharfe <l.s.r@web.de> writes:
> diff --git a/diff.c b/diff.c
> index a178ed3..be11e4e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
> }
> strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
> find_unique_abbrev(one->oid.hash, abbrev));
> - strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> + strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
OK.
> diff --git a/submodule.c b/submodule.c
> index dcc5ce3..8cf40ea 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
> find_unique_abbrev(one->hash, DEFAULT_ABBREV));
> if (!fast_backward && !fast_forward)
> strbuf_addch(&sb, '.');
> - strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> + strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
I wonder how could this change come out of this definition:
@@
expression E1, E2, E3;
@@
- strbuf_addstr(E1, find_unique_abbrev(E2, E3));
+ strbuf_add_unique_abbrev(E1, E2, E3);
^ permalink raw reply
* Re: [PATCH] gitignore: ignore output files of coccicheck make target
From: René Scharfe @ 2016-09-27 20:12 UTC (permalink / raw)
To: Jakub Narębski, Git List; +Cc: Junio C Hamano
In-Reply-To: <745b0eed-4c03-9e72-4903-ab1dbbc3622d@gmail.com>
Am 27.09.2016 um 21:52 schrieb Jakub Narębski:
> W dniu 27.09.2016 o 21:01, René Scharfe pisze:
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>> .gitignore | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/.gitignore b/.gitignore
>> index 05cb58a..f370ba0 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>
> Wouldn't it be better to have this in contrib/coccinelle/.gitignore?
True.
-- >8 --
Subject: [PATCH v2] gitignore: ignore output files of coccicheck make target
Helped-by: Jakub Narębski <jnareb@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/.gitignore | 1 +
1 file changed, 1 insertion(+)
create mode 100644 contrib/coccinelle/.gitignore
diff --git a/contrib/coccinelle/.gitignore b/contrib/coccinelle/.gitignore
new file mode 100644
index 0000000..d3f2964
--- /dev/null
+++ b/contrib/coccinelle/.gitignore
@@ -0,0 +1 @@
+*.patch*
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-27 20:11 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <xmqqh991nq34.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> we seem to avoid using "tr", even though q_to_cr and others do use
> it. I wonder if we had some portability issues with passing NUL
> through tr or something?
>
> ... digs and finds e85fe4d8 ("more tr portability test script
> fixes", 2008-03-12)
>
> So use something like
>
> perl -pe 'y/\012/\000/' <<\-EOF
> ...
> EOF
>
> instead, perhaps?
I actually think it would make more sense to add
lf_to_nul () {
perl -pe 'y/\012/\000/'
}
to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
this route.
^ permalink raw reply
* Re: [PATCH 4/4 v4] ls-files: add pathspec matching for submodules
From: Junio C Hamano @ 2016-09-27 20:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-5-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> - /* Find common prefix for all pathspec's */
> - max_prefix = common_prefix(&pathspec);
> + /*
> + * Find common prefix for all pathspec's
> + * This is used as a performance optimization which unfortunately cannot
> + * be done when recursing into submodules
> + */
> + if (recurse_submodules)
> + max_prefix = NULL;
> + else
> + max_prefix = common_prefix(&pathspec);
> max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
I still wonder if we can do better than this, as this would be a big
cycle-saver especially in recurse-submodules case.
When you get max_prefix that is "a/b/c", there are three cases:
* a/b/c is a path prefix for an entry in the index, e.g. a/b/c/d;
you then can safely use it and you do not have to do any
recursive invocation of ls-files outside "a/b/c". You may match
a/b/c/d in the toplevel, or you may recurse a/b/c/e that is a
submodule, but you won't have to pay attention to submodules
outside.
* a leading path of a/b/c, e.g. a/b, is a gitlink or a blob in the
index; you can use a/b and you only have to recurse into a/b if
that is a submodule; if a/b is a blob, you'd show nothing.
* a/b/c itself and no leading path of it appears in the index; you
know that nothing will match once you know that you are in this
situation.
Because a gitlink "a/b" sorts at the same location in the index as a
regular blob "a/b" would, by feeding the max_prefix common_prefix()
gives you (i.e. "a/b/c") to index_name_pos() to see which one of the
three situations you are in can be done fairly cheaply, I would
think. The index_name_pos() call may find "a/b/c" exactly (case 1),
or return a location where "a/b/c" would be inserted in the list of
existing entries. If there were "a/b" (or "a") in the index, there
wouldn't be any "a/b/x" (or "a/x") at the same time, so a query for
"a/b/c" would land you next to (just after) an existing entry that
is a leading path of it, if such an entry exists, no? That would
allow you to tell case 2 above fairly cheaply, I would expect.
It is a separate issue if adding that support to 4/4 is a good idea;
I personally think doing it as a separate follow-up patch would make
more sense, so all of the above is tangent.
^ permalink raw reply
* Re: [PATCH] gitignore: ignore output files of coccicheck make target
From: Jakub Narębski @ 2016-09-27 19:52 UTC (permalink / raw)
To: René Scharfe, Git List; +Cc: Junio C Hamano
In-Reply-To: <93adb7c5-aac7-7241-e383-3ff7997faf7e@web.de>
W dniu 27.09.2016 o 21:01, René Scharfe pisze:
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> .gitignore | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/.gitignore b/.gitignore
> index 05cb58a..f370ba0 100644
> --- a/.gitignore
> +++ b/.gitignore
Wouldn't it be better to have this in contrib/coccinelle/.gitignore?
> @@ -207,6 +207,7 @@
> /tags
> /TAGS
> /cscope*
> +/contrib/coccinelle/*.patch*
> *.obj
> *.lib
> *.res
^ permalink raw reply
* Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()
From: Jakub Narębski @ 2016-09-27 19:33 UTC (permalink / raw)
To: Jeff King, Lars Schneider
Cc: git, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160927083939.par2ea5nqjrtkkjs@sigill.intra.peff.net>
W dniu 27.09.2016 o 10:39, Jeff King pisze:
> On Mon, Sep 26, 2016 at 09:21:10PM +0200, Lars Schneider wrote:
>
>> On 25 Sep 2016, at 13:26, Jakub Narębski <jnareb@gmail.com> wrote:
>>
>>> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>>>> From: Lars Schneider <larsxschneider@gmail.com>
>>>> ...
>>>>
>>>> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>>>
>>> I'm not sure what naming convention the rest of Git uses, but isn't
>>> it more like '*data' rather than '*buf' here?
>>
>> pkt-line seems to use 'buf' or 'buffer' for everything else.
>
> I do not think we have definite rules, but I would generally expect to
> see "data" as an opaque thing (e.g., passing "void *data" to callbacks).
> "buf" or "buffer" makes sense here, but I don't think it really matters
> that much either way.
True.
>>>> + static char packet_write_buffer[LARGE_PACKET_MAX];
>>>
>>> I think there should be warning (as a comment before function
>>> declaration, or before function definition), that packet_write_gently()
>>> is not thread-safe (nor reentrant, but the latter does not matter here,
>>> I think).
>>>
>>> Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058
>>>
>>> This is not something terribly important; I guess git code has tons
>>> of functions not marked as thread-unsafe...
>>
>> I agree that the function is not thread-safe. However, I can't find
>> an example in the Git source that marks a function as not thread-safe.
>> Unless is it explicitly stated in the coding guidelines I would prefer
>> not to start way to mark functions.
There is *one* example: "fill_textconv is not remotely thread-safe;"
comment in grep.c, but not in diff.{c,h} where it is declared/defined.
Also, it is static function; we should know if it is thread-safe
or not.
I am thinking about supporting streaming in the future, and perhaps
also running different filter drivers (for different files) in parallel.
I guess that using "static __thread char packet_write_buffer[...]"
is out of question (still not reentrant)?
>
> I'd agree. A large number of functions in git are not reentrant, and I
> would not want to give the impression that those missing a warning are
> safe to use.
The fact tha git code is undercommented and underdocumented does not
mean that we should not add comments and documentation.
>
>>>> + if (size > sizeof(packet_write_buffer) - 4) {
>>>
>>> First, wouldn't the following be more readable:
>>>
>>> + if (size + 4 > LARGE_PACKET_MAX) {
>>
>> Peff suggested that here:
>> http://public-inbox.org/git/20160810132814.gqnipsdwyzjmuqjy@sigill.intra.peff.net/
>
> There is a good reason to do size checks as a subtraction from a known
> quantity: you can be sure that you are not introducing an overflow
> (e.g., Jakub's suggestion does the wrong thing when "size" is within 4
> bytes of its maximum value). That's unlikely in this case, but then so
> is the size exceeding LARGE_PACKET_MAX in the first place (arguably this
> should be a die("BUG"), because it is the caller's responsibility to
> split their packets.
Right. I should train myself to watch for overflows.
--
Jakub Narębski
^ permalink raw reply
* Re: [Q] would it be bad to make /etc/gitconfig runtime configurable?
From: Jeff King @ 2016-09-27 19:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa39p926.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 27, 2016 at 10:05:37AM -0700, Junio C Hamano wrote:
> The subject says it all. Would it be bad if we introduce an
> environment variable, GIT_SYSTEM_CONFIG=/etc/gitconfig, that names
> an alternative location of the system-wide configuration file?
>
> That would supersede/deprecate GIT_CONFIG_NOSYSTEM that we
> introduced primarily so that we can run our tests without getting
> affected by the configuration that happens to be effective on the
> host that the test is being run.
I can't think of a reason it would be bad. There shouldn't be a security
question, as controlling the environment already lets you run arbitrary
code, override system config variables, etc.
I know when I was writing the tests for 0d44a2d (config: return
configset value for current_config_ functions, 2016-05-26) I had to omit
the "system" case, because we had no way of pointing at a specific file.
So I do not know whether a runtime system config variable would be
useful for users, but it would definitely make testing easier.
-Peff
^ permalink raw reply
* Re: [PATCH] xdiff: rename "struct group" to "struct xdlgroup"
From: Jeff King @ 2016-09-27 19:21 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <9fb14a41-00b3-f8d1-d8d2-8aa41261492e@alum.mit.edu>
On Tue, Sep 27, 2016 at 03:18:02PM +0200, Michael Haggerty wrote:
> > Let's resolve by giving the xdiff variant a scoped name,
> > which is closer to other xdiff types anyway (e.g.,
> > xdlfile_t, though note that xdiff is fond if typedefs when
> > Git usually is not).
>
> Makes sense to me. I didn't try to adhere to xdiff conventions too
> tightly because I don't think that project is alive anymore, so I don't
> expect we'll be upstreaming anything [1]. But this change definitely
> makes sense.
Yeah, TBH I don't really care about following xdiff coding conventions.
They're pretty far from our own, and at this point I think xdiff is
basically just an imported part of our code base. Mostly my rationale
was that it's not too terribly out of place to give it an "xdl" name,
and it happens to solve my other problem, too. :)
> [1] Though I've since learned that libgit2 also bases their diff code on
> xdiff, so if we avoid changing things gratuitously there is more chance
> that our two projects can benefit from each other's improvements
> whenever they are also licensed compatibly.
I'd agree on not changing things gratuitously.
-Peff
^ permalink raw reply
* thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
From: Jeff King @ 2016-09-27 19:19 UTC (permalink / raw)
To: David Turner; +Cc: git, mhagger, David Turner
In-Reply-To: <1474989574.26902.7.camel@frank>
On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:
> > typedef void (*err_fn)(const char *, ...);
> >
> > static int decode_tree_entry(struct tree_desc *desc,
> > const char *buf, unsigned long size,
> > err_fn err)
> > {
> > ...
> > if (size < 23 || buf[size - 21]) {
> > err("too-short tree object");
> > return -1;
> > }
> > }
> >
> > I dunno. Maybe that is overengineering. I guess we only hit the strbufs
> > in the error path (which used to die!), so nobody really cares that much
> > about the extra allocation.
>
> I don't really like err_fn because:
> (a) without a baton, it's somewhat less general (or less thread-safe)
> than the strbuf approach and
> (b) with a baton, it's two arguments instead of one.
I'm going to ramble for a minute, and I don't think it's worth exploring
for this patch series in particular, so feel free to ignore me.
I think this error concept could be extended fairly elegantly with
something like:
typedef void (*err_fn)(void *, const char *fmt, va_list ap)
struct error_context {
err_fn fn;
void *data;
};
int report_error(struct error_context *err, const char *fmt, ...)
{
if (err->fn) {
va_list ap;
va_start(ap, fmt);
err->fn(err->data, fmt, ap);
va_end(ap);
}
return -1;
}
Then low-level functions just take a context and do:
return report_error(&err, "some error: %s", foo);
And then the callers would pick one of a few generic error contexts:
- passing NULL silences the errors
- a global for chaining to error, like:
struct error_context print_errors = {
error, /* actually a wrapper to handle va_list and NULL data */
NULL
};
- a context that collects errors in a strbuf (or list, etc)
struct strbuf err_buf = STRBUF_INIT;
struct error_context = STRBUF_ERR_CONTEXT(&err_buf);
And that error_context can be passed around like a baton through several
functions.
I remember having a big discussion about error-passing patterns around
the ref refactoring, but I don't remember this particular thing coming
up.
Anyway, this is all way outside the scope of what we should consider for
your current series. If we were to do something like this, it would make
sense to start using it consistently. This discussion just made me think
of it.
-Peff
^ permalink raw reply
* Re: Possible integer overflow parsing malformed objects in git 2.10.0
From: Gustavo Grieco @ 2016-09-27 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqponpqqri.fsf@gitster.mtv.corp.google.com>
----- Original Message -----
> Jeff King <peff@peff.net> writes:
>
> > 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.
Great, it is exactly the type of analysis i was expecting.
>
> I agree with the overall conclusion. This does look like an attempt
> to throw random fuzz at Git and see if and how it breaks, and in this
> particular one Git is simply doing the right thing (and the fault lies
> in the way how ASAN is used and how its result was interpreted).
>
> Throwing random fuzz to see what breaks is not a bad thing to do
> per-se, but anybody who does so without wearing a black hat needs to
> keep two things in mind:
>
> * When a random fuzz attempt does uncover a security issue,
> reporting it here on this list is a grossly irresponsible way to
> disclose the issue. We have the git-security list for that.
That is reasonable, indeed. As we discussed, this type of issues are very unlikely to be easily exploited (or even possible), so i did not think that it could be irresponsible to post this issue here. I will be happy to post only in git-security if you think my reports can uncover security issues (but so far, it was not the case). It is also interesting to mention that git-security is not linked anywhere in the official website (git-scm.com) or the github repository (github.com/git/git).
>
> * A random fuzz may stop Git and that may be perfectly legit thing
> to happen, e.g. the data may request a large but still valid
> amount of memory to be allocated that happens not to fit in the
> hardware the fuzz attempt is being run, and xmalloc() may detect
> the situation and die, like the above example. False positives
> are expected and you want to make sure you cull them before
> making your reports. Otherwise, they will unnecessary burden
> people who are doing real work, i.e. reproduce and correct
> problems that may be security related that are irresponsibly
> disclosed here quickly enough to minimize damage.
I try to discard false positives as much as possible. Despite we just started adding the git generation module to our tool, we got hundreds of aborts caused by the allocation limits of AddressSanitizer: I only reported the one which specifically contains a size that could be interpreted as negative number (in fact, valgrind will also report this issue a 'fishy size' for malloc).
Also, I think my reports should are clear enough to reproduce any issue and i carefully check every test case for reproducibility, still i am open to suggestion on how to improve my bug reports!
>
> Thanks.
>
>
^ permalink raw reply
* [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
From: René Scharfe @ 2016-09-27 19:11 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <b7a61c78-3c46-375c-4cc3-7c363e551bc6@web.de>
Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
instead of taking detours through find_unique_abbrev() and its static
buffer. This is shorter and a bit more efficient.
1eb47f167d65d1d305b9c196a1bb40eb96117cb1 already converted six cases,
this patch covers three more.
A semantic patch for Coccinelle is included for easier checking for
new cases that might be introduced in the future.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/strbuf.cocci | 6 ++++++
diff.c | 2 +-
submodule.c | 2 +-
wt-status.c | 3 +--
4 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 4b7553f..1e24298 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -9,3 +9,9 @@ expression E1, E2;
@@
- strbuf_addf(E1, "%s", E2);
+ strbuf_addstr(E1, E2);
+
+@@
+expression E1, E2, E3;
+@@
+- strbuf_addstr(E1, find_unique_abbrev(E2, E3));
++ strbuf_add_unique_abbrev(E1, E2, E3);
diff --git a/diff.c b/diff.c
index a178ed3..be11e4e 100644
--- a/diff.c
+++ b/diff.c
@@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
}
strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
find_unique_abbrev(one->oid.hash, abbrev));
- strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
+ strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
if (one->mode == two->mode)
strbuf_addf(msg, " %06o", one->mode);
strbuf_addf(msg, "%s\n", reset);
diff --git a/submodule.c b/submodule.c
index dcc5ce3..8cf40ea 100644
--- a/submodule.c
+++ b/submodule.c
@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
- strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
+ strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);
if (message)
strbuf_addf(&sb, " %s%s\n", message, reset);
else
diff --git a/wt-status.c b/wt-status.c
index 9628c1d..99d1b0a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1383,8 +1383,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
if (!strcmp(cb->buf.buf, "HEAD")) {
/* HEAD is relative. Resolve it to the right reflog entry. */
strbuf_reset(&cb->buf);
- strbuf_addstr(&cb->buf,
- find_unique_abbrev(nsha1, DEFAULT_ABBREV));
+ strbuf_add_unique_abbrev(&cb->buf, nsha1, DEFAULT_ABBREV);
}
return 1;
}
--
2.10.0
^ permalink raw reply related
* [PATCH 1/2] use strbuf_addstr() instead of strbuf_addf() with "%s", part 2
From: René Scharfe @ 2016-09-27 19:08 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Replace uses of strbuf_addf() for adding strings with more lightweight
strbuf_addstr() calls. This is shorter and makes the intent clearer.
bc57b9c0cc5a123365a922fa1831177e3fd607ed already converted three cases,
this patch covers two more.
A semantic patch for Coccinelle is included for easier checking for
new cases that might be introduced in the future.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/submodule--helper.c | 2 +-
contrib/coccinelle/strbuf.cocci | 6 ++++++
submodule.c | 2 +-
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e3fdc0a..444ec06 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -753,7 +753,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
if (suc->recursive_prefix)
strbuf_addf(&sb, "%s/%s", suc->recursive_prefix, ce->name);
else
- strbuf_addf(&sb, "%s", ce->name);
+ strbuf_addstr(&sb, ce->name);
strbuf_addf(out, _("Skipping unmerged submodule %s"), sb.buf);
strbuf_addch(out, '\n');
goto cleanup;
diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 7932d48..4b7553f 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -3,3 +3,9 @@ expression E1, E2;
@@
- strbuf_addf(E1, E2);
+ strbuf_addstr(E1, E2);
+
+@@
+expression E1, E2;
+@@
+- strbuf_addf(E1, "%s", E2);
++ strbuf_addstr(E1, E2);
diff --git a/submodule.c b/submodule.c
index 0ef2ff4..dcc5ce3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
find_unique_abbrev(one->hash, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
- strbuf_addf(&sb, "%s", find_unique_abbrev(two->hash, DEFAULT_ABBREV));
+ strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
if (message)
strbuf_addf(&sb, " %s%s\n", message, reset);
else
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v3 2/3] fsck: handle bad trees like other errors
From: Jeff King @ 2016-09-27 19:03 UTC (permalink / raw)
To: David Turner; +Cc: git
In-Reply-To: <1474989806-5002-2-git-send-email-dturner@twosigma.com>
On Tue, Sep 27, 2016 at 11:23:25AM -0400, David Turner wrote:
> +test_expect_success 'unparseable tree object' '
> + test_when_finished "git update-ref -d refs/heads/wrong" &&
> + test_when_finished "remove_object \$tree_sha1" &&
> + test_when_finished "remove_object \$commit_sha1" &&
> + tree_sha1=$(printf "100644 \0twenty-bytes-of-junk" | git hash-object -t tree --stdin -w --literally) &&
> + commit_sha1=$(git commit-tree $tree_sha1) &&
> + git update-ref refs/heads/wrong $commit_sha1 &&
> + test_must_fail git fsck 2>out &&
> + test_i18ngrep "error: empty filename in tree entry" out &&
> + test_i18ngrep "$tree_sha1" out &&
> + ! test_i18ngrep "fatal: empty filename in tree entry" out
> +'
Unfortunately this last one needs to be spelled as:
test_i18ngrep ! "fatal: empty filename in tree entry" out
because the function always pretends to match when GETTEXT_POISON is
set.
Other than the minor test fixups, this all looks good to me.
-Peff
^ permalink raw reply
* [PATCH] gitignore: ignore output files of coccicheck make target
From: René Scharfe @ 2016-09-27 19:01 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore
index 05cb58a..f370ba0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -207,6 +207,7 @@
/tags
/TAGS
/cscope*
+/contrib/coccinelle/*.patch*
*.obj
*.lib
*.res
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v3 1/3] tree-walk: be more specific about corrupt tree errors
From: Jeff King @ 2016-09-27 18:59 UTC (permalink / raw)
To: David Turner; +Cc: git
In-Reply-To: <1474989806-5002-1-git-send-email-dturner@twosigma.com>
On Tue, Sep 27, 2016 at 11:23:24AM -0400, David Turner wrote:
> +test_expect_success 'malformed mode in tree' '
> + hex_sha1=$(echo foo | git hash-object --stdin -w) &&
> + bin_sha1=$(echo $hex_sha1 | perl -ne "printf \"\\\\%03o\", ord for /../g") &&
Sorry, the perl snippet I posted earlier was totally braindead. It gives
you the octal for the raw bytes, but we really just want to convert the
hex to octal (we could also print the raw bytes from the hex, but I
didn't want to take chances on shells that can't handle NULs in
environment variables).
I also find it helps to define a helper function outside of the test
block to avoid quoting hell. So something like:
hex2oct () {
perl -ne 'printf "\\%03o", hex for /../g'
}
test_expect_success ... '
bin_sha1=$(echo $hex_sha1 | hex2oct)
'
-Peff
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-27 18:43 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> if (recurse_submodules &&
> - (show_stage || show_deleted || show_others || show_unmerged ||
> + (show_deleted || show_others || show_unmerged ||
> show_killed || show_modified || show_resolve_undo ||
> - show_valid_bit || show_tag || show_eol || with_tree ||
> - (line_terminator == '\0')))
> + with_tree))
> die("ls-files --recurse-submodules unsupported mode");
Ahh, one more thing, and this comment probably applies to 2/4 not
this one, but if the intention is to shrink this "not supported yet"
check as the series progresses, in the earlier step the check would
need to make sure no pathspec is given, which is first supported in
4/4, I think. It is not a big deal to require rerolling by itself,
bit if you are rerolling 2/4 for other reasons, then...
^ permalink raw reply
* Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Junio C Hamano @ 2016-09-27 18:40 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Pass through some known-safe options when recursing into submodules.
> (--cached, --stage, -v, -t, -z, --debug, --eol)
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> builtin/ls-files.c | 34 ++++++++++++++++++++++++++++++----
> t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++++++-----
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index d4bfc60..a39367f 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -31,6 +31,7 @@ static int debug_mode;
> static int show_eol;
> static int recurse_submodules;
> static const char *submodule_prefix;
> +static struct argv_array recurse_submodules_opts = ARGV_ARRAY_INIT;
I'd imagine that this is also thread-unsafe, but we do not have to
comment it ;-)
> @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
> }
> }
>
> +/*
> + * Compile an argv_array with all of the options supported by --recurse_submodules
> + */
> +static void compile_submodule_options(int show_tag)
> +{
> + if (show_cached)
> + argv_array_push(&recurse_submodules_opts, "--cached");
> + if (show_stage)
> + argv_array_push(&recurse_submodules_opts, "--stage");
> + if (show_valid_bit)
> + argv_array_push(&recurse_submodules_opts, "-v");
> + if (show_tag)
> + argv_array_push(&recurse_submodules_opts, "-t");
> + if (line_terminator == '\0')
> + argv_array_push(&recurse_submodules_opts, "-z");
> + if (debug_mode)
> + argv_array_push(&recurse_submodules_opts, "--debug");
> + if (show_eol)
> + argv_array_push(&recurse_submodules_opts, "--eol");
> +}
OK. These are only the safe ones to pass through? "compile" or
"assemble" is much less important thing to say than how these are
chosen. "pass_supported_options()" or something? I dunno.
> if (recurse_submodules &&
> - (show_stage || show_deleted || show_others || show_unmerged ||
> + (show_deleted || show_others || show_unmerged ||
> show_killed || show_modified || show_resolve_undo ||
> - show_valid_bit || show_tag || show_eol || with_tree ||
> - (line_terminator == '\0')))
> + with_tree))
> die("ls-files --recurse-submodules unsupported mode");
Makes sense.
> +test_expect_success 'ls-files correctly outputs files in submodule with -z' '
> + cat | tr "\n" "\0" >expect <<-\EOF &&
> + .gitmodules
> + a
> + b/b
> + submodule/c
> + EOF
Hmm, what do you need "cat" for here?
In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
we seem to avoid using "tr", even though q_to_cr and others do use
it. I wonder if we had some portability issues with passing NUL
through tr or something?
... digs and finds e85fe4d8 ("more tr portability test script
fixes", 2008-03-12)
So use something like
perl -pe 'y/\012/\000/' <<\-EOF
...
EOF
instead, perhaps?
> + git ls-files --recurse-submodules -z >actual &&
> + test_cmp expect actual
> +'
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Pühringer @ 2016-09-27 18:37 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Santiago Torres, git@vger.kernel.org, Jeff King,
Eric Sunshine, walters
In-Reply-To: <CAGZ79kYitOd5-Cxr0VzhRH70B=z2gh1q2aB5xiwe+HwHuU7dYg@mail.gmail.com>
I made it a habit to use ‘ue’ instead of ‘ü' outside of German speaking countries and in coding. It makes my life easier.
But thanks for the hint.
Lukas
> On Sep 27, 2016, at 2:31 PM, Stefan Beller <sbeller@google.com> wrote:
>
> On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
> <luk.puehringer@gmail.com> wrote:
>> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
>
> No need to stay full ASCII. German umlauts are fine.
> (See `git shortlog -s` for all the contributor names, there are also
> other alphabets in use)
>
> Stefan
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Stefan Beller @ 2016-09-27 18:31 UTC (permalink / raw)
To: Lukas Pühringer
Cc: Junio C Hamano, Santiago Torres, git@vger.kernel.org, Jeff King,
Eric Sunshine, walters
In-Reply-To: <3D863101-CA73-4718-A1E7-A1664A5BDB58@gmail.com>
On Tue, Sep 27, 2016 at 11:25 AM, Lukas Pühringer
<luk.puehringer@gmail.com> wrote:
> Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
No need to stay full ASCII. German umlauts are fine.
(See `git shortlog -s` for all the contributor names, there are also
other alphabets in use)
Stefan
^ permalink raw reply
* Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules
From: Junio C Hamano @ 2016-09-27 18:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1474930003-83750-3-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules. This is done by forking off a
> process to recursively call ls-files on all submodules. Use top-level
> --submodule_prefix option to pass a path to the submodule which it can
> use to prepend to output or pathspec matching logic.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/git-ls-files.txt | 7 +-
> builtin/ls-files.c | 143 ++++++++++++++++++++++++---------
> git.c | 2 +-
> t/t3007-ls-files-recurse-submodules.sh | 100 +++++++++++++++++++++++
> 4 files changed, 212 insertions(+), 40 deletions(-)
> create mode 100755 t/t3007-ls-files-recurse-submodules.sh
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 0d933ac..446209e 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -18,7 +18,8 @@ SYNOPSIS
> [--exclude-per-directory=<file>]
> [--exclude-standard]
> [--error-unmatch] [--with-tree=<tree-ish>]
> - [--full-name] [--abbrev] [--] [<file>...]
> + [--full-name] [--recurse-submodules]
> + [--abbrev] [--] [<file>...]
>
> DESCRIPTION
> -----------
> @@ -137,6 +138,10 @@ a space) at the start of each line:
> option forces paths to be output relative to the project
> top directory.
>
> +--recurse-submodules::
> + Recursively calls ls-files on each submodule in the repository.
> + Currently there is only support for the --cached mode.
> +
> --abbrev[=<n>]::
> Instead of showing the full 40-byte hexadecimal object
> lines, show only a partial prefix.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 00ea91a..d4bfc60 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
> #include "resolve-undo.h"
> #include "string-list.h"
> #include "pathspec.h"
> +#include "run-command.h"
>
> static int abbrev;
> static int show_deleted;
> @@ -28,6 +29,8 @@ static int show_valid_bit;
> static int line_terminator = '\n';
> static int debug_mode;
> static int show_eol;
> +static int recurse_submodules;
> +static const char *submodule_prefix;
I would have expected this to added to environment.c in the previous
step, but it is OK--I'd imagine you'd grab this from the environment
and carrying a piece of information from git.c to here by setenv()
followed by getenv() feels somewhat roundabout, though.
> static const char *prefix;
> static int max_prefix_len;
> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> static void write_name(const char *name)
> {
> /*
> + * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> + * by the caller.
As Peff mentioned in his review in another thread, a large number of
functions in git are not reentrant, and I do not think we would want
to give the impression that those missing a warning are safe to use.
Other than that, this step looks OK. 3/4 and later would be a lot
more fun to review ;-)
^ permalink raw reply
* Re: [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: Lukas Pühringer @ 2016-09-27 18:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: santiago, git, peff, sunshine, walters
In-Reply-To: <xmqqponpnqyf.fsf@gitster.mtv.corp.google.com>
Good, I will change it to 'Lukas Puehringer' then, when we send you the updated batch of patches, that address your latest comments.
Thanks,
Lukas
> On Sep 27, 2016, at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lukas Pühringer <luk.puehringer@gmail.com> writes:
>
>> Thanks for checking. I am fine with Lukas P, unless git prefers
>> full last names. In that case I am fine with changing too.
>
> We do prefer full names, so that it would be consistent with court
> document when you are involved in copyright inflingement case ;-)
^ 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