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

René Scharfe [off-list ref] 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.

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

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 <redacted>
---
 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

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 <redacted>
---
 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

Am 21.10.2012 12:40, schrieb Johan Herland:

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é