git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 3/4] merge: merge unborn index before setting ref

From
Jeff King <peff@peff.net>
Date
Mar 25, 2011, 18:10 UTC
Message-ID
<20110325181038.GC14898@sigill.intra.peff.net>
In-Reply-To
<20110325180644.GA30838@sigill.intra.peff.net>

When we merge into an unborn branch, there are basically two steps:

  1. Write the sha1 of the new commit into the ref pointed
     to by HEAD.
  2. Update the index with the new content, and check it out
     to the working tree.

We currently do them in this order. However, (2) is the step that is much more likely to fail, since it can be blocked by things like untracked working tree files. When it does, the merge fails and we are left with an empty index but an updated HEAD.

This patch switches the order, so that a failure in updating the index leaves us unchanged. Of course, a failure in updating the ref now leaves us with an updated index and mis-matched HEAD. That is arguably not much better, but it is probably less likely to actually happen.

Signed-off-by: Jeff King <peff@peff.net>
---
I noticed this while diagnosing the pull problem fixed in 4/4. As
discused, this is just trading one set of error conditions for another.
The "right" thing to do is probably to rollback, but of course that can
fail, too, and it's more effort. I think in practice this is fine.
 builtin/merge.c            |    2 +-
 t/t7607-merge-overwrite.sh |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index aa3453c..c8d028c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1063,9 +1063,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		remote_head = peel_to_type(argv[0], 0, NULL, OBJ_COMMIT);
 		if (!remote_head)
 			die("%s - not something we can merge", argv[0]);
+		read_empty(remote_head->sha1, 0);
 		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
 				DIE_ON_ERR);
-		read_empty(remote_head->sha1, 0);
 		return 0;
 	} else {
 		struct strbuf merge_names = STRBUF_INIT;
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index c86e298..b54e840 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -157,6 +157,10 @@ test_expect_success 'will not overwrite untracked file on unborn branch' '
 	test_cmp important c0.c
 '
 
+test_expect_success 'failed merge leaves unborn branch in the womb' '
+	test_must_fail git rev-parse --verify HEAD
+'
+
 test_expect_success 'set up unborn branch and content' '
 	git symbolic-ref HEAD refs/heads/unborn &&
 	rm -f .git/index &&
-- 
1.7.4.33.gb8855.dirty
Previous: Jeff KingNext: Jeff King
Message 7 of 8 in “Why git silently replaces untracked files?”
  1. igor.mikushkinMar 25, 2011
  2. Jeff KingMar 25, 2011
  3. igor.mikushkinMar 25, 2011
  4. Jeff KingMar 25, 2011
  5. 1/4 t7607: mark known breakage in test 11 as fixedJeff King, Mar 25, 2011
  6. 2/4 t7607: clean up stray untracked fileJeff King, Mar 25, 2011
  7. 3/4 merge: merge unborn index before setting refJeff King, Mar 25, 2011
  8. 4/4 pull: do not clobber untracked files on initial pullJeff King, Mar 25, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.