git filter-branch doesn't dereference annotated tags

git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]

Please disregard the previous email that contains an incorrect fix
suggestion. I wish my first contribution was flawless.

Here is what's happening.
git-filter-branch let git-update-ref -d verify that the value for $ref
matches $sha1.
However, when $ref points to an annotated tag that is being deleted,
that verification fails because $sha1 is the commit underneath.

I think there are two possible fixes:
  1) either make git-filter-branch dereference annotated tags and do
the verification itself then use the two arguments version of git
update-ref
  2) in the case of an annotated tag, pass another <old value> to git update-ref

Please find below a patch that implements solution 1). Please note the
patch doesn't contain a unit test for this situation as I wasn't sure
how to provide one. Yet I tested it on the repository I'm working on.

Gregory

>From 9d21960088a61bfbac1ffdb4b13e3038f88ab4d6 Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Mon, 31 Dec 2012 15:30:36 +0100
Subject: [PATCH] git-filter-branch: support annotated tags deletion

git-filter-branch let git-update-ref -d verify that the value for $ref matches
$sha1. However, when $ref is an annotated tag being deleted that verfication
fails because $sha1 corresponds to a commit object.

Instead of asking git-update-ref to verify values actually match, dereference
$ref ourselves and test against $sha1 first. Then invoke git-update-ref with two
arguments.

Signed-off-by: Gregory Pakosz <gpakosz@visionobjects.com>
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..bbee6d0 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -383,7 +383,7 @@ do
 	case "$rewritten" in
 	'')
 		echo "Ref '$ref' was deleted"
-		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
+		test $(git rev-parse --verify "$ref^{commit}") = $sha1 && git
update-ref -m "filter-branch: delete" -d "$ref" ||
 			die "Could not delete $ref"
 	;;
 	$_x40)
-- 
1.8.0.1

[-- Attachment #2: 0001-git-filter-branch-support-annotated-tags-deletion.patch --]
[-- Type: application/octet-stream, Size: 1211 bytes --]

From 9d21960088a61bfbac1ffdb4b13e3038f88ab4d6 Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Mon, 31 Dec 2012 15:30:36 +0100
Subject: [PATCH] git-filter-branch: support annotated tags deletion

git-filter-branch let git-update-ref -d verify that the value for $ref matches
$sha1. However, when $ref is an annotated tag being deleted that verfication
fails because $sha1 corresponds to a commit object.

Instead of asking git-update-ref to verify values actually match, dereference
$ref ourselves and test against $sha1 first. Then invoke git-update-ref with two
arguments.

Signed-off-by: Gregory Pakosz <gpakosz@visionobjects.com>
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..bbee6d0 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -383,7 +383,7 @@ do
 	case "$rewritten" in
 	'')
 		echo "Ref '$ref' was deleted"
-		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
+		test $(git rev-parse --verify "$ref^{commit}") = $sha1 && git update-ref -m "filter-branch: delete" -d "$ref" ||
 			die "Could not delete $ref"
 	;;
 	$_x40)
-- 
1.8.0.1

Re: git filter-branch doesn't dereference annotated tags

[Junio C Hamano]

Grégory Pakosz <gpakosz@visionobjects.com> writes:

>   1) either make git-filter-branch dereference annotated tags and do
> the verification itself then use the two arguments version of git
> update-ref
>   2) in the case of an annotated tag, pass another <old value> to git update-ref
>
> Please find below a patch that implements solution 1).
> ...
>  		echo "Ref '$ref' was deleted"
> -		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
> +		test $(git rev-parse --verify "$ref^{commit}") = $sha1 && git
> update-ref -m "filter-branch: delete" -d "$ref" ||

Thanks.  A few comments.

At the design level.  Where does this $sha1 come from in the first
place?  If a ref that named the annotated tag was deleted, shouldn't
we arrange things so this part of the code receives the $sha1 of the
tag that corresponds to the $ref so that "update-ref -d" can check
that nobody tampered with the repository while the script was
working?

At the implementation level.  When the ref being deleted pointed at
a tree or a blob, the original would have correctly removed it, but
will the updated one?

Re: git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

