Re: [PATCH] filter-branch: pass tag name via stdin without newline
- From
Eric Vander Weele <ericvw@gmail.com>
- Date
- Dec 7, 2015, 05:15 UTC
- Message-ID
- <CAGhVqYv5ZwDbj3NK=34vUx136JfyyDvrVr0fWF=vaiHZ8gQhtg@mail.gmail.com>
- In-Reply-To
- <xmqqd1ujnf1i.fsf@gitster.mtv.corp.google.com>
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