René Scharfe <rene.scharfe@xxxxxxxxxxxxxx> writes: > 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 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? > --- > 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 && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html