From a356e8e2a724012c8120bfa69133b6118b1565f4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Mar 2017 03:23:06 -0500 Subject: t3204: test git-branch @-expansion corner cases git-branch feeds the branch names from the command line to strbuf_branchname(), but we do not yet tell that function which kinds of expansions should be allowed. Let's create a set of tests that cover both the allowed and disallowed cases. That shows off some breakages where we currently create or delete the wrong ref (and will make sure that we do not break any cases that _should_ be working when we do add more restrictions). Note that we check branch creation and deletion, but do not bother with renames. Those follow the same code path as creation. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3204-branch-name-interpretation.sh | 123 ++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100755 t/t3204-branch-name-interpretation.sh (limited to 't/t3204-branch-name-interpretation.sh') diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh new file mode 100755 index 0000000000..e671a7a644 --- /dev/null +++ b/t/t3204-branch-name-interpretation.sh @@ -0,0 +1,123 @@ +#!/bin/sh + +test_description='interpreting exotic branch name arguments + +Branch name arguments are usually names which are taken to be inside of +refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc. +This script aims to check the behavior of those corner cases. +' +. ./test-lib.sh + +expect_branch() { + git log -1 --format=%s "$1" >actual && + echo "$2" >expect && + test_cmp expect actual +} + +expect_deleted() { + test_must_fail git rev-parse --verify "$1" +} + +test_expect_success 'set up repo' ' + test_commit one && + test_commit two && + git remote add origin foo.git +' + +test_expect_success 'update branch via @{-1}' ' + git branch previous one && + + git checkout previous && + git checkout master && + + git branch -f @{-1} two && + expect_branch previous two +' + +test_expect_success 'update branch via local @{upstream}' ' + git branch local one && + git branch --set-upstream-to=local && + + git branch -f @{upstream} two && + expect_branch local two +' + +test_expect_failure 'disallow updating branch via remote @{upstream}' ' + git update-ref refs/remotes/origin/remote one && + git branch --set-upstream-to=origin/remote && + + test_must_fail git branch -f @{upstream} two +' + +test_expect_success 'create branch with pseudo-qualified name' ' + git branch refs/heads/qualified two && + expect_branch refs/heads/refs/heads/qualified two +' + +test_expect_success 'delete branch via @{-1}' ' + git branch previous-del && + + git checkout previous-del && + git checkout master && + + git branch -D @{-1} && + expect_deleted previous-del +' + +test_expect_success 'delete branch via local @{upstream}' ' + git branch local-del && + git branch --set-upstream-to=local-del && + + git branch -D @{upstream} && + expect_deleted local-del +' + +test_expect_success 'delete branch via remote @{upstream}' ' + git update-ref refs/remotes/origin/remote-del two && + git branch --set-upstream-to=origin/remote-del && + + git branch -r -D @{upstream} && + expect_deleted origin/remote-del +' + +# Note that we create two oddly named local branches here. We want to make +# sure that we do not accidentally delete either of them, even if +# shorten_unambiguous_ref() tweaks the name to avoid ambiguity. +test_expect_failure 'delete @{upstream} expansion matches -r option' ' + git update-ref refs/remotes/origin/remote-del two && + git branch --set-upstream-to=origin/remote-del && + git update-ref refs/heads/origin/remote-del two && + git update-ref refs/heads/remotes/origin/remote-del two && + + test_must_fail git branch -D @{upstream} && + expect_branch refs/heads/origin/remote-del two && + expect_branch refs/heads/remotes/origin/remote-del two +' + +test_expect_failure 'disallow deleting remote branch via @{-1}' ' + git update-ref refs/remotes/origin/previous one && + + git checkout -b origin/previous two && + git checkout master && + + test_must_fail git branch -r -D @{-1} && + expect_branch refs/remotes/origin/previous one && + expect_branch refs/heads/origin/previous two +' + +# The thing we are testing here is that "@" is the real branch refs/heads/@, +# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a +# sane thing, but it _is_ technically allowed for now. If we disallow it, these +# can be switched to test_must_fail. +test_expect_failure 'create branch named "@"' ' + git branch -f @ one && + expect_branch refs/heads/@ one +' + +test_expect_failure 'delete branch named "@"' ' + git update-ref refs/heads/@ two && + git branch -D @ && + expect_deleted refs/heads/@ +' + +test_done -- cgit v1.2.3 From 6b145e016aaf512d0026cbd2c78fa28476f043b4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Mar 2017 03:23:10 -0500 Subject: branch: restrict @-expansions when deleting We use strbuf_branchname() to expand the branch name from the command line, so you can delete the branch given by @{-1}, for example. However, we allow other nonsense like "@", and we do not respect our "-r" flag (so we may end up deleting an oddly-named local ref instead of a remote one). We can fix this by passing the appropriate "allowed" flag to strbuf_branchname(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3204-branch-name-interpretation.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 't/t3204-branch-name-interpretation.sh') diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index e671a7a644..4f4af1fb46 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -83,7 +83,7 @@ test_expect_success 'delete branch via remote @{upstream}' ' # Note that we create two oddly named local branches here. We want to make # sure that we do not accidentally delete either of them, even if # shorten_unambiguous_ref() tweaks the name to avoid ambiguity. -test_expect_failure 'delete @{upstream} expansion matches -r option' ' +test_expect_success 'delete @{upstream} expansion matches -r option' ' git update-ref refs/remotes/origin/remote-del two && git branch --set-upstream-to=origin/remote-del && git update-ref refs/heads/origin/remote-del two && @@ -94,7 +94,7 @@ test_expect_failure 'delete @{upstream} expansion matches -r option' ' expect_branch refs/heads/remotes/origin/remote-del two ' -test_expect_failure 'disallow deleting remote branch via @{-1}' ' +test_expect_success 'disallow deleting remote branch via @{-1}' ' git update-ref refs/remotes/origin/previous one && git checkout -b origin/previous two && @@ -114,7 +114,7 @@ test_expect_failure 'create branch named "@"' ' expect_branch refs/heads/@ one ' -test_expect_failure 'delete branch named "@"' ' +test_expect_success 'delete branch named "@"' ' git update-ref refs/heads/@ two && git branch -D @ && expect_deleted refs/heads/@ -- cgit v1.2.3 From 7d5c960bf6f98601dce992f1ffaf4c7ab932e6dc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Mar 2017 03:23:14 -0500 Subject: strbuf_check_ref_format(): expand only local branches This function asks strbuf_branchname() to expand any @-marks in the branchname, and then we blindly stick refs/heads/ in front of the result. This is obviously nonsense if the expansion is "HEAD" or a ref in refs/remotes/. The most obvious end-user effect is that creating or renaming a branch with an expansion may have confusing results (e.g., creating refs/heads/origin/master from "@{upstream}" when the operation should be disallowed). We can fix this by telling strbuf_branchname() that we are only interested in local expansions. Any unexpanded bits are then fed to check_ref_format(), which either disallows them (in the case of "@{upstream}") or lets them through ("refs/heads/@" is technically valid, if a bit silly). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3204-branch-name-interpretation.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 't/t3204-branch-name-interpretation.sh') diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index 4f4af1fb46..05e88f92d9 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -42,7 +42,7 @@ test_expect_success 'update branch via local @{upstream}' ' expect_branch local two ' -test_expect_failure 'disallow updating branch via remote @{upstream}' ' +test_expect_success 'disallow updating branch via remote @{upstream}' ' git update-ref refs/remotes/origin/remote one && git branch --set-upstream-to=origin/remote && @@ -109,7 +109,7 @@ test_expect_success 'disallow deleting remote branch via @{-1}' ' # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a # sane thing, but it _is_ technically allowed for now. If we disallow it, these # can be switched to test_must_fail. -test_expect_failure 'create branch named "@"' ' +test_expect_success 'create branch named "@"' ' git branch -f @ one && expect_branch refs/heads/@ one ' -- cgit v1.2.3 From fd4692ff7040e690546ed139a419995e76a96196 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Mar 2017 03:23:18 -0500 Subject: checkout: restrict @-expansions when finding branch When we parse "git checkout $NAME", we try to interpret $NAME as a local branch-name. If it is, then we point HEAD to that branch. Otherwise, we detach the HEAD at whatever commit $NAME points to. We do the interpretation by calling strbuf_branchname(), and then blindly sticking "refs/heads/" on the front. This leads to nonsense results when expansions like "@{upstream}" or "@" point to something besides a local branch. We end up with a local branch name like "refs/heads/origin/master" or "refs/heads/HEAD". Normally this has no user-visible effect because those branches don't exist, and so we fallback to feeding the result to get_sha1(), which resolves them correctly. But as the new test in t3204 shows, there are corner cases where the effect is observable, and we check out the wrong local branch rather than detaching to the correct one. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t3204-branch-name-interpretation.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 't/t3204-branch-name-interpretation.sh') diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh index 05e88f92d9..698d9cc4f3 100755 --- a/t/t3204-branch-name-interpretation.sh +++ b/t/t3204-branch-name-interpretation.sh @@ -120,4 +120,14 @@ test_expect_success 'delete branch named "@"' ' expect_deleted refs/heads/@ ' +test_expect_success 'checkout does not treat remote @{upstream} as a branch' ' + git update-ref refs/remotes/origin/checkout one && + git branch --set-upstream-to=origin/checkout && + git update-ref refs/heads/origin/checkout two && + git update-ref refs/heads/remotes/origin/checkout two && + + git checkout @{upstream} && + expect_branch HEAD one +' + test_done -- cgit v1.2.3