BUG when trying to delete symbolic refs

BUG when trying to delete symbolic refs

[Johan Herland]

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, <johan@herland.net>
www.herland.net

Re: BUG when trying to delete symbolic refs

[René Scharfe]

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

[PATCH] refs: lock symref that is to be deleted, not its target

[René Scharfe]

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 <rene.scharfe@lsrfire.ath.cx>
---
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

Re: BUG when trying to delete symbolic refs

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> 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 &&

Re: BUG when trying to delete symbolic refs

[René Scharfe]

Am 16.10.2012 18:09, schrieb Junio C Hamano:
> 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?

No, it doesn't, neither with nor without the patch.  But we can make it
work, and also address a UI issue.  This series starts with two patches
that only move code around, then follows the patch you commented on, a
patch addressing dangling symrefs and finally a change to the way
deleted symrefs are reported by git branch.

  branch: factor out check_branch_commit()
  branch: factor out delete_branch_config()
  branch: delete symref branch, not its target
  branch: skip commit checks when deleting symref branches
  branch: show targets of deleted symrefs, not sha1s

 builtin/branch.c  | 75 ++++++++++++++++++++++++++++++++++++-------------------
 t/t3200-branch.sh | 19 ++++++++++++++
 2 files changed, 68 insertions(+), 26 deletions(-)

-- 
1.7.12

[PATCH 1/5] branch: factor out check_branch_commit()

[René Scharfe]

Move the code to perform checks on the tip commit of a branch
to its own function.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/branch.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ffd2684..852019e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -154,10 +154,28 @@ static int branch_merged(int kind, const char *name,
 	return merged;
 }
 
+static int check_branch_commit(const char *branchname, const char *refname,
+			       unsigned char *sha1, struct commit *head_rev,
+			       int kinds, int force)
+{
+	struct commit *rev = lookup_commit_reference(sha1);
+	if (!rev) {
+		error(_("Couldn't look up commit object for '%s'"), refname);
+		return -1;
+	}
+	if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
+		error(_("The branch '%s' is not fully merged.\n"
+		      "If you are sure you want to delete it, "
+		      "run 'git branch -D %s'."), branchname, branchname);
+		return -1;
+	}
+	return 0;
+}
+
 static int delete_branches(int argc, const char **argv, int force, int kinds,
 			   int quiet)
 {
-	struct commit *rev, *head_rev = NULL;
+	struct commit *head_rev = NULL;
 	unsigned char sha1[20];
 	char *name = NULL;
 	const char *fmt;
@@ -206,17 +224,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			continue;
 		}
 
-		rev = lookup_commit_reference(sha1);
-		if (!rev) {
-			error(_("Couldn't look up commit object for '%s'"), name);
-			ret = 1;
-			continue;
-		}
-
-		if (!force && !branch_merged(kinds, bname.buf, rev, head_rev)) {
-			error(_("The branch '%s' is not fully merged.\n"
-			      "If you are sure you want to delete it, "
-			      "run 'git branch -D %s'."), bname.buf, bname.buf);
+		if (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
+					force)) {
 			ret = 1;
 			continue;
 		}
-- 
1.7.12

[PATCH 2/5] branch: factor out delete_branch_config()

[René Scharfe]

Provide a small helper function for deleting branch config sections.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/branch.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 852019e..97c7361 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -172,6 +172,15 @@ static int check_branch_commit(const char *branchname, const char *refname,
 	return 0;
 }
 
+static void delete_branch_config(const char *branchname)
+{
+	struct strbuf buf = STRBUF_INIT;
+	strbuf_addf(&buf, "branch.%s", branchname);
+	if (git_config_rename_section(buf.buf, NULL) < 0)
+		warning(_("Update of config-file failed"));
+	strbuf_release(&buf);
+}
+
 static int delete_branches(int argc, const char **argv, int force, int kinds,
 			   int quiet)
 {
@@ -237,17 +246,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			      bname.buf);
 			ret = 1;
 		} else {
-			struct strbuf buf = STRBUF_INIT;
 			if (!quiet)
 				printf(remote_branch
 				       ? _("Deleted remote branch %s (was %s).\n")
 				       : _("Deleted branch %s (was %s).\n"),
 				       bname.buf,
 				       find_unique_abbrev(sha1, DEFAULT_ABBREV));
-			strbuf_addf(&buf, "branch.%s", bname.buf);
-			if (git_config_rename_section(buf.buf, NULL) < 0)
-				warning(_("Update of config-file failed"));
-			strbuf_release(&buf);
+			delete_branch_config(bname.buf);
 		}
 	}
 