[-- Attachment #1: Type: text/plain, Size: 4328 bytes --]

> Thanks.  A few comments.
>
> At the design level.  Where does this $sha1 come from in the first
> place?
>
actually, sha1=$(git rev-parse "$ref"^0)
(please remember that I'm discovering git internals while figuring out
how to make git filter-branch work in my use case)

in my use case, $ref is "refs/tags/4.0" which is an annotated tag

$ git rev-parse "refs/tags/4.0"
e941b1999906c17b59320f776d58b71fc2fcdb72

$ git cat-file -t e941b1999906c17b59320f776d58b71fc2fcdb72
tag

$ git rev-parse e941b1999906c17b59320f776d58b71fc2fcdb72^0
dcd7cdc18240dd9a54b30d757dd2347f52040490

$ git cat-file -t dcd7cdc18240dd9a54b30d757dd2347f52040490
commit

so $sha1 is dcd7cdc18240dd9a54b30d757dd2347f52040490

and then git-filter-branch calls git update-ref -m "filter-branch:
delete" -d "refs/tags/4.0" dcd7cdc18240dd9a54b30d757dd2347f52040490
which makes git update-index complains
e941b1999906c17b59320f776d58b71fc2fcdb72 !=
dcd7cdc18240dd9a54b30d757dd2347f52040490

so hmm, adding test $(git rev-parse --verify "$ref^{commit}") = $sha1
as I did in my patch is always true since sha1=$(git rev-parse
"$ref"^0)

>  If a ref that named the annotated tag was deleted, shouldn't
> we arrange things so this part of the code receives the $sha1 of the
> tag that corresponds to the $ref
>
I'm not sure what you mean by "a ref that named the annotated tag was deleted"
What's happening in my situation is that the commit the tag points to
gets rewritten to nothing as the result of my filtering:
"refs/tags/4.0" points to e941b1999906c17b59320f776d58b71fc2fcdb72
(tag) which points to dcd7cdc18240dd9a54b30d757dd2347f52040490
(commit) which gets rewritten to nothing so the tag must be deleted.

> so that "update-ref -d" can check
> that nobody tampered with the repository while the script was
> working?
>
I'm not quite sure what could possibly go well if somebody tampers
with the repository while it's being filtered with git filter-branch
anyways???

If we want to address "did somebody tamper with the repository while
the script was working?", then test $(git rev-parse --verify
"$ref^{commit}") = $sha1 verifies somebody didn't tamper with $ref
since we got $sha1 from it.
But that doesn't ensure tampering didn't take place in between
  test $(git rev-parse --verify "$ref^{commit}") = $sha1
and
  git update-ref -m "filter-branch: delete" -d "$ref".

How defensive should git filter-branch really be?

> At the implementation level.  When the ref being deleted pointed at
> a tree or a blob, the original would have correctly removed it, but
> will the updated one?
>
Yes.

Now that you made me think about it even more, the title of that
thread isn't "git filter-branch doesn't dereference annotated tags".
It in fact does as per sha1=$(git rev-parse "$ref"^0).

Maybe the suggested fix should be: in case the tag points to a commit
that has been rewritten to nothing, get $sha1 again without
dereferencing recursively with sha1=$(git rev-parse "$ref") then use
the 3 arguments versions of git update-ref as before.

Thanks for reading
Gregory

>From 59f86c9c07715734d59009c15816220f996b75be Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Mon, 31 Dec 2012 15:30:36 +0100
Subject: [PATCH] git-filter-branch: support annotated tags deletion

git-filter-branch let git-update-ref -d verify that the value for $ref matches
$sha1. $sha1 is obtained form dereferencing $ref recursively. In case $sha1 gets
rewritten to nothing as per result of the filtering, the tag should be deleted.
However, in case of an annotated tag, git-update-ref -d fails because $ref
doesn't directly point to $sha1.

To make git-filter-branch properly delete an annotated tag, obtain $sha1 again
withouth dereferencing the tag before asking git-update-ref to verify $ref and
$sha1 match.

Signed-off-by: Gregory Pakosz <gpakosz@visionobjects.com>
---
 git-filter-branch.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..7ae9912 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -383,6 +383,7 @@ do
 	case "$rewritten" in
 	'')
 		echo "Ref '$ref' was deleted"
