{"thread":{"id":"33831","subject":"[PATCH] strbuf_branchname(): do not double-expand @{-1}~22","startedAt":"2013-05-16T00:29:51Z","lastAt":"2013-05-16T03:20:06Z","messageCount":2,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"217490","messageId":"7vfvxnahs0.fsf@alter.siamese.dyndns.org","threadId":"33831","inReplyTo":null,"subject":"[PATCH] strbuf_branchname(): do not double-expand @{-1}~22","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-16T00:29:51Z","receivedAt":"2013-05-16T00:29:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you were on 'frotz' branch before you checked out your current\nbranch, \"git merge @{-1}~22\" means the same as \"git merge frotz~22\".\n\nThe strbuf_branchname() function, when interpret_branch_name() gives\nup resolving \"@{-1}~22\" fully, returns \"frotz\" and tells the caller\nthat it only resolved \"@{-1}\" part of the input, mistakes this as a\ntotal failure, and appends the whole thing to the result, yielding\n\"frotz@{-1}~22\", which does not make any sense.\n\nInspect the return valud from interpret_branch_name() a bit more\ncarefully.  When it errored out without consuming anything, we will\nget -1 and we should return the whole thing.  Otherwise, we should\nappend the remainder (i.e. \"~22\" in the earlier example) to the\npartially resolved name (i.e. \"frotz\").\n\nThe test suite adds enough number of checkout to make @{-12} in the\nlast test in t0100 that tried to check \"we haven't flipped branches\nthat many times\" error case; raise the number to a hundred.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The original code in a552de75eb01 (strbuf_branchname(): a wrapper\n   for branch name shorthands, 2009-03-21) did not have this problem\n   only because interpret_branch_name() did not return a partial\n   success, but in today's code after d46a8301930a (fix parsing of\n   @{-1}@{u} combination, 2010-01-28), it should pay attention to\n   the condition.\n\n   There might be other callers of interpret_branch_name() that\n   still assume there is no partial success; I didn't check.\n\n sha1_name.c         |  8 ++++++--\n t/t0100-previous.sh | 15 +++++++++++++--\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..371a49d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1055,9 +1055,13 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n int strbuf_branchname(struct strbuf *sb, const char *name)\n {\n \tint len = strlen(name);\n-\tif (interpret_branch_name(name, sb) == len)\n+\tint used = interpret_branch_name(name, sb);\n+\n+\tif (used == len)\n \t\treturn 0;\n-\tstrbuf_add(sb, name, len);\n+\tif (used < 0)\n+\t\tused = 0;\n+\tstrbuf_add(sb, name + used, len - used);\n \treturn len;\n }\n \ndiff --git a/t/t0100-previous.sh b/t/t0100-previous.sh\nindex 315b9b3..e0a6940 100755\n--- a/t/t0100-previous.sh\n+++ b/t/t0100-previous.sh\n@@ -27,6 +27,7 @@ test_expect_success 'merge @{-1}' '\n \ttest_commit B &&\n \tgit checkout A &&\n \ttest_commit C &&\n+\ttest_commit D &&\n \tgit branch -f master B &&\n \tgit branch -f other &&\n \tgit checkout other &&\n@@ -35,14 +36,24 @@ test_expect_success 'merge @{-1}' '\n \tgit cat-file commit HEAD | grep \"Merge branch '\\''other'\\''\"\n '\n \n-test_expect_success 'merge @{-1} when there is not enough switches yet' '\n+test_expect_success 'merge @{-1}~1' '\n+\tgit checkout master &&\n+\tgit reset --hard B &&\n+\tgit checkout other &&\n+\tgit checkout master &&\n+\tgit merge @{-1}~1 &&\n+\tgit cat-file commit HEAD >actual &&\n+\tgrep \"Merge branch '\\''other'\\''\" actual\n+'\n+\n+test_expect_success 'merge @{-100} before checking out that many branches yet' '\n \tgit reflog expire --expire=now &&\n \tgit checkout -f master &&\n \tgit reset --hard B &&\n \tgit branch -f other C &&\n \tgit checkout other &&\n \tgit checkout master &&\n-\ttest_must_fail git merge @{-12}\n+\ttest_must_fail git merge @{-100}\n '\n \n test_done\n-- \n1.8.3-rc2-210-gbc3cf50\n"},{"id":"217494","messageId":"20130516032006.GA13296@sigill.intra.peff.net","threadId":"33831","inReplyTo":"7vfvxnahs0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] strbuf_branchname(): do not double-expand @{-1}~22","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-16T03:20:06Z","receivedAt":"2013-05-16T03:20:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 15, 2013 at 05:29:51PM -0700, Junio C Hamano wrote:\n\n> If you were on 'frotz' branch before you checked out your current\n> branch, \"git merge @{-1}~22\" means the same as \"git merge frotz~22\".\n> \n> The strbuf_branchname() function, when interpret_branch_name() gives\n> up resolving \"@{-1}~22\" fully, returns \"frotz\" and tells the caller\n> that it only resolved \"@{-1}\" part of the input, mistakes this as a\n> total failure, and appends the whole thing to the result, yielding\n> \"frotz@{-1}~22\", which does not make any sense.\n> \n> Inspect the return valud from interpret_branch_name() a bit more\n> carefully.  When it errored out without consuming anything, we will\n> get -1 and we should return the whole thing.  Otherwise, we should\n> append the remainder (i.e. \"~22\" in the earlier example) to the\n> partially resolved name (i.e. \"frotz\").\n\nThanks, I think your patch looks like the right solution.\n\nAlso, s/valud/value/ in the commit message.\n\n>  * The original code in a552de75eb01 (strbuf_branchname(): a wrapper\n>    for branch name shorthands, 2009-03-21) did not have this problem\n>    only because interpret_branch_name() did not return a partial\n>    success, but in today's code after d46a8301930a (fix parsing of\n>    @{-1}@{u} combination, 2010-01-28), it should pay attention to\n>    the condition.\n\nA quick grep shows substitute_branch_name does not distinguish these\ncases, either, but I think that is OK. It is used by dwim_ref and\ndwim_log to convert a string into a refname, and a partial parse of\nsomething like \"@{u}~22\" should be a failure (it does not return a ref,\nbut rather a commit).\n\nIt does look like substitute_branch_name may leak \"buf\" in such a case,\nthough.\n\n-Peff\n"}]}