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

From: Jeff King <peff@peff.net>
Date: Apr 3, 2012, 09:19 UTC
Message-ID: <20120403091937.GB14483@sigill.intra.peff.net>
In-Reply-To: <20120403084035.GA14483@sigill.intra.peff.net>

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

Show 6 quoted lines

> > 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;
 }