Hi,
At $dayjob we renamed a branch, and for a grace period, we kept the
old name as a symref/alias to the new name, to give our users a window
for switching. This has worked well, until we tried to remove the
symref/alias. The following script demonstrates what we discovered:
$ git --version
git version 1.8.0.rc2.249.g6cc8227
$ git init symref_delete_test/
Initialized empty Git repository in .../symref_delete_test/.git/
$ cd symref_delete_test/
$ echo foo > foo && git add foo && git commit -m foo
[master (root-commit) c7ae77e] foo
1 file changed, 1 insertion(+)
create mode 100644 foo
$ git gc
Counting objects: 3, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
$ cat .git/packed-refs
# pack-refs with: peeled
c7ae77e537138bee3f722e57e1af87a7011466cb refs/heads/master
$ echo bar > foo && git commit -am bar
[master 7451bf0] bar
1 file changed, 1 insertion(+), 1 deletion(-)
$ git symbolic-ref refs/heads/alias refs/heads/master
$ git rev-parse master
7451bf08b7aacedc9e88a9fa37a6c1f701071bbe
$ git rev-parse alias
7451bf08b7aacedc9e88a9fa37a6c1f701071bbe
$ git branch -d alias
Deleted branch alias (was 7451bf0).
$ git rev-parse master
c7ae77e537138bee3f722e57e1af87a7011466cb
$ git rev-parse alias
c7ae77e537138bee3f722e57e1af87a7011466cb
$ cat .git/packed-refs
# pack-refs with: peeled
c7ae77e537138bee3f722e57e1af87a7011466cb refs/heads/master
$ ls .git/refs/heads/
alias
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...
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.
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").
Obviously, I would advocate for option A. What say you?
Have fun! :)
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
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
When delete_ref is called on a symref then it locks its target and then
either deletes the target or the symref, depending on whether the flag
REF_NODEREF was set in the parameter delopt.
Instead, simply pass the flag to lock_ref_sha1_basic, which will then
either lock the target or the symref, and delete the locked ref.
This reimplements part of eca35a25 (Fix git branch -m for symrefs.).
Signed-off-by: Rene Scharfe <redacted>
---
Independent patch, kind of related.
refs.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index da74a2b..9d1685b 100644
--- a/refs.c
+++ b/refs.c
@@ -1753,26 +1753,18 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
struct ref_lock *lock;
int err, i = 0, ret = 0, flag = 0;
- lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
+ lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
if (!lock)
return 1;
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- const char *path;
-
- if (!(delopt & REF_NODEREF)) {
- i = strlen(lock->lk->filename) - 5; /* .lock */
- lock->lk->filename[i] = 0;
- path = lock->lk->filename;
- } else {
- path = git_path("%s", refname);
- }
- err = unlink_or_warn(path);
+ i = strlen(lock->lk->filename) - 5; /* .lock */
+ lock->lk->filename[i] = 0;
+ err = unlink_or_warn(lock->lk->filename);
if (err && errno != ENOENT)
ret = 1;
- if (!(delopt & REF_NODEREF))
- lock->lk->filename[i] = '.';
+ lock->lk->filename[i] = '.';
}
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack--
1.7.12