Re: [PATCH] filter-branch: handle deletion of annotated tags

[Clemens Buchacher <clemens.buchacher@intel.com>]

If filter-branch removes a commit which an annotated tag points to,
and that tag is in the list of refs to be rewritten, we die with an
error like this:

    error: Ref refs/tags/a1t is at 699cce2774f79a0830d8c5f631deca12d4b1ee8c but expected ba247450492030b03e3d2a9d5fef7ef67519483e
    Could not delete refs/tags/a1t

In order to update refs, we first peel the ref until we find a
commit sha1. We then pass the commit sha1 to update-ref as the
<oldvalue> parameter. Please consider the following scenarios:

 a) The ref points to a commit object directly. In this case,
    update-ref will find that the current value of the ref still
    matches oldvalue, and succeeds. This check is redundant, since
    we only just queried the current value.

 b) The ref points to a tag object. In this case, update-ref will
    error out, since the commit sha1 cannot match the current value
    of the ref. If the commit has been removed, or rewritten into
    multiple commits, we simply die. If the commit has been
    rewritten, we output a warning message saying that to rewrite
    tags one should use --tag-name-filter, and then we continue. If
    --tag-name-filter is active, the tag will later be rewritten.

There seems to be no added value in passing the <oldvalue>
parameter. So remove it.

This fixes deletion of tag objects. We also do not die any more if
a tag object points to a commit which has been rewritten into
multiple commits. However, we probably will die later in the
--tag-name-filter code, because it does not seem to handle this
case.

This is a minimalist fix which leaves the following issues open:

 o In the absence of --tag-name-filter, we rewrite lightweight tags, but
   not annotated tags, which is not intuitive. We do output a warning,
   though:

   $ git filter-branch --msg-filter "cat && echo hi" -- --all
   [...]
   WARNING: You said to rewrite tagged commits, but not the corresponding tag.
   WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.

 o Annotated tags are backed up as lightweight tags.

 o Annotated tags are backed up even in the absence of
   --tag-name-filter. But in this case backup is not needed because
   they are not rewritten.

These issues could be solved by moving the tag rewriting logic from
tag-name-filter to the regular ref updating code, and
tag-name-filter should deal only with renaming tags. However, this
would change behavior. Currently, the following command would
rewrite tags:

    git filter-branch --msg-filter "cat && echo hi" \
        --tag-name-filter cat -- --branches

With the suggested behavior, tags would be rewritten only if we
include them in the rev-list options:

    git filter-branch --msg-filter="cat && echo hi" -- --all

I am not sure if we can afford to change behavior like that.

Cc: Thomas Rast <tr@thomasrast.ch>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Clemens Buchacher <clemens.buchacher@intel.com>
---

Re-send with Thomas' email address fixed.

 git-filter-branch.sh     | 20 +++++++++-----------
 t/t7003-filter-branch.sh | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..7ca1d99 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -399,21 +399,19 @@ do
 	case "$rewritten" in
 	'')
 		echo "Ref '$ref' was deleted"
-		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
+		git update-ref -m "filter-branch: delete" -d "$ref" ||
 			die "Could not delete $ref"
 	;;
 	$_x40)
 		echo "Ref '$ref' was rewritten"
-		if ! git update-ref -m "filter-branch: rewrite" \
-					"$ref" $rewritten $sha1 2>/dev/null; then
-			if test $(git cat-file -t "$ref") = tag; then
-				if test -z "$filter_tag_name"; then
-					warn "WARNING: You said to rewrite tagged commits, but not the corresponding tag."
-					warn "WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag."
-				fi
-			else
-				die "Could not rewrite $ref"
+		if test $(git cat-file -t "$ref") = tag; then
+			if test -z "$filter_tag_name"; then
+				warn "WARNING: You said to rewrite tagged commits, but not the corresponding tag."
+				warn "WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag."
 			fi
+		else
+			git update-ref -m "filter-branch: rewrite" "$ref" $rewritten ||
+				die "Could not rewrite $ref"
 		fi
 	;;
 	*)
@@ -423,7 +421,7 @@ do
 		warn "WARNING: Ref '$ref' points to the first one now."
 		rewritten=$(echo "$rewritten" | head -n 1)
 		git update-ref -m "filter-branch: rewrite to first" \
-				"$ref" $rewritten $sha1 ||
+				"$ref" $rewritten ||
 			die "Could not rewrite $ref"
 	;;
 	esac
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 855afda..6a34527 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -261,6 +261,7 @@ test_expect_success 'Subdirectory filter with disappearing trees' '
 '
 
 test_expect_success 'Tag name filtering retains tag message' '
+	git update-ref -d refs/tags/T &&
 	git tag -m atag T &&
 	git cat-file tag T > expect &&
 	git filter-branch -f --tag-name-filter cat &&
@@ -268,6 +269,26 @@ test_expect_success 'Tag name filtering retains tag message' '
 	test_cmp expect actual
 '
 
+test_expect_success "Rewrite commit referenced by annotated tag" '
+	git update-ref -d refs/tags/T &&
+	git tag -a -m atag T &&
+	git rev-parse refs/tags/T^0 >old_commit &&
+	git filter-branch -f --msg-filter "cat && echo foo" --tag-name-filter cat refs/tags/T &&
+	echo tag >type.expect &&
+	git cat-file -t refs/tags/T >type.actual &&
+	test_cmp type.expect type.actual &&
+	git rev-parse refs/tags/T^0 >new_commit &&
+	test_must_fail test_cmp old_commit new_commit
+'
+
+test_expect_success "Remove all commits" '
+	git branch removed-branch &&
+	git tag -a -m atag removed-tag &&
+	git filter-branch -f --commit-filter "skip_commit \"\$@\"" removed-branch removed-tag &&
+	test_must_fail git rev-parse refs/heads/removed-branch &&
+	test_must_fail git rev-parse refs/tags/removed-tag
+'
+
 faux_gpg_tag='object XXXXXX
 type commit
 tag S
-- 
1.9.4