Re: [PATCH 1/5] fetch-pack: sort incoming heads
- From
Jeff King <peff@peff.net>
- Date
- May 24, 2012, 06:04 UTC
- Message-ID
- <20120524060451.GA13502@sigill.intra.peff.net>
- In-Reply-To
- <20120522202336.GA31231@sigill.intra.peff.net>
On Tue, May 22, 2012 at 04:23:36PM -0400, Jeff King wrote:
Show 18 quoted lines
> 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@peff.net> --- 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