[PATCH] filter-branch: pass tag name via stdin without newline

[PATCH] filter-branch: pass tag name via stdin without newline

[Eric N. Vander Weele]

"git filter-branch --tag-name-filter" fails when the user-provided
command attempts to trivially append text to the originally tag name,
passed via stdin, due to an unexpected newline ('\n').  The newline is
introduced due to "echo" piping the original tag name to the
user-provided tag name filter command.

The only portable usage of "echo" is without any options and escape
sequences.  Therefore, replacing "echo" with "printf" is a suitable,
POSIX compliant alternative.

Signed-off-by: Eric N. Vander Weele <ericvw@gmail.com>
---
 git-filter-branch.sh     | 2 +-
 t/t7003-filter-branch.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 98f1779..949cd30 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -503,7 +503,7 @@ if [ "$filter_tag_name" ]; then
 		new_sha1="$(cat "../map/$sha1")"
 		GIT_COMMIT="$sha1"
 		export GIT_COMMIT
-		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
+		new_ref="$(printf "$ref" | eval "$filter_tag_name")" ||
 			die "tag name filter failed: $filter_tag_name"
 
 		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 869e0bf..0db6808 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'Tag name filter does not pass tag ref with newline' '
+	git filter-branch -f --tag-name-filter "cat && printf "_append"" -- A &&
+	git rev-parse A_append > /dev/null 2>&1
+'
+
 faux_gpg_tag='object XXXXXX
 type commit
 tag S
-- 
2.6.3

Re: [PATCH] filter-branch: pass tag name via stdin without newline

[Junio C Hamano]

"Eric N. Vander Weele" <ericvw@gmail.com> writes:

> "git filter-branch --tag-name-filter" fails when the user-provided
> command attempts to trivially append text to the originally tag name,
> passed via stdin, due to an unexpected newline ('\n').  The newline is
> introduced due to "echo" piping the original tag name to the
> user-provided tag name filter command.

Is there any other place where we feed such an incomplete line
(i.e. a line without the terminating LF) to the filters in this
command?

I am guessing that the answer to that question would be no, and all
existing scripts by users expect the newline at the end of each
line.  So I would have to say that "due to an unexpected newline" is
a misleading statement--isn't it a bug in your filter that it did
not expect one?

> The only portable usage of "echo" is without any options and escape
> sequences.  Therefore, replacing "echo" with "printf" is a suitable,
> POSIX compliant alternative.

Yes, if we wanted to emit an incomplete line, we would be using

    printf "%s" "$var"

instead of

    echo -n "$var"

for portability.  But that would not belong to the log message for
this patch.  If the patch were to correct a script that originally
used "echo -n" to produce an incomplete line to instead use "printf",
these three lines would have been a perfect log message.

Thanks.

> Signed-off-by: Eric N. Vander Weele <ericvw@gmail.com>
> ---
>  git-filter-branch.sh     | 2 +-
>  t/t7003-filter-branch.sh | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 98f1779..949cd30 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -503,7 +503,7 @@ if [ "$filter_tag_name" ]; then
>  		new_sha1="$(cat "../map/$sha1")"
>  		GIT_COMMIT="$sha1"
>  		export GIT_COMMIT
> -		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
> +		new_ref="$(printf "$ref" | eval "$filter_tag_name")" ||
>  			die "tag name filter failed: $filter_tag_name"
>  
>  		echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
> index 869e0bf..0db6808 100755
> --- a/t/t7003-filter-branch.sh
> +++ b/t/t7003-filter-branch.sh
> @@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'Tag name filter does not pass tag ref with newline' '
> +	git filter-branch -f --tag-name-filter "cat && printf "_append"" -- A &&
> +	git rev-parse A_append > /dev/null 2>&1
> +'
> +
>  faux_gpg_tag='object XXXXXX
>  type commit
>  tag S

Re: [PATCH] filter-branch: pass tag name via stdin without newline

[Eric Vander Weele]

