threads / patch / 33831

patchstrbuf_branchname(): do not double-expand @{-1}~22

Subject: [PATCH] strbuf_branchname(): do not double-expand @{-1}~22

## tl;dr

2 messages between May 16, 2013 and May 16, 2013. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Junio C Hamano· May 16, 2013, 00:29 UTC · lore

If you were on 'frotz' branch before you checked out your current branch, "git merge @{-1}~22" means the same as "git merge frotz~22".

The strbuf_branchname() function, when interpret_branch_name() gives up resolving "@{-1}~22" fully, returns "frotz" and tells the caller that it only resolved "@{-1}" part of the input, mistakes this as a total failure, and appends the whole thing to the result, yielding "frotz@{-1}~22", which does not make any sense.

Inspect the return valud from interpret_branch_name() a bit more carefully. When it errored out without consuming anything, we will get -1 and we should return the whole thing. Otherwise, we should append the remainder (i.e. "~22" in the earlier example) to the partially resolved name (i.e. "frotz").

The test suite adds enough number of checkout to make @{-12} in the last test in t0100 that tried to check "we haven't flipped branches that many times" error case; raise the number to a hundred.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The original code in a552de75eb01 (strbuf_branchname(): a wrapper
   for branch name shorthands, 2009-03-21) did not have this problem
   only because interpret_branch_name() did not return a partial
   success, but in today's code after d46a8301930a (fix parsing of
   @{-1}@{u} combination, 2010-01-28), it should pay attention to
   the condition.
   There might be other callers of interpret_branch_name() that
   still assume there is no partial success; I didn't check.
 sha1_name.c         |  8 ++++++--
 t/t0100-previous.sh | 15 +++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)
Show changes to 2 files +19 −4

sha1_name.c, t/t0100-previous.sh

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..371a49d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1055,9 +1055,13 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 int strbuf_branchname(struct strbuf *sb, const char *name)
 {
 	int len = strlen(name);
-	if (interpret_branch_name(name, sb) == len)
+	int used = interpret_branch_name(name, sb);
+
+	if (used == len)
 		return 0;
-	strbuf_add(sb, name, len);
+	if (used < 0)
+		used = 0;
+	strbuf_add(sb, name + used, len - used);
 	return len;
 }
 
diff --git a/t/t0100-previous.sh b/t/t0100-previous.sh
index 315b9b3..e0a6940 100755
--- a/t/t0100-previous.sh
+++ b/t/t0100-previous.sh
@@ -27,6 +27,7 @@ test_expect_success 'merge @{-1}' '
 	test_commit B &&
 	git checkout A &&
 	test_commit C &&
+	test_commit D &&
 	git branch -f master B &&
 	git branch -f other &&
 	git checkout other &&
@@ -35,14 +36,24 @@ test_expect_success 'merge @{-1}' '
 	git cat-file commit HEAD | grep "Merge branch '\''other'\''"
 '
 
-test_expect_success 'merge @{-1} when there is not enough switches yet' '
+test_expect_success 'merge @{-1}~1' '
+	git checkout master &&
+	git reset --hard B &&
+	git checkout other &&
+	git checkout master &&
+	git merge @{-1}~1 &&
+	git cat-file commit HEAD >actual &&
+	grep "Merge branch '\''other'\''" actual
+'
+
+test_expect_success 'merge @{-100} before checking out that many branches yet' '
 	git reflog expire --expire=now &&
 	git checkout -f master &&
 	git reset --hard B &&
 	git branch -f other C &&
 	git checkout other &&
 	git checkout master &&
-	test_must_fail git merge @{-12}
+	test_must_fail git merge @{-100}
 '
 
 test_done
-- 
1.8.3-rc2-210-gbc3cf50
Jeff King· May 16, 2013, 03:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] strbuf_branchname(): do not double-expand @{-1}~22

On Wed, May 15, 2013 at 05:29:51PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> If you were on 'frotz' branch before you checked out your current
> branch, "git merge @{-1}~22" means the same as "git merge frotz~22".
> 
> The strbuf_branchname() function, when interpret_branch_name() gives
> up resolving "@{-1}~22" fully, returns "frotz" and tells the caller
> that it only resolved "@{-1}" part of the input, mistakes this as a
> total failure, and appends the whole thing to the result, yielding
> "frotz@{-1}~22", which does not make any sense.
> 
> Inspect the return valud from interpret_branch_name() a bit more
> carefully.  When it errored out without consuming anything, we will
> get -1 and we should return the whole thing.  Otherwise, we should
> append the remainder (i.e. "~22" in the earlier example) to the
> partially resolved name (i.e. "frotz").
Thanks, I think your patch looks like the right solution.
Also, s/valud/value/ in the commit message.
Show 6 quoted lines
>  * The original code in a552de75eb01 (strbuf_branchname(): a wrapper
>    for branch name shorthands, 2009-03-21) did not have this problem
>    only because interpret_branch_name() did not return a partial
>    success, but in today's code after d46a8301930a (fix parsing of
>    @{-1}@{u} combination, 2010-01-28), it should pay attention to
>    the condition.

A quick grep shows substitute_branch_name does not distinguish these cases, either, but I think that is OK. It is used by dwim_ref and dwim_log to convert a string into a refname, and a partial parse of something like "@{u}~22" should be a failure (it does not return a ref, but rather a commit).

It does look like substitute_branch_name may leak "buf" in such a case, though.

-Peff

← back to recent threads