+		test $(git cat-file -t "$ref") = 'tag' && sha1=$(git rev-parse "$ref")
 		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
 			die "Could not delete $ref"
 	;;
-- 
1.8.0.1

[-- Attachment #2: 0001-git-filter-branch-support-annotated-tags-deletion.patch --]
[-- Type: application/octet-stream, Size: 1285 bytes --]

From 59f86c9c07715734d59009c15816220f996b75be Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Mon, 31 Dec 2012 15:30:36 +0100
Subject: [PATCH] git-filter-branch: support annotated tags deletion

git-filter-branch let git-update-ref -d verify that the value for $ref matches
$sha1. $sha1 is obtained form dereferencing $ref recursively. In case $sha1 gets
rewritten to nothing as per result of the filtering, the tag should be deleted.
However, in case of an annotated tag, git-update-ref -d fails because $ref
doesn't directly point to $sha1.

To make git-filter-branch properly delete an annotated tag, obtain $sha1 again
withouth dereferencing the tag before asking git-update-ref to verify $ref and
$sha1 match.

Signed-off-by: Gregory Pakosz <gpakosz@visionobjects.com>
---
 git-filter-branch.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..7ae9912 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -383,6 +383,7 @@ do
 	case "$rewritten" in
 	'')
 		echo "Ref '$ref' was deleted"
+		test $(git cat-file -t "$ref") = 'tag' && sha1=$(git rev-parse "$ref")
 		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
 			die "Could not delete $ref"
 	;;
-- 
1.8.0.1

Re: git filter-branch doesn't dereference annotated tags

[Junio C Hamano]

Grégory Pakosz <gpakosz@visionobjects.com> writes:

> in my use case, $ref is "refs/tags/4.0" which is an annotated tag
>
> $ git rev-parse "refs/tags/4.0"
> e941b1999906c17b59320f776d58b71fc2fcdb72

Yeah, but in that case it appears to me that you told the command to
rewrite the tag itself and the history behind the commit the tag
refers to, but the end result did not rewrite the tag itself and
left the tag pointing at the original history.

The "$rewritten" variable being empty in this codepath tells me that
the command decided that it *does* want to delete the tag, but as
J6t mentioned in his review, it is unclear to me what is expected by
the user.

If the command and the filters you gave the command decided the tag
should be removed, then not being able to delete it is a problem and
the code you patched that compares the object name of the tag and
the object name of the commit the tag refers to is certainly doing a
wrong comparison. 

But I have this suspicion that you did not want to remove the tag in
the first place.  The application of "map" shell function to obtain
$rewritten is done on the unwrapped object name by passing "$ref^0"
to rev-parse, but perhaps that "^0" is the real source of the
problem instead, so that it checks what new object the original
annotated tag was rewritten to?  If the tag object was rewritten,
either to empty to signal its removal or to a new object name, then
we do want to update-ref it, but the decision should be made on its
object name, not the object name of the commit it points at?

Re: git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

> Yeah, but in that case it appears to me that you told the command to
> rewrite the tag itself and the history behind the commit the tag
> refers to, but the end result did not rewrite the tag itself and
> left the tag pointing at the original history.
>
The problem exhibits at the point git filter-branch updates the references.
I indeed asked to rewrite tagged commits by passing --tag-name-filter cat

> The "$rewritten" variable being empty in this codepath tells me that
> the command decided that it *does* want to delete the tag, but as
> J6t mentioned in his review, it is unclear to me what is expected by
> the user.
>
The history behind the commit the tag refers to is empty after
filtering has been applied.
As a user, I expected the tag to be removed: it's not illogical, all
tags that pointed to history that has been entirely filtered out
should go away imho.
--tag-name-filter doesn't allow to deleted tags as J6t mentioned and
I'm not surre what the new contract would be: empty tag name to delete
a tag so if $(map $sha1) yields '' I can decided to delete the tag?

If the tag wasn't an annotated one, I guess it would have been
successfully deleted which exhibits a different behavior between
annotated and lightweight tags.