-- 
1.7.12

[PATCH 3/5] branch: delete symref branch, not its target

[René Scharfe]

If a branch that is to be deleted happens to be a symref to another
branch, the current code removes the targeted branch instead of the
one it was called for.

Change this surprising behaviour and delete the symref branch
instead.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/branch.c  |  2 +-
 t/t3200-branch.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 97c7361..5e1e5b4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -239,7 +239,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..ec5f70e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -262,6 +262,17 @@ 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/symref refs/heads/target &&
+	sha1=$(git rev-parse symref | cut -c 1-7) &&
+	echo "Deleted branch symref (was $sha1)." >expect &&
+	git branch -d symref >actual &&
+	test_path_is_file .git/refs/heads/target &&
+	test_path_is_missing .git/refs/heads/symref &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'renaming a symref is not allowed' \
 '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
-- 
1.7.12

[PATCH 4/5] branch: skip commit checks when deleting symref branches

[René Scharfe]

Before a branch is deleted, we check that it points to a valid
commit.  With -d we also check that the commit is a merged; this
check is not done with -D.

The reason for that is that commits pointed to by branches should
never go missing; if they do then something broke and it's better
to stop instead of adding to the mess.  And a non-merged commit
may contain changes that are worth preserving, so we require the
stronger option -D instead of -d to get rid of them.

If a branch consists of a symref, these concerns don't apply.
Deleting such a branch can't make a commit become unreferenced,
so we don't need to check if it is merged, or even if it is
actually a valid commit.  Skip them in that case.  This allows
us to delete dangling symref branches.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/branch.c  | 10 ++++++++--
 t/t3200-branch.sh |  9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 5e1e5b4..d87035a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -214,6 +214,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			die(_("Couldn't look up commit object for HEAD"));
 	}
 	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