On Sun, Dec 6, 2015 at 6:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Eric N. Vander Weele" <ericvw@gmail.com> writes:
>
>> "git filter-branch --tag-name-filter" fails when the user-provided
>> command attempts to trivially append text to the originally tag name,
>> passed via stdin, due to an unexpected newline ('\n').  The newline is
>> introduced due to "echo" piping the original tag name to the
>> user-provided tag name filter command.
>
> Is there any other place where we feed such an incomplete line
> (i.e. a line without the terminating LF) to the filters in this
> command?
>
> I am guessing that the answer to that question would be no, and all
> existing scripts by users expect the newline at the end of each
> line.  So I would have to say that "due to an unexpected newline" is
> a misleading statement--isn't it a bug in your filter that it did
> not expect one?
>
I agree that there isn't any other place in "git filter-branch" where
an incomplete line is fed to the filters.

My surprise was due to asymmetry between of the filter receiving a tag
name with a newline and that the behavior of trailing newlines being
stripped from what is fed to stdout (due to behavior of command
substitution).  Given that an embedded newline within a ref name is
invalid, I thought it may be desirable suppress newline.  Actually,
now that I think about, POSIX standard defines a line as "a sequence
of zero or more non- <newline> characters plus a terminating <newline>
character."

My apologies for the misleading statement and will correct my filter
accordingly to handle the terminating newline for appending text.
>
>> The only portable usage of "echo" is without any options and escape
>> sequences.  Therefore, replacing "echo" with "printf" is a suitable,
>> POSIX compliant alternative.
>
> Yes, if we wanted to emit an incomplete line, we would be using
>
>     printf "%s" "$var"
>
> instead of
>
>     echo -n "$var"
>
> for portability.  But that would not belong to the log message for
> this patch.  If the patch were to correct a script that originally
> used "echo -n" to produce an incomplete line to instead use "printf",
> these three lines would have been a perfect log message.
>
I appreciate the feedback - thanks.  The intention of those three
lines were to give context for using "printf" instead of adding
options to "echo" to suppress the newline; however, that was under the
assumption that suppressing the newline was desired behavior.
>
> Thanks.
>
>> Signed-off-by: Eric N. Vander Weele <ericvw@gmail.com>
>> ---
>>  git-filter-branch.sh     | 2 +-
>>  t/t7003-filter-branch.sh | 5 +++++
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
>> index 98f1779..949cd30 100755
>> --- a/git-filter-branch.sh
>> +++ b/git-filter-branch.sh
>> @@ -503,7 +503,7 @@ if [ "$filter_tag_name" ]; then
>>               new_sha1="$(cat "../map/$sha1")"
>>               GIT_COMMIT="$sha1"
>>               export GIT_COMMIT
>> -             new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
>> +             new_ref="$(printf "$ref" | eval "$filter_tag_name")" ||
>>                       die "tag name filter failed: $filter_tag_name"
>>
>>               echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
>> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
>> index 869e0bf..0db6808 100755
>> --- a/t/t7003-filter-branch.sh
>> +++ b/t/t7003-filter-branch.sh
>> @@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '
>>       test_cmp expect actual
>>  '
>>
>> +test_expect_success 'Tag name filter does not pass tag ref with newline' '
>> +     git filter-branch -f --tag-name-filter "cat && printf "_append"" -- A &&
>> +     git rev-parse A_append > /dev/null 2>&1
>> +'
>> +
>>  faux_gpg_tag='object XXXXXX
>>  type commit
>>  tag S

Re: [PATCH] filter-branch: pass tag name via stdin without newline

[Jeff King]

On Sun, Dec 06, 2015 at 06:55:21PM -0800, Junio C Hamano wrote:

> "Eric N. Vander Weele" <ericvw@gmail.com> writes:
> 
> > "git filter-branch --tag-name-filter" fails when the user-provided
> > command attempts to trivially append text to the originally tag name,
> > passed via stdin, due to an unexpected newline ('\n').  The newline is
> > introduced due to "echo" piping the original tag name to the
> > user-provided tag name filter command.
> 
> Is there any other place where we feed such an incomplete line
> (i.e. a line without the terminating LF) to the filters in this
> command?

I suspect we would break filters if we did. On some systems, tools like
`sed` and `tr` often behave funny when there is no trailing newline, but
I do not recall all of the specific instances (if one felt like engaging
in some light masochism, searching the mailing list archive will
probably turn up results).

-Peff