Re: BUG when trying to delete symbolic refs
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:02
Possibly related (same subject, not in this thread)
- 2016-06-15 · BUG when trying to delete symbolic refs · Johan Herland <hidden>
- 2016-06-15 · Re: BUG when trying to delete symbolic refs · René Scharfe <hidden>
René Scharfe [off-list ref] writes:
Am 15.10.2012 10:50, schrieb Johan Herland:quoted
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.quoted
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
Thanks for a reminder.
I can't imagine why deletion should be prohibited as well, though.
I am not sure if it is a good idea to let "update-ref -d" work on a
symref, with or without --no-deref. There are cases where you want
to remove the pointer ("symbolic-ref -d" is there for that), and
there are cases where you want to remove the underlying ref (but of
course you can "update-ref -d" the underlying ref yourself). If
"update-ref -d" refused to work on a symref, we do not have to worry
about the confusion "which one is removed---the pointer, or the
pointee?"
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.
I suspect that is primarily because using a symref to represent anything other than $GIT_DIR/HEAD and $GIT_DIR/refs/remotes/*/HEAD is outside the normally supported use case and in the "may happen to work" territory. Having said all that, I think your patch is going in the right direction. If somebody had a symbolic ref in refs/heads/, the removal should remove it, not the pointee, which may not even exist. Does "branch -d sym" work correctly with your patch when refs/heads/sym is pointing at something that does not exist?
quoted hunk
--- 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 &&