> If the command and the filters you gave the command decided the tag
> should be removed, then not being able to delete it is a problem and
> the code you patched that compares the object name of the tag and
> the object name of the commit the tag refers to is certainly doing a
> wrong comparison.
>
That's what I believe.
The index and commit filters are made to keep only a couple of
directories and get rid of the rest.
Those directories didn't exist early in the history. Commits in that
early part of the history were tagged with annotated tags.

> But I have this suspicion that you did not want to remove the tag in
> the first place.
>
The tag pointed to something the filters decided to throw out. Since I
want tags to be updated, it doesn't make much sense to keep it and I'm
expecting its deletion.

It appears to me that the suggested fix doing test $(git cat-file -t
"$ref") = 'tag' && sha1=$(git rev-parse "$ref") so that $sha1 is
obtained again from the tag ref but without dereferencing recursively
should only apply if --tag-name-filter has been provided. What do you
think?

> The application of "map" shell function to obtain
> $rewritten is done on the unwrapped object name by passing "$ref^0"
> to rev-parse, but perhaps that "^0" is the real source of the
> problem instead, so that it checks what new object the original
> annotated tag was rewritten to?  If the tag object was rewritten,
> either to empty to signal its removal or to a new object name, then
> we do want to update-ref it, but the decision should be made on its
> object name, not the object name of the commit it points at?
>
Are you suggesting $sha1 should be obtained differently before
entering case "$rewritten" ?
That would mean changing sha1=$(git rev-parse "$ref"^0) at line 376 to
something like $(git cat-file -t "$ref") = 'tag' && sha1=$(git
rev-parse "$ref") || sha1=$(git rev-parse "$ref^0") ?

Gregory

Re: git filter-branch doesn't dereference annotated tags

[Junio C Hamano]

Grégory Pakosz <gpakosz@visionobjects.com> writes:

> Are you suggesting $sha1 should be obtained differently before
> entering case "$rewritten" ?
> That would mean changing sha1=$(git rev-parse "$ref"^0) at line 376 to
> something like $(git cat-file -t "$ref") = 'tag' && sha1=$(git
> rev-parse "$ref") || sha1=$(git rev-parse "$ref^0") ?

I was wondering if it should be

	sha1=$(git rev-parse --verify "$ref")

or something that does not dereference a tag at all.

The way I read what that loop seems to want to do is:

	Read each refname that was given originally from the file
	$tempdir/heads, find out the object it used to refer to and
	have it in $sha1, find out what new object the object was
	rewritten to and have it in $rewritten, and:

	(1) if the rewrite left the object unchanged, do nothing but
	    warn users just in case this was a mistake;
	(2) if the rewrite told us to remove it, then delete the
	    ref; or
        (3) if the rewrite gave us a new object, replace the ref to 
	    point to that new one.

	And in the latter two cases, save the original one in
	$orig_namespace so that the user can choose to recover if
	this filter-branch was done by mistake.

So I do not think unwraping the ref at that point makes any sense,
unless it is not prepared to handle annotated tags at all by
unwrapping tags too early.

What am I missing?

Re: git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

> I was wondering if it should be
>
>         sha1=$(git rev-parse --verify "$ref")
>
> or something that does not dereference a tag at all.
>
> The way I read what that loop seems to want to do is:
>
>         Read each refname that was given originally from the file
>         $tempdir/heads, find out the object it used to refer to and
>         have it in $sha1, find out what new object the object was
>         rewritten to and have it in $rewritten, and:
>
>         (1) if the rewrite left the object unchanged, do nothing but
>             warn users just in case this was a mistake;
>         (2) if the rewrite told us to remove it, then delete the
>             ref; or
>         (3) if the rewrite gave us a new object, replace the ref to
>             point to that new one.
>
>         And in the latter two cases, save the original one in
>         $orig_namespace so that the user can choose to recover if
>         this filter-branch was done by mistake.
>
> So I do not think unwraping the ref at that point makes any sense,
> unless it is not prepared to handle annotated tags at all by
> unwrapping tags too early.
>
> What am I missing?
>
So we have an annotated tag that points to a commit that is rewritten
to nothing as the result of the filtering. What should happen?

