[PATCH] filter-branch: Grok special characters in tag names

The tag rewriting code used a 'sed' expression to substitute the new tag
name into the corresponding field of the annotated tag object. But this is
problematic if the tag name contains special characters. In particular,
if the tag name contained a slash, then the 'sed' expression had a syntax
error. We now protect against this by using 'printf' to assemble the
tag header.

Signed-off-by: Johannes Sixt <redacted>
---
 git-filter-branch.sh     |   12 +++++++-----
 t/t7003-filter-branch.sh |    8 ++++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 2688254..81392ad 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh

@@ -412,15 +412,17 @@ if [ "$filter_tag_name" ]; then
 		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
 
 		if [ "$type" = "tag" ]; then
-			new_sha1=$(git cat-file tag "$ref" |
+			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
+						"$new_sha1" "$new_ref"
+				git cat-file tag "$ref" |
 				sed -n \
 				    -e "1,/^$/{
-					  s/^object .*/object $new_sha1/
-					  s/^type .*/type commit/
-					  s/^tag .*/tag $new_ref/
+					  /^object /d
+					  /^type /d
+					  /^tag /d
 					}" \
 				    -e '/^-----BEGIN PGP SIGNATURE-----/q' \
-				    -e 'p' |
+				    -e 'p' ) |
 				git mktag) ||
 				die "Could not create new tag object for $ref"
 			if git cat-file tag "$ref" | \

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 233254f..95f13a8 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh

@@ -254,4 +254,12 @@ test_expect_success 'Tag name filtering strips gpg signature' '
 	test_cmp expect actual
 '
 
+test_expect_success 'Tag name filtering allows slashes in tag names' '
+	git tag -m tag-with-slash X/1 &&
+	git cat-file tag X/1 | sed -e s,X/1,X/2, > expect &&
+	git filter-branch -f --tag-name-filter "echo X/2" &&
+	git cat-file tag X/2 > actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.6.0.137.g19b5f8c

Johannes Sixt wrote:

Junio complained that my initial version of this was fragile which has
similarities with the above. Initially, I was blindly changing the first line
to contain "object...", second line to "type...", etc.

Would something like the following be equivalent _and_ clearer? Emphasis on "and"
because both are necessary, not because I strongly feel it to be so.

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index a324cf0..11c5c04 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh

@@ -419,9 +419,12 @@ if [ "$filter_tag_name" ]; then
 			new_sha1=$(git cat-file tag "$ref" |
 				sed -n \
 				    -e "1,/^$/{
-					  s/^object .*/object $new_sha1/
-					  s/^type .*/type commit/
-					  s/^tag .*/tag $new_ref/
+					  /^object .*/c\\
+object $new_sha1
+					  /^type .*/c\\
+type commit
+					  /^tag .*/c\\
+tag $new_ref
 					}" \
 				    -e '/^-----BEGIN PGP SIGNATURE-----/q' \
 				    -e 'p' |


-brandon

On Donnerstag, 21. August 2008, Brandon Casey wrote:

Johannes Sixt wrote:

quoted

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 2688254..81392ad 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh

@@ -412,15 +412,17 @@ if [ "$filter_tag_name" ]; then
 		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"

 		if [ "$type" = "tag" ]; then
-			new_sha1=$(git cat-file tag "$ref" |
+			new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
+						"$new_sha1" "$new_ref"
+				git cat-file tag "$ref" |
 				sed -n \
 				    -e "1,/^$/{
-					  s/^object .*/object $new_sha1/
-					  s/^type .*/type commit/
-					  s/^tag .*/tag $new_ref/
+					  /^object /d
+					  /^type /d
+					  /^tag /d

Junio complained that my initial version of this was fragile which has
similarities with the above. Initially, I was blindly changing the first
line to contain "object...", second line to "type...", etc.

I don't see what's wrong with that. "object" must be the first line anyway, 
otherwise git mktag chokes.

This looks fine, too. (The '.*' in /^foo .*/c\\ is unnecessary.)

-- Hannes

Johannes Sixt wrote:

(The '.*' in /^foo .*/c\\ is unnecessary.)

Yeah, I noticed after I sent it.

-brandon