Re: [PATCH 3/3] revision: insert unsorted, then sort in prepare_revision_walk()

[Jeff King <peff@peff.net>]

On Tue, Apr 03, 2012 at 04:40:36AM -0400, Jeff King wrote:

> > It looks nice and to the point, but breaks several tests for me
> > (t3508, t4013, t4041, t4202, t6003, t6009, t6016, t6018 and t7401).
> > Not sure why.
> 
> I probably screwed up the reversal or something. My patch was a quick "I
> was thinking of this direction" and I didn't actually test it well.

Ugh. Not that it matters now, but the patch below fixes my version.

Not only did I manage to screw up the reversal, but I also messed up the
calling convention for qsort. So the moral is that we should take 100
lines of your tested code over 5 lines of my junk. ;)

As a fun fact, I tried fixing the reversal by sorting low to high, and
then just doing the commit_list_insert calls in that order (since it
prepends). However, that loses the stability of the sort. It turns out
that t4207 fails in this case (though not reliably, since your commits
might or might not be made in the same second).

I had already checked your mergesort() implementation to be sure that it
is stable (and it is). But it's nice to know that t4207 also backs up
the analysis. :)

-Peff

---
diff --git a/revision.c b/revision.c
index 22c26d0..15bf30a 100644
--- a/revision.c
+++ b/revision.c
@@ -2064,11 +2064,11 @@ static void set_children(struct rev_info *revs)
 
 static int commit_compare_by_date(const void *va, const void *vb)
 {
-	const struct commit *a = va;
-	const struct commit *b = vb;
-	if (a->date < b->date)
+	const struct commit *a = *(const struct commit **)va;
+	const struct commit *b = *(const struct commit **)vb;
+	if (a->date > b->date)
 		return -1;
-	if (b->date < a->date)
+	if (b->date > a->date)
 		return 1;
 	return 0;
 }