My initial questions and patching suggestions are based on git
1.7.10.4 behavior.
However, playing with git HEAD exhibits a slightly different behavior:
it breaks when invoking git mktag line 459 (introduced by
1bf6551e42c79a594689a356a9b14759d55f3cf5):
  error: char7: could not get SHA1 hash
  fatal: invalid tag signature file
  Could not create new tag object for tag-a

It's basically the same problem. In my opinion, lines 447-466 should
take into account $new_sha1 is empty.

Please forgive me again for not having configured my mailer yet :(
When I'm ready to provide a patch that implements a solution we all
agree with I'll use git send-email.
In the mean time, I would like to pursue the discussion in this mail
thread so please find attached a patch that deletes a tag instead of
invoking the tag-name-filter when it detects $new_sha1 is empty.

I tested the patch doesn't break t7003. What do you think?

Gregory

[-- Attachment #2: 0001-git-filter-branch-allow-deletion-of-tags-when-refere.patch --]
[-- Type: application/octet-stream, Size: 2591 bytes --]

From 2cb9d5bc605cc2f2d8a6603b6a06657516959aa6 Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Wed, 2 Jan 2013 23:02:03 +0100
Subject: [PATCH] git-filter-branch: allow deletion of tags when referenced
 commit gets rewritten to nothing

---
 git-filter-branch.sh | 61 ++++++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..2e8569c 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -436,36 +436,41 @@ if [ "$filter_tag_name" ]; then
 
 		[ -f "../map/$sha1" ] || continue
 		new_sha1="$(cat "../map/$sha1")"
-		GIT_COMMIT="$sha1"
-		export GIT_COMMIT
-		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
-			die "tag name filter failed: $filter_tag_name"
-
-		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
-
-		if [ "$type" = "tag" ]; then
-			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
-						"$new_sha1" "$new_ref"
-				git cat-file tag "$ref" |
-				sed -n \
-				    -e '1,/^$/{
-					  /^object /d
-					  /^type /d
-					  /^tag /d
-					}' \
-				    -e '/^-----BEGIN PGP SIGNATURE-----/q' \
-				    -e 'p' ) |
-				git mktag) ||
-				die "Could not create new tag object for $ref"
-			if git cat-file tag "$ref" | \
-			   sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
-			then
-				warn "gpg signature stripped from tag object $sha1t"
+		if [ -n "$new_sha1" ]; then
+			GIT_COMMIT="$sha1"
+			export GIT_COMMIT
+			new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
+				die "tag name filter failed: $filter_tag_name"
+
+			echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
+
+			if [ "$type" = "tag" ]; then
+				new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
+							"$new_sha1" "$new_ref"
+					git cat-file tag "$ref" |
+					sed -n \
+							-e '1,/^$/{
+							/^object /d
+							/^type /d
+							/^tag /d
+						}' \
+							-e '/^-----BEGIN PGP SIGNATURE-----/q' \
+							-e 'p' ) |
+					git mktag) ||
+					die "Could not create new tag object for $ref"
+				if git cat-file tag "$ref" | \
+					sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
+				then
+					warn "gpg signature stripped from tag object $sha1t"
+				fi
 			fi
-		fi
 
-		git update-ref "refs/tags/$new_ref" "$new_sha1" ||
-			die "Could not write tag $new_ref"
+			git update-ref "refs/tags/$new_ref" "$new_sha1" ||
+				die "Could not write tag $new_ref"
+		else
+			git update-ref -d "refs/tags/$ref" "$sha1t" ||
+				die "Could not delete tag $ref"
+		fi
 	done
 fi
 
-- 
1.8.0.1

Re: git filter-branch doesn't dereference annotated tags

[Junio C Hamano]

Grégory Pakosz <gpakosz@visionobjects.com> writes:

> So we have an annotated tag that points to a commit that is rewritten
> to nothing as the result of the filtering. What should happen?

If the user asked to filter that tag itself, it may make sense to
remove it, rather than keeping it pointing at the original commit,
because the commit it used to point at no longer exists in the
alternate history being created by filter-branch.

