From: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>
To: Johan Herland <johan@herland.net>
Cc: Git mailing list <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
Miklos Vajna <vmiklos@suse.cz>
Subject: Re: BUG when trying to delete symbolic refs
Date: Tue, 16 Oct 2012 12:05:18 +0200 [thread overview]
Message-ID: <507D315E.8040101@lsrfire.ath.cx> (raw)
In-Reply-To: <CALKQrgfnvV+1XHjeSytj+LxkAabZJK3hewxH7WT0nkX-ewOKUA@mail.gmail.com>
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
next prev parent reply other threads:[~2012-10-16 10:05 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-15 8:50 BUG when trying to delete symbolic refs Johan Herland
2012-10-16 10:05 ` René Scharfe [this message]
2012-10-16 10:22 ` [PATCH] refs: lock symref that is to be deleted, not its target René Scharfe
2012-10-16 16:09 ` BUG when trying to delete symbolic refs Junio C Hamano
2012-10-18 11:59 ` René Scharfe
2012-10-18 12:02 ` [PATCH 1/5] branch: factor out check_branch_commit() René Scharfe
2012-10-18 12:04 ` [PATCH 2/5] branch: factor out delete_branch_config() René Scharfe
2012-10-18 12:05 ` [PATCH 3/5] branch: delete symref branch, not its target René Scharfe
2012-10-18 12:07 ` [PATCH 4/5] branch: skip commit checks when deleting symref branches René Scharfe
2012-10-18 21:34 ` Junio C Hamano
2012-10-21 10:40 ` [PATCH 0/2] Fix remaining issue when deleting symrefs Johan Herland
2012-10-21 10:40 ` [PATCH 1/2] t1400-update-ref: Add test verifying bug with symrefs in delete_ref() Johan Herland
2012-10-21 10:40 ` [PATCH 2/2] Fix failure to delete a packed ref through a symref Johan Herland
2012-10-21 17:46 ` René Scharfe
2012-10-21 19:09 ` Junio C Hamano
2012-10-18 12:08 ` [PATCH 5/5] branch: show targets of deleted symrefs, not sha1s René Scharfe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=507D315E.8040101@lsrfire.ath.cx \
--to=rene.scharfe@lsrfire.ath.cx \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johan@herland.net \
--cc=vmiklos@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).