Subject: [PATCH v3] reflog-walk: don't segfault on non-commit sha1's in the reflog

From: Dennis Kaarsemaker <[email-masked]>
Date: Thu, 31 Dec 2015 00:33:03 +0100

git reflog (ab)uses the log machinery to display its list of log
entries. To do so it must fake commit parent information for the log
walker.

For refs in refs/heads this is no problem, as they should only ever
point to commits. Tags and other refs however can point to anything,
thus their reflog may contain non-commit objects.

To avoid segfaulting, we check whether reflog entries are commits before
feeding them to the log walker and skip any non-commits. This means that
git reflog output will be incomplete for such refs, but that's one step
up from segfaulting. A more complete solution would be to decouple git
reflog from the log walker machinery.

Signed-off-by: Dennis Kaarsemaker <[email-masked]>
Helped-by: Nguyễn Thái Ngọc Duy <[email-masked]>
Helped-by: Junio C Hamano <[email-masked]>
---
 reflog-walk.c     | 16 +++++++++++-----
 t/t1410-reflog.sh | 13 +++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

Junio C Hamano wrote:

> However, I see that there are one of two things that you could do to
> make this part of code do slightly better than stopping at the first
> non-commit object:
> 
>  - pretend that the non-commit entry never existed in the first
>     place and return the commit that appears in the reflog next.

This turned out to be doable in the same code segment: just keep on
processing reflog entries until you hit a commit or run out of entries.
That (and the updated foremerly-failing test) are the only changes
between v2 and v3.

I'll try to actually implement the proper solution, but that'll take a
while. Until then, this at least stops a segfault :)

diff --git a/reflog-walk.c b/reflog-walk.c
index 85b8a54..0ebd1da 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 	struct commit_info *commit_info =
 		get_commit_info(commit, &info->reflogs, 0);
 	struct commit_reflog *commit_reflog;
+	struct object *logobj;
 	struct reflog_info *reflog;
 
 	info->last_commit_reflog = NULL;
@@ -232,15 +233,20 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 		commit->parents = NULL;
 		return;
 	}
-
-	reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
 	info->last_commit_reflog = commit_reflog;
-	commit_reflog->recno--;
-	commit_info->commit = (struct commit *)parse_object(reflog->osha1);
-	if (!commit_info->commit) {
+
+	do {
+		reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
+		commit_reflog->recno--;
+		logobj = parse_object(reflog->osha1);
+	} while (commit_reflog->recno && (logobj && logobj->type != OBJ_COMMIT));
+
+	if (!logobj || logobj->type != OBJ_COMMIT) {
+		commit_info->commit = NULL;
 		commit->parents = NULL;
 		return;
 	}
+	commit_info->commit = (struct commit *)logobj;
 
 	commit->parents = xcalloc(1, sizeof(struct commit_list));
 	commit->parents->item = commit_info->commit;
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index b79049f..f97513c 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ boundaries' '
 	test_cmp expect actual
 '
 
+test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
+	git update-ref --create-reflog -m "Creating ref" \
+		refs/tests/tree-in-reflog HEAD &&
+	git update-ref -m "Forcing tree" refs/tests/tree-in-reflog HEAD^{tree} &&
+	git update-ref -m "Restoring to commit" refs/tests/tree-in-reflog HEAD &&
+	git reflog refs/tests/tree-in-reflog
+'
+
+test_expect_success 'reflog containing non-commit sha1s displays properly' '
+	git reflog refs/tests/tree-in-reflog >actual &&
+	test_line_count = 2 actual
+'
+
 test_done
-- 
2.7.0-rc1-207-ga35084c

-- 
Dennis Kaarsemaker <[email-masked]>
http://twitter.com/seveas