> It's basically the same problem. In my opinion, lines 447-466 should
> take into account $new_sha1 is empty.

Yeah, I think that is a sensible observation.

Having said that, I welcome comments from others, of course.  My
involvement in this script has been very limited.

Re: git filter-branch doesn't dereference annotated tags

[Johannes Sixt]

Am 03.01.2013 00:19, schrieb Junio C Hamano:
> Grégory Pakosz <gpakosz@visionobjects.com> writes:
> 
>> So we have an annotated tag that points to a commit that is rewritten
>> to nothing as the result of the filtering. What should happen?
> 
> If the user asked to filter that tag itself, it may make sense to
> remove it, rather than keeping it pointing at the original commit,
> because the commit it used to point at no longer exists in the
> alternate history being created by filter-branch.

IOW, if the command was something like

  git filter-branch ...filter options... -- v1.0 master ...

and v1.0 is an annotated tag, then it is reasonable to expect v1.0 to be
deleted if the commit it points to goes away. But if the commit did not
go away, but was rewritten, then it is equally reasonable to expect that
the tag is also rewritten. But I don't think that we currently do the
latter.

Therefore, IMO, a change that implements the former behavior should also
implement the latter behavior.

-- Hannes

Re: git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

>
> IOW, if the command was something like
>
>   git filter-branch ...filter options... -- v1.0 master ...
>
> and v1.0 is an annotated tag, then it is reasonable to expect v1.0 to be
> deleted if the commit it points to goes away. But if the commit did not
> go away, but was rewritten, then it is equally reasonable to expect that
> the tag is also rewritten. But I don't think that we currently do the
> latter.
>
When the commit doesn't go away, the tag is currently being rewritten properly.

> Therefore, IMO, a change that implements the former behavior should also
> implement the latter behavior.
>
The patch in my latest email does both. (yet lacks unit tests for now)

Gregory

Re: git filter-branch doesn't dereference annotated tags

[Johannes Sixt]

Am 03.01.2013 10:50, schrieb Grégory Pakosz:
>>
>> IOW, if the command was something like
>>
>>   git filter-branch ...filter options... -- v1.0 master ...
>>
>> and v1.0 is an annotated tag, then it is reasonable to expect v1.0 to be
>> deleted if the commit it points to goes away. But if the commit did not
>> go away, but was rewritten, then it is equally reasonable to expect that
>> the tag is also rewritten. But I don't think that we currently do the
>> latter.
>>
> When the commit doesn't go away, the tag is currently being rewritten properly.

Indeed, but only if a --tag-name-filter was specified.

>> Therefore, IMO, a change that implements the former behavior should also
>> implement the latter behavior.
>>
> The patch in my latest email does both. (yet lacks unit tests for now)

If it deletes a tag only when --tag-name-filter was specified, than that
should be fine.

-- Hannes

Re: git filter-branch doesn't dereference annotated tags

[Brandon Casey]

On Thu, Jan 3, 2013 at 2:33 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 03.01.2013 10:50, schrieb Grégory Pakosz:
>>>
>>> IOW, if the command was something like
>>>
>>>   git filter-branch ...filter options... -- v1.0 master ...
>>>
>>> and v1.0 is an annotated tag, then it is reasonable to expect v1.0 to be
>>> deleted if the commit it points to goes away. But if the commit did not
>>> go away, but was rewritten, then it is equally reasonable to expect that
>>> the tag is also rewritten. But I don't think that we currently do the
>>> latter.
>>>
>> When the commit doesn't go away, the tag is currently being rewritten properly.
>
> Indeed, but only if a --tag-name-filter was specified.
>
>>> Therefore, IMO, a change that implements the former behavior should also
>>> implement the latter behavior.
>>>
>> The patch in my latest email does both. (yet lacks unit tests for now)
>
> If it deletes a tag only when --tag-name-filter was specified, than that
> should be fine.

Hmm, if a tag name filter _other_ than 'cat' is supplied, I think a
user will expect that the original tags will _not_ be touched, and
especially not deleted.

Rather than blindly deleting the original tag ref, maybe we should
still call the user's tag name filter, and then attempt to delete the
"new" name provided by the filter, if it exists.  If the filter was
'cat', then the new and old names will be the same.

