threads / patch / 31834

patch, 5 partst1400-update-ref: Add test verifying bug with symrefs in delete_ref()

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

## tl;dr

7 messages between Oct 16, 2012 and Oct 16, 2012. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Johan Herland· Oct 16, 2012, 13:44 UTC · lore

[PATCH 0/5] Fixing problem with deleting symrefs

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)
Show 5 quoted lines
> 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

Johan Herland· Oct 16, 2012, 13:44 UTC · re: Johan Herland · lore

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(+)
Show changes to t/t1400-update-ref.sh +18 −0
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
Johan Herland· Oct 16, 2012, 13:44 UTC · re: Johan Herland · lore

[PATCH 2/5] delete_ref(): Fix deletion of refs through symbolic refs

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(-)
Show changes to 2 files +18 −17

refs.c, t/t1400-update-ref.sh

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
Johan Herland· Oct 16, 2012, 13:44 UTC · re: Johan Herland · lore

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

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(-)
Show changes to refs.c +1 −1
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
Johan Herland· Oct 16, 2012, 13:44 UTC · re: Johan Herland · lore

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

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
Show changes to t/t3220-symbolic-ref-as-branch-alias.sh +53 −0
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
Johan Herland· Oct 16, 2012, 13:44 UTC · re: Johan Herland · lore

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

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(-)
Show changes to 2 files +2 −2

builtin/branch.c, t/t3220-symbolic-ref-as-branch-alias.sh

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
Junio C Hamano· Oct 16, 2012, 16:54 UTC · re: Johan Herland · lore

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

Johan Herland <johan@herland.net> writes:
Show 16 quoted lines
> 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?
Show 8 quoted lines
>> 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.

← back to recent threads