Re: [PATCH] filter-branch: pass tag name via stdin without newline
On Sun, Dec 6, 2015 at 6:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> "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.
Show 18 quoted lines
>
>> 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.
Show 38 quoted lines
>
> 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
Eric N. Vander Weele