+		const char *target;
+		int flags = 0;
+
 		strbuf_branchname(&bname, argv[i]);
 		if (kinds == REF_LOCAL_BRANCH && !strcmp(head, bname.buf)) {
 			error(_("Cannot delete the branch '%s' "
@@ -225,7 +228,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		free(name);
 
 		name = mkpathdup(fmt, bname.buf);
-		if (read_ref(name, sha1)) {
+		target = resolve_ref_unsafe(name, sha1, 0, &flags);
+		if (!target ||
+		    (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
 			error(remote_branch
 			      ? _("remote branch '%s' not found.")
 			      : _("branch '%s' not found."), bname.buf);
@@ -233,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			continue;
 		}
 
-		if (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
+		if (!(flags & REF_ISSYMREF) &&
+		    check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
 					force)) {
 			ret = 1;
 			continue;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ec5f70e..1323f6f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -273,6 +273,15 @@ test_expect_success 'deleting a symref' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'deleting a dangling symref' '
+	git symbolic-ref refs/heads/dangling-symref nowhere &&
+	test_path_is_file .git/refs/heads/dangling-symref &&
+	echo "Deleted branch dangling-symref (was 0000000)." >expect &&
+	git branch -d dangling-symref >actual &&
+	test_path_is_missing .git/refs/heads/dangling-symref &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'renaming a symref is not allowed' \
 '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
-- 
1.7.12

[PATCH 5/5] branch: show targets of deleted symrefs, not sha1s

[René Scharfe]

git branch reports the abbreviated hash of the head commit of
a deleted branch to make it easier for a user to undo the
operation.  For symref branches this doesn't help.  Print the
symref target instead for them.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin/branch.c  | 19 +++++++++++--------
 t/t3200-branch.sh |  5 ++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index d87035a..1ec9c02 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,15 +251,18 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			      : _("Error deleting branch '%s'"),
 			      bname.buf);
 			ret = 1;
-		} else {
-			if (!quiet)
-				printf(remote_branch
-				       ? _("Deleted remote branch %s (was %s).\n")
-				       : _("Deleted branch %s (was %s).\n"),
-				       bname.buf,
-				       find_unique_abbrev(sha1, DEFAULT_ABBREV));
-			delete_branch_config(bname.buf);
+			continue;
+		}
+		if (!quiet) {
+			printf(remote_branch
+			       ? _("Deleted remote branch %s (was %s).\n")
+			       : _("Deleted branch %s (was %s).\n"),
+			       bname.buf,
+			       (flags & REF_ISSYMREF)
+			       ? target
+			       : find_unique_abbrev(sha1, DEFAULT_ABBREV));
 		}
+		delete_branch_config(bname.buf);
 	}
 
 	free(name);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 1323f6f..80e6be3 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -265,8 +265,7 @@ test_expect_success 'config information was renamed, too' \
 test_expect_success 'deleting a symref' '
 	git branch target &&
 	git symbolic-ref refs/heads/symref refs/heads/target &&
-	sha1=$(git rev-parse symref | cut -c 1-7) &&
-	echo "Deleted branch symref (was $sha1)." >expect &&
+	echo "Deleted branch symref (was refs/heads/target)." >expect &&
 	git branch -d symref >actual &&
 	test_path_is_file .git/refs/heads/target &&
 	test_path_is_missing .git/refs/heads/symref &&
@@ -276,7 +275,7 @@ test_expect_success 'deleting a symref' '
 test_expect_success 'deleting a dangling symref' '
 	git symbolic-ref refs/heads/dangling-symref nowhere &&
 	test_path_is_file .git/refs/heads/dangling-symref &&
-	echo "Deleted branch dangling-symref (was 0000000)." >expect &&
+	echo "Deleted branch dangling-symref (was nowhere)." >expect &&
 	git branch -d dangling-symref >actual &&
 	test_path_is_missing .git/refs/heads/dangling-symref &&
 	test_i18ncmp expect actual
-- 
1.7.12

Re: [PATCH 4/5] branch: skip commit checks when deleting symref branches

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Before a branch is deleted, we check that it points to a valid
> commit.  With -d we also check that the commit is a merged; this
> check is not done with -D.
>
> The reason for that is that commits pointed to by branches should
> never go missing; if they do then something broke and it's better
> to stop instead of adding to the mess.  And a non-merged commit
> may contain changes that are worth preserving, so we require the
> stronger option -D instead of -d to get rid of them.
>
> If a branch consists of a symref, these concerns don't apply.
> Deleting such a branch can't make a commit become unreferenced,
> so we don't need to check if it is merged, or even if it is
> actually a valid commit.  Skip them in that case.  This allows
> us to delete dangling symref branches.

Purist in me tells me that we should be using "symbolic-ref -d" to
correct from such a misconfiguration, but ignoring that, the above
logic makes perfect sense to me, especially together with the next
change to tell what branch the symref was pointing at, which can be
used by the user when the user removes such a symbolic ref by
mistake.

Thanks.  Will queue all five.

[PATCH 0/2] Fix remaining issue when deleting symrefs

[Johan Herland]

The following two patches are based on rs/branch-del-symref, and fixes
the remaining failure to delete a packed ref through a symref.

The first patch demonstrates the bug with a testcase, and the second
patch fixes the bug.

Feel free to squash the two patches into one, if you prefer to keep
both the testcase and subsequent fix in a single commit.


Have fun! :)

...Johan

Johan Herland (2):
  t1400-update-ref: Add test verifying bug with symrefs in delete_ref()
  Fix failure to delete a packed ref through a symref

 refs.c                |  2 +-
 t/t1400-update-ref.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

--
1.7.12.1.609.g5cd6968

[PATCH 1/2] t1400-update-ref: Add test verifying bug with symrefs in delete_ref()

[Johan Herland]

When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'
to delete refs/heads/master), we currently fail to remove the packed
version of that ref. This testcase demonstrates the bug.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t1400-update-ref.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 4fd83a6..f7ec203 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -74,6 +74,24 @@ test_expect_success "delete $m (by HEAD)" '
 '
 rm -f .git/$m
 
+test_expect_success \
+	"create $m (by HEAD)" \
+	"git update-ref HEAD $A &&
+	 test $A"' = $(cat .git/'"$m"')'
+test_expect_success \
+	"pack refs" \
+	"git pack-refs --all"
+test_expect_success \
+	"move $m (by HEAD)" \
+	"git update-ref HEAD $B $A &&
+	 test $B"' = $(cat .git/'"$m"')'
+test_expect_failure "delete $m (by HEAD) should remove both packed and loose $m" '
+	git update-ref -d HEAD $B &&
+	! grep "$m" .git/packed-refs &&
+	! test -f .git/$m
+'
+rm -f .git/$m
+
 cp -f .git/HEAD .git/HEAD.orig
 test_expect_success "delete symref without dereference" '
 	git update-ref --no-deref -d HEAD &&
-- 
1.7.12.1.609.g5cd6968

[PATCH 2/2] Fix failure to delete a packed ref through a symref

[Johan Herland]

When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'
to delete refs/heads/master), we would remove the loose ref, but a packed
version of the same ref would remain, the end result being that instead of
deleting refs/heads/master we would appear to reset it to its state as of
the last repack.

