On Tue, May 22, 2012 at 04:23:36PM -0400, Jeff King wrote:
> On Tue, May 22, 2012 at 01:08:42PM -0700, Junio C Hamano wrote:
>
> > > @@ -1076,6 +1081,8 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
> > > st.st_mtime = 0;
> > > }
> > >
> > > + qsort(heads, nr_heads, sizeof(*heads), compare_heads);
> > > +
> > > if (heads && nr_heads)
> > > nr_heads = remove_duplicates(nr_heads, heads);
> >
> > Hrm, could heads and/or nr_heads be NULL/0 here when we try to run qsort()
> > in this codepath?
> [...]
> A sane qsort would see that its second parameter is 0 and never try to
> dereference the array. But I'm not sure all qsort implementations we
> will see are sane, so it's probably better to protect it by putting it
> inside the conditional block just below.
I eye-balled what you queued in pu, and I wonder if you mis-read my
"just below" as "just below the existing line in the conditional" and
not "in the conditional that is just below the code we are talking
about".
I think we need this on top (or squashed in, but it looks like it is
already in next):
-- >8 --
Subject: fetch-pack: sort incoming heads list earlier
Commit 4435968 started sorting heads fed to fetch-pack so
that later commits could use more optimized algorithms;
commit 7db8d53 switched the remove_duplicates function to
such an algorithm.
Of course, the sorting is more effective if you do it
_before_ the algorithm in question.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
I suspect that all parts of git feed the refs in sorted order already,
which is why there were no test failures. But we should handle arbitrary
order from the command-line.
builtin/fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 8a72473..b18ba05 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1082,8 +1082,8 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
}
if (heads && nr_heads) {
- nr_heads = remove_duplicates(nr_heads, heads);
qsort(heads, nr_heads, sizeof(*heads), compare_heads);
+ nr_heads = remove_duplicates(nr_heads, heads);
}
if (!ref) {
--
1.7.10.1.25.g7031a0f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html