Sort refs by the date the ref actually carries

2 files changed, 66 insertions, 13 deletions

diff --git a/source/ui-refs.c b/source/ui-refs.c
index 932ba6d..84b929c 100644
--- a/source/ui-refs.c
+++ b/source/ui-refs.c
@@ -27,14 +27,6 @@ static int cmp_ref_name(const void *a, const void *b)
 	return strcmp(r1->refname, r2->refname);
 }
 
-static int cmp_branch_age(const void *a, const void *b)
-{
-	struct refinfo *r1 = *(struct refinfo **)a;
-	struct refinfo *r2 = *(struct refinfo **)b;
-
-	return cmp_age(r1->commit->committer_date, r2->commit->committer_date);
-}
-
 static int get_ref_age(struct refinfo *ref)
 {
 	if (!ref->object)
@@ -48,7 +40,11 @@ static int get_ref_age(struct refinfo *ref)
 	return 0;
 }
 
-static int cmp_tag_age(const void *a, const void *b)
+// tag and commit share a union in struct refinfo and only the member matching
+// the object type is ever filled, so the date has to be reached through
+// get_ref_age rather than by assuming a branch points at a commit. Reading the
+// wrong member ran off the end of the smaller struct.
+static int cmp_ref_age(const void *a, const void *b)
 {
 	struct refinfo *r1 = *(struct refinfo **)a;
 	struct refinfo *r2 = *(struct refinfo **)b;
@@ -203,7 +199,7 @@ void cgit_print_branches(int maxcount)
 	if (maxcount == 0 || maxcount > list.count)
 		maxcount = list.count;
 
-	qsort(list.refs, list.count, sizeof(*list.refs), cmp_branch_age);
+	qsort(list.refs, list.count, sizeof(*list.refs), cmp_ref_age);
 	if (ctx.repo->branch_sort == 0)
 		qsort(list.refs, maxcount, sizeof(*list.refs), cmp_ref_name);
 
@@ -227,7 +223,7 @@ void cgit_print_tags(int maxcount)
 			      cgit_refs_cb, &list);
 	if (list.count == 0)
 		return;
-	qsort(list.refs, list.count, sizeof(*list.refs), cmp_tag_age);
+	qsort(list.refs, list.count, sizeof(*list.refs), cmp_ref_age);
 	if (!maxcount)
 		maxcount = list.count;
 	else if (maxcount > list.count)
@@ -253,7 +249,7 @@ static void print_branches_page(int pagesize)
 	print_branch_header();
 	collect_branches(&list);
 
-	qsort(list.refs, list.count, sizeof(*list.refs), cmp_branch_age);
+	qsort(list.refs, list.count, sizeof(*list.refs), cmp_ref_age);
 	if (ctx.repo->branch_sort == 0)
 		qsort(list.refs, list.count, sizeof(*list.refs), cmp_ref_name);
 
@@ -284,7 +280,7 @@ static void print_tags_page(int pagesize)
 			      cgit_refs_cb, &list);
 	if (list.count == 0)
 		return;
-	qsort(list.refs, list.count, sizeof(*list.refs), cmp_tag_age);
+	qsort(list.refs, list.count, sizeof(*list.refs), cmp_ref_age);
 
 	if (pagesize <= 0 || pagesize > list.count)
 		pagesize = list.count;
diff --git a/tests/t0200-security.sh b/tests/t0200-security.sh
index df60174..9238262 100755
--- a/tests/t0200-security.sh
+++ b/tests/t0200-security.sh
@@ -172,6 +172,63 @@ test_expect_success 'a message-less commit renders without crashing' '
 	grep "no commit message" tmp
 '
 
+# --- A branch need not point at a commit ------------------------------------
+# struct refinfo keeps taginfo and commitinfo in a union and fills only the one
+# matching the object type. Sorting branches by reading the commit member read
+# past the end of the smaller taginfo, and read NULL for a tree, which crashed.
+# git update-ref refuses to write these, so the refs go in as loose files: a
+# repository is just files on disk and cgit reads whatever is there.
+test_expect_success 'set up a repo whose branches point at odd objects' '
+	mkrepo repos/oddref 2 &&
+	(
+		cd repos/oddref &&
+		git tag -a annotated -m note &&
+		git rev-parse annotated >.git/refs/heads/points-at-tag &&
+		git rev-parse HEAD^{tree} >.git/refs/heads/points-at-tree &&
+		git for-each-ref refs/heads/ >refs.out &&
+		grep -q "tree.refs/heads/points-at-tree" refs.out &&
+		grep -q "tag.refs/heads/points-at-tag" refs.out
+	) &&
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "branch-sort=age" &&
+		echo "repo.url=oddref" &&
+		echo "repo.path=$PWD/repos/oddref/.git"
+	} >oddrefrc
+'
+
+test_expect_success 'refs page sorts such branches without crashing' '
+	CGIT_CONFIG="$PWD/oddrefrc" QUERY_STRING="url=oddref/refs/" cgit >tmp &&
+	grep "points-at-tree" tmp &&
+	grep "</html>" tmp
+'
+
+test_expect_success 'the branch page sorts them without crashing' '
+	CGIT_CONFIG="$PWD/oddrefrc" QUERY_STRING="url=oddref/refs/heads/" cgit >tmp &&
+	grep "points-at-tree" tmp &&
+	grep "</html>" tmp
+'
+
+test_expect_success 'the summary page sorts them without crashing' '
+	CGIT_CONFIG="$PWD/oddrefrc" QUERY_STRING="url=oddref/" cgit >tmp &&
+	grep "points-at-tree" tmp &&
+	grep "</html>" tmp
+'
+
+test_expect_success 'name-sorted branches are unaffected' '
+	{
+		echo "virtual-root=/" &&
+		echo "cache-size=0" &&
+		echo "branch-sort=name" &&
+		echo "repo.url=oddref" &&
+		echo "repo.path=$PWD/repos/oddref/.git"
+	} >oddrefnamerc &&
+	CGIT_CONFIG="$PWD/oddrefnamerc" QUERY_STRING="url=oddref/refs/heads/" cgit >tmp &&
+	grep "points-at-tree" tmp &&
+	grep "</html>" tmp
+'
+
 # --- A repository cannot supply a printf format string ----------------------
 # module-link is a template, and scan-path lets a repository set it through its
 # own cgitrc. Handing it to printf let a repo owner crash the process, or read