Re: [PATCH 1/5] fetch-pack: sort incoming heads

[Jeff King <peff@peff.net>]

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@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