[PATCH 0/5] Fixing problem with deleting symrefs

[PATCH 0/5] Fixing problem with deleting symrefs

[Johan Herland]

I see that Rene Scharfe has also worked on the same issue, while I was
preparing these patches...

On Mon, Oct 15, 2012 at 11:29 AM, Junio C Hamano <jch2355@gmail.com> wrote:
> Even though update-ref deferences a symref when it updates one to point at a
> new object, I personally don't think update-ref -d that derefs makes any
> sense. I'd rather see it error out when given a symref, with or without
> --no-deref option.

I'm not sure. We have multiple testcases that directly test deleting a ref
through a symref (e.g. t1400), so supporting this seems like a concious
decision. Erroring out when given a symref will break the following
testcases:
 - t1400 (git update-ref -d)
 - t3310 & t3311 (git notes merge --abort is broken)
 - t5505 (git remote set-head --delete and renaming a remote is broken)

> But even if it did, removing a ref pointed by a symref should really remove
> it, and forgetting to remove a leftover entry in packed-ref has no excuse
> bug.
>
> I'd say what you observed is a double bug.

Patch #1 - #2 fixes the 2nd bug (removing through a symref should remove
both loose and packed versions of the ref).

Patch #3 fixes an associated problem where deleting a symref would remove
the dereferenced ref's reflog instead of the symref's reflog.

Patch #4 - #5 introduces solution A presented in my previous mail (i.e.
'git branch -d' never dereferences symrefs).

Johan Herland (5):
  t1400-update-ref: Add test verifying bug with symrefs in delete_ref()
  delete_ref(): Fix deletion of refs through symbolic refs
  delete_ref(): Remove the correct reflog when deleting symrefs
  Add tests for using symbolic refs as branch name aliases.
  branch -d: Fix failure to remove branch aliases (symrefs)

 builtin/branch.c                        |  2 +-
 refs.c                                  | 35 +++++++++++-----------
 t/t1400-update-ref.sh                   | 18 +++++++++++
 t/t3220-symbolic-ref-as-branch-alias.sh | 53 +++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 18 deletions(-)
 create mode 100755 t/t3220-symbolic-ref-as-branch-alias.sh

--
1.7.12.1.609.g5cd6968

[PATCH 1/5] 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/5] delete_ref(): Fix deletion of refs through symbolic refs

[Johan Herland]

When deleting a ref through a symref (e.g. using 'git update-ref -d HEAD'
to delete refs/heads/master), the packed version of that ref would not
be deleted, because delete_ref() would pass the symref name (as opposed
to the dereferenced ref name) to repack_without_ref().

This patch revamps the logic in delete_ref() to make it easier to read,
and to clarify how it operates when given a symref.

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

diff --git a/refs.c b/refs.c
index da74a2b..df4fe20 100644
--- a/refs.c
+++ b/refs.c
@@ -1751,34 +1751,35 @@ static int repack_without_ref(const char *refname)
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
 	struct ref_lock *lock;
-	int err, i = 0, ret = 0, flag = 0;
+	int err, delete_symref, ret = 0, flag = 0;
 
 	lock = lock_ref_sha1_basic(refname, sha1, 0, &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);