This patch fixes the issue, by making sure we pass the correct ref name
when invoking repack_without_ref() from within delete_ref().

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c                | 2 +-
 t/t1400-update-ref.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 726c53c..6cec1c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	 * packed one.  Also, if it was not loose we need to repack
 	 * without it.
 	 */
-	ret |= repack_without_ref(refname);
+	ret |= repack_without_ref(lock->ref_name);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
 	invalidate_ref_cache(NULL);
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f7ec203..e415ee0 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -85,7 +85,7 @@ test_expect_success \
 	"move $m (by HEAD)" \
 	"git update-ref HEAD $B $A &&
 	 test $B"' = $(cat .git/'"$m"')'
-test_expect_failure "delete $m (by HEAD) should remove both packed and loose $m" '
+test_expect_success "delete $m (by HEAD) should remove both packed and loose $m" '
 	git update-ref -d HEAD $B &&
 	! grep "$m" .git/packed-refs &&
 	! test -f .git/$m
-- 
1.7.12.1.609.g5cd6968

Re: [PATCH 2/2] Fix failure to delete a packed ref through a symref

[René Scharfe]

Am 21.10.2012 12:40, schrieb Johan Herland:
> When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'
> to delete refs/heads/master), we would remove the loose ref, but a packed
> version of the same ref would remain, the end result being that instead of
> deleting refs/heads/master we would appear to reset it to its state as of
> the last repack.
>
> This patch fixes the issue, by making sure we pass the correct ref name
> when invoking repack_without_ref() from within delete_ref().
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---
>   refs.c                | 2 +-
>   t/t1400-update-ref.sh | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 726c53c..6cec1c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>   	 * packed one.  Also, if it was not loose we need to repack
>   	 * without it.
>   	 */
> -	ret |= repack_without_ref(refname);
> +	ret |= repack_without_ref(lock->ref_name);
>
>   	unlink_or_warn(git_path("logs/%s", lock->ref_name));
>   	invalidate_ref_cache(NULL);

Looks reasonable.

FWIW, this is independent of 547d058f in next (refs: lock symref that is 
to be deleted, not its target), which only affects behaviour when 
REF_NODEREF is set, while this one here only makes a difference with 
symrefs and REF_NODEREF unset.

René

Re: [PATCH 2/2] Fix failure to delete a packed ref through a symref

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'
> to delete refs/heads/master), we would remove the loose ref, but a packed
> version of the same ref would remain, the end result being that instead of
> deleting refs/heads/master we would appear to reset it to its state as of
> the last repack.
>
> This patch fixes the issue, by making sure we pass the correct ref name
> when invoking repack_without_ref() from within delete_ref().
>
> Signed-off-by: Johan Herland <johan@herland.net>
> ---

Thanks.  Will queue.

>  refs.c                | 2 +-
>  t/t1400-update-ref.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 726c53c..6cec1c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1779,7 +1779,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>  	 * packed one.  Also, if it was not loose we need to repack
>  	 * without it.
>  	 */
> -	ret |= repack_without_ref(refname);
> +	ret |= repack_without_ref(lock->ref_name);
>  
>  	unlink_or_warn(git_path("logs/%s", lock->ref_name));
>  	invalidate_ref_cache(NULL);
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index f7ec203..e415ee0 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -85,7 +85,7 @@ test_expect_success \
>  	"move $m (by HEAD)" \
>  	"git update-ref HEAD $B $A &&
>  	 test $B"' = $(cat .git/'"$m"')'
> -test_expect_failure "delete $m (by HEAD) should remove both packed and loose $m" '
> +test_expect_success "delete $m (by HEAD) should remove both packed and loose $m" '
>  	git update-ref -d HEAD $B &&
>  	! grep "$m" .git/packed-refs &&
>  	! test -f .git/$m