threads / patch / 14753

patchFix merge name generation in "merge in C"

Subject: [PATCH] Fix merge name generation in "merge in C"

## tl;dr

4 messages between Jul 30, 2008 and Oct 13, 2008. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Junio C Hamano· Jul 30, 2008, 08:12 UTC · lore

When merging an early part of a branch, e.g. "git merge xyzzy~20", we were supposed to say "branch 'xyzzy' (early part)", but it incorrectly said "branch 'refs/heads/xy' (early part)" instead.

The logic was supposed to first strip away "~20" part to make sure that what follows "~" is a non-zero posint, prefix it with "refs/heads/" and ask resolve_ref() if it is a ref. If it is, then we know xyzzy was a branch, and we can give the correct message.

However, there were a few bugs. First of all, the logic to build this "true branch refname" did not count the characters correctly. At this point of the code, "len" is the number of trailing, non-name part of the given extended SHA-1 expression given by the user, i.e. number of bytes in "~20" in the above example.

In addition, the message forgot to skip "refs/heads/" it prefixed from the output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * It is a bit surprising that after beating merge-in-C to death, we
   still find a minor breakage like this.
 builtin-merge.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin-merge.c +2 −2
diff --git a/builtin-merge.c b/builtin-merge.c
index e78fa18..dde0c7e 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -396,12 +396,12 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		struct strbuf truname = STRBUF_INIT;
 		strbuf_addstr(&truname, "refs/heads/");
 		strbuf_addstr(&truname, remote);
-		strbuf_setlen(&truname, len+11);
+		strbuf_setlen(&truname, truname.len - len);
 		if (resolve_ref(truname.buf, buf_sha, 0, 0)) {
 			strbuf_addf(msg,
 				    "%s\t\tbranch '%s'%s of .\n",
 				    sha1_to_hex(remote_head->sha1),
-				    truname.buf,
+				    truname.buf + 11,
 				    (early ? " (early part)" : ""));
 			return;
 		}
Miklos Vajna· Jul 30, 2008, 17:23 UTC · re: Junio C Hamano · lore

[PATCH] Add testcase to ensure merging an early part of a branch is done properly

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Wed, Jul 30, 2008 at 01:12:19AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> In addition, the message forgot to skip "refs/heads/" it prefixed from
> the
> output.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * It is a bit surprising that after beating merge-in-C to death, we
>    still find a minor breakage like this.

Uh-oh. Here is a testcase that fails with master, but passes with your patch.

Thanks for catching this.
 t/t7607-merge-early.sh |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)
 create mode 100755 t/t7607-merge-early.sh
Show changes to t/t7607-merge-early.sh +39 −0
diff --git a/t/t7607-merge-early.sh b/t/t7607-merge-early.sh
new file mode 100755
index 0000000..9dd3ac5
--- /dev/null
+++ b/t/t7607-merge-early.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing merging an early part of a branch.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2 &&
+	git reset --hard c0 &&
+	echo c3 > c3.c &&
+	git add c3.c &&
+	git commit -m c3 &&
+	git tag c3
+'
+
+cat >expected <<EOF
+Merge branch 'c2' (early part)
+EOF
+
+test_expect_success 'merge early part of c2' '
+	git merge c2~1 &&
+	git show -s --pretty=format:%s HEAD > actual &&
+	test_cmp actual expected
+'
+
+test_done

-- 
1.6.0.rc0.14.g95f8.dirty
Miklos Vajna· Oct 13, 2008, 20:54 UTC · re: Miklos Vajna · lore

[PATCH] Add testcase to ensure merging an early part of a branch is done properly

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
[ Adding Shawn to CC as the interim maintainer. ]
On Wed, Jul 30, 2008 at 07:23:28PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> Uh-oh. Here is a testcase that fails with master, but passes with your
> patch.

I remember you had a note about it's a bad habit to write a new test for every bug, so here is an updated version that just improves t7600-merge.sh and does not adds a new one.

 t/t7600-merge.sh |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
Show changes to t/t7600-merge.sh +26 −0
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 7313ac2..a4443a7 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -520,4 +520,30 @@ test_expect_success 'refresh the index before merging' '
 
 test_debug 'gitk --all'
 
+cat >expected <<EOF
+Merge branch 'c5' (early part)
+EOF
+
+test_expect_success 'merge early part of c2' '
+	git reset --hard c3 &&
+	echo c4 > c4.c &&
+	git add c4.c &&
+	git commit -m c4 &&
+	git tag c4 &&
+	echo c5 > c5.c &&
+	git add c5.c &&
+	git commit -m c5 &&
+	git tag c5 &&
+	git reset --hard c3 &&
+	echo c6 > c6.c &&
+	git add c6.c &&
+	git commit -m c6 &&
+	git tag c6 &&
+	git merge c5~1 &&
+	git show -s --pretty=format:%s HEAD > actual &&
+	test_cmp actual expected
+'
+
+test_debug 'gitk --all'
+
 test_done

-- 
1.6.0.2
Shawn O. Pearce· Oct 13, 2008, 21:03 UTC · re: Miklos Vajna · lore

Re: [PATCH] Add testcase to ensure merging an early part of a branch is done properly

Miklos Vajna <vmiklos@frugalware.org> wrote:
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
> 
> [ Adding Shawn to CC as the interim maintainer. ]
In case you missed it, Junio has the pumpkin now.  (He's picked up
my topics, merged my tree, and has freed me from the time sink that
is known as maintainership.)
 
-- 
Shawn.

← back to recent threads