+	/* The following variables are at play here:
+	 *  - refname may be a symref (in this case lock->orig_ref_name holds
+	 *    the symref name, and lock->ref_name holds the dereferenced name)
+	 *  - The dereferenced ref name (lock->ref_name) may be a loose ref, a
+	 *    packed ref, or both.
+	 *  - If REF_NODEREF is enabled - and refname is a symref, we should
+	 *    delete the symref; otherwise delete the dereferenced ref.
+	 */
+	delete_symref = (flag & REF_ISSYMREF && delopt & REF_NODEREF);
+	refname = delete_symref ? lock->orig_ref_name : lock->ref_name;
+
+	if (!(flag & REF_ISPACKED) || delete_symref) {
+		/* loose */
+		err = unlink_or_warn(git_path("%s", refname));
 		if (err && errno != ENOENT)
 			ret = 1;
-
-		if (!(delopt & REF_NODEREF))
-			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
 	 * without it.
 	 */
-	ret |= repack_without_ref(refname);
+	if (!delete_symref)
+		ret |= repack_without_ref(refname);
 
 	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

[PATCH 3/5] delete_ref(): Remove the correct reflog when deleting symrefs

[Johan Herland]

When deleting a symref (e.g. HEAD), we would incorrectly remove the
reflog of the dereferenced ref (e.g. .git/logs/refs/heads/master),
insted of the symref's reflog (e.g. .git/logs/HEAD).

This patch ensures that we remove the reflog that corresponds to the
removed (sym)ref.

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index df4fe20..f2508bf 100644
--- a/refs.c
+++ b/refs.c
@@ -1781,7 +1781,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	if (!delete_symref)
 		ret |= repack_without_ref(refname);
 
-	unlink_or_warn(git_path("logs/%s", lock->ref_name));
+	unlink_or_warn(git_path("logs/%s", refname));
 	invalidate_ref_cache(NULL);
 	unlock_ref(lock);
 	return ret;
-- 
1.7.12.1.609.g5cd6968

[PATCH 4/5] Add tests for using symbolic refs as branch name aliases.

[Johan Herland]

A branch name alias is an alternative name for a branch, that is in most
respects equivalent to using the proper branch name. It is implemented as
a symbolic ref from the alias to the proper branch name.

Currently branch aliases work well up to the point where you try to delete
them (with "git branch -d"), at which point they incorrectly delete the
proper branch name instead of the alias. This is reflected in these tests,
and will be fixed in a subsequent patch.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t3220-symbolic-ref-as-branch-alias.sh | 53 +++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 t/t3220-symbolic-ref-as-branch-alias.sh

diff --git a/t/t3220-symbolic-ref-as-branch-alias.sh b/t/t3220-symbolic-ref-as-branch-alias.sh
new file mode 100755
index 0000000..39f3a33
--- /dev/null
+++ b/t/t3220-symbolic-ref-as-branch-alias.sh
@@ -0,0 +1,53 @@
+#!/bin/sh
+
+test_description='Using a symbolic ref as a branch name alias
+
+This test uses refs/heads/alias as a symbolic ref to refs/heads/master, and
+verifies that it works as a branch name alias, namely:
+ - commits on "master" are automatically reflected on "alias"
+ - creating or deleting "alias" does not affect "master"
+ - the "alias" is not broken by "git gc"
+'
+. ./test-lib.sh
+
+test_expect_success 'prepare a trivial repository' '
+	echo Hello > A &&
+	git add A &&
+	git commit -m "Hello" &&
+	git rev-parse --verify HEAD > expect
+'
+
+test_expect_success 'create symref: refs/heads/alias -> refs/heads/master' '
+	git symbolic-ref refs/heads/alias refs/heads/master &&
+	git rev-parse --verify master > master &&
+	git rev-parse --verify alias > alias &&
+	test_cmp expect master &&
+	test_cmp expect alias
+'
+
+test_expect_success '"git gc" does not change "alias" symref' '
+	git gc &&
+	git rev-parse --verify master > master &&
+	git rev-parse --verify alias > alias &&
+	test_cmp expect master &&
+	test_cmp expect alias
+'
+
+test_expect_success 'commits are reflected through "alias" symref' '
+	echo World >> A &&
+	git commit -am "A" &&
+	git rev-parse --verify HEAD > expect &&
+	git rev-parse --verify master > master &&
+	git rev-parse --verify alias > alias &&
+	test_cmp expect master &&
+	test_cmp expect alias
+'
+
+test_expect_failure '"branch -d" deletes the "alias" symref' '
+	git branch -d alias &&
+	git rev-parse --verify master > master &&
+	test_must_fail git rev-parse --verify alias &&
+	test_cmp expect master
+'
+
+test_done
-- 
1.7.12.1.609.g5cd6968

[PATCH 5/5] branch -d: Fix failure to remove branch aliases (symrefs)

[Johan Herland]

With refs/heads/alias set up as a symref to refs/heads/master,
'git branch -d alias' should remove refs/heads/alias and not
refs/heads/master.

Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/branch.c                        | 2 +-
 t/t3220-symbolic-ref-as-branch-alias.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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/t3220-symbolic-ref-as-branch-alias.sh b/t/t3220-symbolic-ref-as-branch-alias.sh
index 39f3a33..8ebec7a 100755
--- a/t/t3220-symbolic-ref-as-branch-alias.sh
+++ b/t/t3220-symbolic-ref-as-branch-alias.sh
@@ -43,7 +43,7 @@ test_expect_success 'commits are reflected through "alias" symref' '
 	test_cmp expect alias
 '
 
-test_expect_failure '"branch -d" deletes the "alias" symref' '
+test_expect_success '"branch -d" deletes the "alias" symref' '
 	git branch -d alias &&
 	git rev-parse --verify master > master &&
 	test_must_fail git rev-parse --verify alias &&
-- 
1.7.12.1.609.g5cd6968

Re: [PATCH 0/5] Fixing problem with deleting symrefs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I see that Rene Scharfe has also worked on the same issue, while I was
> preparing these patches...
>
> On Mon, Oct 15, 2012 at 11:29 AM, Junio C Hamano <jch2355@gmail.com> wrote:
>> Even though update-ref deferences a symref when it updates one to point at a
>> new object, I personally don't think update-ref -d that derefs makes any
>> sense. I'd rather see it error out when given a symref, with or without
>> --no-deref option.
>
> I'm not sure. We have multiple testcases that directly test deleting a ref
> through a symref (e.g. t1400), so supporting this seems like a concious
> decision. Erroring out when given a symref will break the following
> testcases:
>  - t1400 (git update-ref -d)
>  - t3310 & t3311 (git notes merge --abort is broken)
>  - t5505 (git remote set-head --delete and renaming a remote is broken)

"Concious" does not necessarily mean "Sane", though.  But this is
water under the bridge.  Too many people must have started relying
on this crazy "feature" since mid 2008, and removing it would break
them.

 - "update-ref -d --no-deref SYMREF", even though it is a synonym
   for "symbolic-ref -d SYMREF", makes sense.  Removing it would
   only buy us breakage.

 - "update-ref -d --no-deref SYMREF OLDSHA1" does not make *ANY*
   sense as a request, but again it would not hurt to keep it.

 - "update-ref -d --deref SYMREF [OLDSHA1]" is questionable.  What
   is the use case?  What is the next step after doing such an
   operation, now you have SYMREF that is dangling?

>> But even if it did, removing a ref pointed by a symref should really remove
>> it, and forgetting to remove a leftover entry in packed-ref has no excuse
>> bug.
>>
>> I'd say what you observed is a double bug.
>
> Patch #1 - #2 fixes the 2nd bug (removing through a symref should remove
> both loose and packed versions of the ref).

OK.  That is surely needed.