has_uncommitted_changes(): fall back to empty tree

peff · gitster · commit 3506dc944558 · 2018-07-11T12:12:37.000-07:00
If has_uncommitted_changes() can't resolve HEAD (e.g.,
because it's unborn or corrupt), then we end up calling
run_diff_index() with an empty revs.pending array. This
causes a segfault, as run_diff_index() blindly looks at the
first pending item.

Fixing this raises a question of fault: should
run_diff_index() handle this case, or is the caller wrong to
pass an empty pending list?

Looking at the other callers of run_diff_index(), they
handle this in one of three ways:

 - they resolve the object themselves, and avoid doing the
   diff if it's not valid

 - they resolve the object themselves, and fall back to the
   empty tree

 - they use setup_revisions(), which will die() if the
   object isn't valid

Since this is the only broken caller, that argues that the
fix should go there. Falling back to the empty tree makes
sense here, as we'd claim uncommitted changes if and only if
the index is non-empty. This may be a little funny in the
case of corruption (the corrupt HEAD probably _isn't_
empty), but:

  - we don't actually know the reason here that HEAD didn't
    resolve (the much more likely case is that we have an
    unborn HEAD, in which case the empty tree comparison is
    the right thing)

  - this matches how other code, like "git diff", behaves

While we're thinking about it, let's add an assertion to
run_diff_index(). It should always be passed a single
object, and as this bug shows, it's easy to get it wrong
(and an assertion is easier to hunt down than a segfault, or
a quietly ignored extra tree).

Reported-by: Ævar Arnfjörð Bjarmason &lt;avarab@gmail.com&gt;
Signed-off-by: Jeff King &lt;peff@peff.net&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/diff-lib.c b/diff-lib.c
@@ -513,6 +513,9 @@ int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
 
+	if (revs->pending.nr != 1)
+		BUG("run_diff_index must be passed exactly one tree");
+
 	ent = revs->pending.objects;
 	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
 		exit(128);
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
@@ -618,6 +618,18 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
 	)
 '
 
+test_expect_success 'pull --rebase fails on corrupt HEAD' '
+	test_when_finished "rm -rf corrupt" &&
+	git init corrupt &&
+	(
+		cd corrupt &&
+		test_commit one &&
+		obj=$(git rev-parse --verify HEAD | sed "s#^..#&/#") &&
+		rm -f .git/objects/$obj &&
+		test_must_fail git pull --rebase
+	)
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
 	mkdir src &&
 	(cd src &&
diff --git a/wt-status.c b/wt-status.c
@@ -2315,7 +2315,17 @@ int has_uncommitted_changes(int ignore_submodules)
 	if (ignore_submodules)
 		rev_info.diffopt.flags.ignore_submodules = 1;
 	rev_info.diffopt.flags.quick = 1;
+
 	add_head_to_pending(&rev_info);
+	if (!rev_info.pending.nr) {
+		/*
+		 * We have no head (or it's corrupt); use the empty tree,
+		 * which will complain if the index is non-empty.
+		 */
+		struct tree *tree = lookup_tree(the_hash_algo->empty_tree);
+		add_pending_object(&rev_info, &tree->object, "");
+	}
+
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_index(&rev_info, 1);
 	return diff_result_code(&rev_info.diffopt, result);

Original file line number	Diff line number	Diff line change
`@@ -513,6 +513,9 @@ int run_diff_index(struct rev_info *revs, int cached)`
`513`	`513`	`{`
`514`	`514`	`struct object_array_entry *ent;`
`515`	`515`
	`516`	`+ if (revs->pending.nr != 1)`
	`517`	`+ BUG("run_diff_index must be passed exactly one tree");`
	`518`	`+`
`516`	`519`	`ent = revs->pending.objects;`
`517`	`520`	`if (diff_cache(revs, &ent->item->oid, ent->name, cached))`
`518`	`521`	`exit(128);`