-Brandon

Re: git filter-branch doesn't dereference annotated tags

[Johannes Sixt]

Am 31.12.2012 17:24, schrieb Grégory Pakosz:
> Please disregard the previous email that contains an incorrect fix
> suggestion. I wish my first contribution was flawless.
> 
> Here is what's happening.
> git-filter-branch let git-update-ref -d verify that the value for $ref
> matches $sha1.
> However, when $ref points to an annotated tag that is being deleted,
> that verification fails because $sha1 is the commit underneath.
> 
> I think there are two possible fixes:
>   1) either make git-filter-branch dereference annotated tags and do
> the verification itself then use the two arguments version of git
> update-ref
>   2) in the case of an annotated tag, pass another <old value> to git update-ref
> 
> Please find below a patch that implements solution 1). Please note the
> patch doesn't contain a unit test for this situation as I wasn't sure
> how to provide one. Yet I tested it on the repository I'm working on.
> 
> Gregory

We write material like this below the three-dash line of the patch.

> 
> From 9d21960088a61bfbac1ffdb4b13e3038f88ab4d6 Mon Sep 17 00:00:00 2001
> From: Gregory Pakosz <gpakosz@visionobjects.com>
> Date: Mon, 31 Dec 2012 15:30:36 +0100

Then you can remove these three lines because they are inferred from the
email message. And you should not attach the patch, only place it
inline, but make sure that it is not line-wrapped and space-mangled on
the mail route.

> Subject: [PATCH] git-filter-branch: support annotated tags deletion
> 
> git-filter-branch let git-update-ref -d verify that the value for $ref matches
> $sha1. However, when $ref is an annotated tag being deleted that verfication
> fails because $sha1 corresponds to a commit object.
> 
> Instead of asking git-update-ref to verify values actually match, dereference
> $ref ourselves and test against $sha1 first. Then invoke git-update-ref with two
> arguments.

It would have been very helpful if you could summarize the conditions
under which the unexpected behavior happens in the commit message. A
test case would also be great; it should be a matter of inserting
another case in t/t7003.

Without that information, I can't decide whether it is a good thing that
a tag (annotated or not) is deleted, because we have --tag-name-filter
to treat tags (even though you can't delete tags with this feature).

> Signed-off-by: Gregory Pakosz <gpakosz@visionobjects.com>
> ---
>  git-filter-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 5314249..bbee6d0 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -383,7 +383,7 @@ do
>  	case "$rewritten" in
>  	'')
>  		echo "Ref '$ref' was deleted"
> -		git update-ref -m "filter-branch: delete" -d "$ref" $sha1 ||
> +		test $(git rev-parse --verify "$ref^{commit}") = $sha1 && git
> update-ref -m "filter-branch: delete" -d "$ref" ||
>  			die "Could not delete $ref"

As written, it counts as error "Could not delete $ref" if the test...
command fails. Is this intended?

-- Hannes

git filter-branch doesn't dereference annotated tags

[Grégory Pakosz]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

Hello,

I noticed git-filter-branch doesn't dereference annotated tags prior
to invoking git update-ref -d.

Please find a patch attached that changes the call to git update-ref:

-git update-ref -m "filter-branch: delete" -d "$ref" $sha1
+git update-ref -m "filter-branch: delete" -d $(git rev-parse --verify
"$ref^{commit}") $sha1

Regards,
Gregory

[-- Attachment #2: 0001-git-filter-branch-Dereference-annotated-tags-upon-de.patch --]
[-- Type: application/octet-stream, Size: 869 bytes --]

From cee5462f26bbb280f471ba1220398924bfd4bfd7 Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Mon, 31 Dec 2012 15:30:36 +0100
Subject: [PATCH] git-filter-branch: Dereference annotated tags upon deletion

git-filter-branch didn't dereference annotated tags upon deletion which made
git-update-ref -d unhappy.
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..773a91b 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -383,7 +383,7 @@ 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 $(git rev-parse --verify "$ref^{commit}") $sha1 ||
 			die "Could not delete $ref"
 	;;
 	$_x40)
-- 
1.8.0.1