{"thread":{"id":"40949","subject":"[PATCH] filter-branch: pass tag name via stdin without newline","startedAt":"2015-12-07T01:17:26Z","lastAt":"2015-12-07T20:44:40Z","messageCount":4,"participants":["Eric N. Vander Weele","Junio C Hamano","Eric Vander Weele","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"274089","messageId":"1449451046-19752-1-git-send-email-ericvw@gmail.com","threadId":"40949","inReplyTo":null,"subject":"[PATCH] filter-branch: pass tag name via stdin without newline","fromName":"Eric N. Vander Weele","fromEmail":"ericvw@gmail.com","sentAt":"2015-12-07T01:17:26Z","receivedAt":"2015-12-07T01:17:26Z","isPatch":true,"sender":{"key":"ericvw@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1284017?v=4"},"body":"\"git filter-branch --tag-name-filter\" fails when the user-provided\ncommand attempts to trivially append text to the originally tag name,\npassed via stdin, due to an unexpected newline ('\\n').  The newline is\nintroduced due to \"echo\" piping the original tag name to the\nuser-provided tag name filter command.\n\nThe only portable usage of \"echo\" is without any options and escape\nsequences.  Therefore, replacing \"echo\" with \"printf\" is a suitable,\nPOSIX compliant alternative.\n\nSigned-off-by: Eric N. Vander Weele <ericvw@gmail.com>\n---\n git-filter-branch.sh     | 2 +-\n t/t7003-filter-branch.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 98f1779..949cd30 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -503,7 +503,7 @@ if [ \"$filter_tag_name\" ]; then\n \t\tnew_sha1=\"$(cat \"../map/$sha1\")\"\n \t\tGIT_COMMIT=\"$sha1\"\n \t\texport GIT_COMMIT\n-\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\" ||\n+\t\tnew_ref=\"$(printf \"$ref\" | eval \"$filter_tag_name\")\" ||\n \t\t\tdie \"tag name filter failed: $filter_tag_name\"\n \n \t\techo \"$ref -> $new_ref ($sha1 -> $new_sha1)\"\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 869e0bf..0db6808 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'Tag name filter does not pass tag ref with newline' '\n+\tgit filter-branch -f --tag-name-filter \"cat && printf \"_append\"\" -- A &&\n+\tgit rev-parse A_append > /dev/null 2>&1\n+'\n+\n faux_gpg_tag='object XXXXXX\n type commit\n tag S\n-- \n2.6.3\n"},{"id":"274091","messageId":"xmqqd1ujnf1i.fsf@gitster.mtv.corp.google.com","threadId":"40949","inReplyTo":"1449451046-19752-1-git-send-email-ericvw@gmail.com","subject":"Re: [PATCH] filter-branch: pass tag name via stdin without newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-07T02:55:21Z","receivedAt":"2015-12-07T02:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric N. Vander Weele\" <ericvw@gmail.com> writes:\n\n> \"git filter-branch --tag-name-filter\" fails when the user-provided\n> command attempts to trivially append text to the originally tag name,\n> passed via stdin, due to an unexpected newline ('\\n').  The newline is\n> introduced due to \"echo\" piping the original tag name to the\n> user-provided tag name filter command.\n\nIs there any other place where we feed such an incomplete line\n(i.e. a line without the terminating LF) to the filters in this\ncommand?\n\nI am guessing that the answer to that question would be no, and all\nexisting scripts by users expect the newline at the end of each\nline.  So I would have to say that \"due to an unexpected newline\" is\na misleading statement--isn't it a bug in your filter that it did\nnot expect one?\n\n> The only portable usage of \"echo\" is without any options and escape\n> sequences.  Therefore, replacing \"echo\" with \"printf\" is a suitable,\n> POSIX compliant alternative.\n\nYes, if we wanted to emit an incomplete line, we would be using\n\n    printf \"%s\" \"$var\"\n\ninstead of\n\n    echo -n \"$var\"\n\nfor portability.  But that would not belong to the log message for\nthis patch.  If the patch were to correct a script that originally\nused \"echo -n\" to produce an incomplete line to instead use \"printf\",\nthese three lines would have been a perfect log message.\n\nThanks.\n\n> Signed-off-by: Eric N. Vander Weele <ericvw@gmail.com>\n> ---\n>  git-filter-branch.sh     | 2 +-\n>  t/t7003-filter-branch.sh | 5 +++++\n>  2 files changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n> index 98f1779..949cd30 100755\n> --- a/git-filter-branch.sh\n> +++ b/git-filter-branch.sh\n> @@ -503,7 +503,7 @@ if [ \"$filter_tag_name\" ]; then\n>  \t\tnew_sha1=\"$(cat \"../map/$sha1\")\"\n>  \t\tGIT_COMMIT=\"$sha1\"\n>  \t\texport GIT_COMMIT\n> -\t\tnew_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\" ||\n> +\t\tnew_ref=\"$(printf \"$ref\" | eval \"$filter_tag_name\")\" ||\n>  \t\t\tdie \"tag name filter failed: $filter_tag_name\"\n>  \n>  \t\techo \"$ref -> $new_ref ($sha1 -> $new_sha1)\"\n> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\n> index 869e0bf..0db6808 100755\n> --- a/t/t7003-filter-branch.sh\n> +++ b/t/t7003-filter-branch.sh\n> @@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'Tag name filter does not pass tag ref with newline' '\n> +\tgit filter-branch -f --tag-name-filter \"cat && printf \"_append\"\" -- A &&\n> +\tgit rev-parse A_append > /dev/null 2>&1\n> +'\n> +\n>  faux_gpg_tag='object XXXXXX\n>  type commit\n>  tag S\n"},{"id":"274096","messageId":"CAGhVqYv5ZwDbj3NK=34vUx136JfyyDvrVr0fWF=vaiHZ8gQhtg@mail.gmail.com","threadId":"40949","inReplyTo":"xmqqd1ujnf1i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] filter-branch: pass tag name via stdin without newline","fromName":"Eric Vander Weele","fromEmail":"ericvw@gmail.com","sentAt":"2015-12-07T05:15:33Z","receivedAt":"2015-12-07T05:15:33Z","isPatch":true,"sender":{"key":"ericvw@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1284017?v=4"},"body":"On Sun, Dec 6, 2015 at 6:55 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Eric N. Vander Weele\" <ericvw@gmail.com> writes:\n>\n>> \"git filter-branch --tag-name-filter\" fails when the user-provided\n>> command attempts to trivially append text to the originally tag name,\n>> passed via stdin, due to an unexpected newline ('\\n').  The newline is\n>> introduced due to \"echo\" piping the original tag name to the\n>> user-provided tag name filter command.\n>\n> Is there any other place where we feed such an incomplete line\n> (i.e. a line without the terminating LF) to the filters in this\n> command?\n>\n> I am guessing that the answer to that question would be no, and all\n> existing scripts by users expect the newline at the end of each\n> line.  So I would have to say that \"due to an unexpected newline\" is\n> a misleading statement--isn't it a bug in your filter that it did\n> not expect one?\n>\nI agree that there isn't any other place in \"git filter-branch\" where\nan incomplete line is fed to the filters.\n\nMy surprise was due to asymmetry between of the filter receiving a tag\nname with a newline and that the behavior of trailing newlines being\nstripped from what is fed to stdout (due to behavior of command\nsubstitution).  Given that an embedded newline within a ref name is\ninvalid, I thought it may be desirable suppress newline.  Actually,\nnow that I think about, POSIX standard defines a line as \"a sequence\nof zero or more non- <newline> characters plus a terminating <newline>\ncharacter.\"\n\nMy apologies for the misleading statement and will correct my filter\naccordingly to handle the terminating newline for appending text.\n>\n>> The only portable usage of \"echo\" is without any options and escape\n>> sequences.  Therefore, replacing \"echo\" with \"printf\" is a suitable,\n>> POSIX compliant alternative.\n>\n> Yes, if we wanted to emit an incomplete line, we would be using\n>\n>     printf \"%s\" \"$var\"\n>\n> instead of\n>\n>     echo -n \"$var\"\n>\n> for portability.  But that would not belong to the log message for\n> this patch.  If the patch were to correct a script that originally\n> used \"echo -n\" to produce an incomplete line to instead use \"printf\",\n> these three lines would have been a perfect log message.\n>\nI appreciate the feedback - thanks.  The intention of those three\nlines were to give context for using \"printf\" instead of adding\noptions to \"echo\" to suppress the newline; however, that was under the\nassumption that suppressing the newline was desired behavior.\n>\n> Thanks.\n>\n>> Signed-off-by: Eric N. Vander Weele <ericvw@gmail.com>\n>> ---\n>>  git-filter-branch.sh     | 2 +-\n>>  t/t7003-filter-branch.sh | 5 +++++\n>>  2 files changed, 6 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-filter-branch.sh b/git-filter-branch.sh\n>> index 98f1779..949cd30 100755\n>> --- a/git-filter-branch.sh\n>> +++ b/git-filter-branch.sh\n>> @@ -503,7 +503,7 @@ if [ \"$filter_tag_name\" ]; then\n>>               new_sha1=\"$(cat \"../map/$sha1\")\"\n>>               GIT_COMMIT=\"$sha1\"\n>>               export GIT_COMMIT\n>> -             new_ref=\"$(echo \"$ref\" | eval \"$filter_tag_name\")\" ||\n>> +             new_ref=\"$(printf \"$ref\" | eval \"$filter_tag_name\")\" ||\n>>                       die \"tag name filter failed: $filter_tag_name\"\n>>\n>>               echo \"$ref -> $new_ref ($sha1 -> $new_sha1)\"\n>> diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\n>> index 869e0bf..0db6808 100755\n>> --- a/t/t7003-filter-branch.sh\n>> +++ b/t/t7003-filter-branch.sh\n>> @@ -269,6 +269,11 @@ test_expect_success 'Tag name filtering retains tag message' '\n>>       test_cmp expect actual\n>>  '\n>>\n>> +test_expect_success 'Tag name filter does not pass tag ref with newline' '\n>> +     git filter-branch -f --tag-name-filter \"cat && printf \"_append\"\" -- A &&\n>> +     git rev-parse A_append > /dev/null 2>&1\n>> +'\n>> +\n>>  faux_gpg_tag='object XXXXXX\n>>  type commit\n>>  tag S\n"},{"id":"274136","messageId":"20151207204440.GC30203@sigill.intra.peff.net","threadId":"40949","inReplyTo":"xmqqd1ujnf1i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] filter-branch: pass tag name via stdin without newline","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-07T20:44:40Z","receivedAt":"2015-12-07T20:44:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 06, 2015 at 06:55:21PM -0800, Junio C Hamano wrote:\n\n> \"Eric N. Vander Weele\" <ericvw@gmail.com> writes:\n> \n> > \"git filter-branch --tag-name-filter\" fails when the user-provided\n> > command attempts to trivially append text to the originally tag name,\n> > passed via stdin, due to an unexpected newline ('\\n').  The newline is\n> > introduced due to \"echo\" piping the original tag name to the\n> > user-provided tag name filter command.\n> \n> Is there any other place where we feed such an incomplete line\n> (i.e. a line without the terminating LF) to the filters in this\n> command?\n\nI suspect we would break filters if we did. On some systems, tools like\n`sed` and `tr` often behave funny when there is no trailing newline, but\nI do not recall all of the specific instances (if one felt like engaging\nin some light masochism, searching the mailing list archive will\nprobably turn up results).\n\n-Peff\n"}]}