Re: BUG when trying to delete symbolic refs

Am 15.10.2012 10:50, schrieb Johan Herland:

Basically, there is a "master" branch, and an "alias" symref to
"master". When we naively try to delete the symref with "git branch -d
alias", it ends up:

  - NOT deleting the "alias" symref
  - DELETING the "master" loose ref
  - NOT deleting the "master" packed ref

So, from the user perspective, "git branch -d alias" ends up resetting
"master" (and "alias") back to the last time we happened to run "git
gc". Needless to say, this is not quite what we had in mind...

AFAICS, there may be three possible "acceptable" outcomes when we run
"git branch -d alias" in the above scenario:

  A. The symbolic ref is deleted. This is obviously what we expected...

Below is a patch to do that.

  B. The command fails because "alias" is a symref. This would be
understandable if we don't want to teach "branch -d" about symrefs.
But then, the error message should ideally explain which command we
should use to remove the symref.

Renaming of symrefs with branch -m is disallowed because it's more
complicated than it looks at first; this was discussed here:
http://thread.gmane.org/gmane.comp.version-control.git/98825/focus=99206

I can't imagine why deletion should be prohibited as well, though.

  C. The "master" ref (BOTH loose and packed versions of it) is
deleted. This would be less helpful for us, but Git would at least be
internally consistent (in that the symref would be resolved, and the
command would become "git branch -d master").

Are there use cases for this behaviour?

While I don't use symrefs, I'd somehow expect them to behave like
symbolic links on Unix do, where rm removes a link, not its target.

But I wonder why most delete_ref() calls in the code actually don't use
the flag REF_NODEREF, thus deleting symref targets instead of the
symrefs themselves.  I may be missing something important here.

---
 builtin/branch.c  |  2 +-
 t/t3200-branch.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ffd2684..31af114 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -221,7 +221,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			continue;
 		}
 
-		if (delete_ref(name, sha1, 0)) {
+		if (delete_ref(name, sha1, REF_NODEREF)) {
 			error(remote_branch
 			      ? _("Error deleting remote branch '%s'")
 			      : _("Error deleting branch '%s'"),

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 79c8d01..4b73406 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -262,6 +262,16 @@ test_expect_success 'config information was renamed, too' \
 	"test $(git config branch.s.dummy) = Hello &&
 	 test_must_fail git config branch.s/s/dummy"
 
+test_expect_success 'deleting a symref' '
+	git branch target &&
+	git symbolic-ref refs/heads/symlink refs/heads/target &&
+
+	git branch -d symlink &&
+
+	test_path_is_file .git/refs/heads/target &&
+	test_path_is_missing .git/refs/heads/symlink
+'
+
 test_expect_success 'renaming a symref is not allowed' \
 '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&

-- 
1.7.12