{"thread":{"id":"35088","subject":"[PATCH] git-subtree: Avoid using echo -n even indirectly","startedAt":"2013-10-09T03:57:25Z","lastAt":"2013-10-11T09:32:11Z","messageCount":9,"participants":["Paolo G. Giarrusso","Tay Ray Chuan","Paolo Giarrusso","Johannes Sixt","Matthieu Moy","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228635","messageId":"1381291045-98372-1-git-send-email-p.giarrusso@gmail.com","threadId":"35088","inReplyTo":null,"subject":"[PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Paolo G. Giarrusso","fromEmail":"p.giarrusso@gmail.com","sentAt":"2013-10-09T03:57:25Z","receivedAt":"2013-10-09T03:57:25Z","isPatch":true,"sender":{"key":"p.giarrusso@gmail.com","avatar":"https://gravatar.com/avatar/323ea8c76793d9193ad88ccff30f44d2ee0a51c63abe58d950b6454859e09bb6?d=mp&s=160"},"body":"Since say uses echo, this uses echo -n, which is not portable - see\n19c3c5fdcb35b66b792534c5dc4e8d87a3952d2a.\n\nWithout this commit, the output looks like:\n\n...\n-n 1891/    1936 (1883)\n-n 1892/    1936 (1884)\n-n 1893/    1936 (1885)\n...\n\nSigned-off-by: Paolo G. Giarrusso <p.giarrusso@gmail.com>\n---\n\nPlease CC me on replies, as I am not subscribed to this mailing list.\nI am tracking this submission via https://github.com/git/git/pull/61, which I'll\nduly close myself when the discussion is resolved.\n\n contrib/subtree/git-subtree.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7d7af03..ebfb78f 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -592,7 +592,9 @@ cmd_split()\n \teval \"$grl\" |\n \twhile read rev parents; do\n \t\trevcount=$(($revcount + 1))\n-\t\tsay -n \"$revcount/$revmax ($createcount)\n\"\n+\t\tif [ -z \"$quiet\" ]; then\n+\t\t\tprintf \"%s\" \"$revcount/$revmax ($createcount)\n\" >&2\n+\t\tfi\n \t\tdebug \"Processing commit: $rev\"\n \t\texists=$(cache_get $rev)\n \t\tif [ -n \"$exists\" ]; then\n-- \n1.8.4\n"},{"id":"228637","messageId":"CALUzUxovH5MTP1_DMigRx1Hp+X6v3_YdLK2vSG4KwRLNSDHN-w@mail.gmail.com","threadId":"35088","inReplyTo":"1381291045-98372-1-git-send-email-p.giarrusso@gmail.com","subject":"Re: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2013-10-09T10:20:40Z","receivedAt":"2013-10-09T10:20:40Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso\n<p.giarrusso@gmail.com> wrote:\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 7d7af03..ebfb78f 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -592,7 +592,9 @@ cmd_split()\n>         eval \"$grl\" |\n>         while read rev parents; do\n>                 revcount=$(($revcount + 1))\n> -               say -n \"$revcount/$revmax ($createcount)\n> \"\n> +               if [ -z \"$quiet\" ]; then\n> +                       printf \"%s\" \"$revcount/$revmax ($createcount)\n> \" >&2\n> +               fi\n\nReviewers might wish to know that \"say\" in git-subtree is defined as\n\n        say()\n        {\n                if [ -z \"$quiet\" ]; then\n                        echo \"$@\" >&2\n               fi\n        }\n\nHence the \"if\" and the redirect.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"228639","messageId":"CAAcnjCQ+c=8wYZkqS7VzZ_bDDDkSwdQ3qdz77osrSpKDS9ON7Q@mail.gmail.com","threadId":"35088","inReplyTo":"CAAcnjCT1bdR+9kDW=q_326OhiSMm3_j-yOh0-ayTkObK3bZ3bQ@mail.gmail.com","subject":"Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Paolo Giarrusso","fromEmail":"p.giarrusso@gmail.com","sentAt":"2013-10-09T10:32:31Z","receivedAt":"2013-10-09T10:32:31Z","isPatch":true,"sender":{"key":"p.giarrusso@gmail.com","avatar":"https://gravatar.com/avatar/323ea8c76793d9193ad88ccff30f44d2ee0a51c63abe58d950b6454859e09bb6?d=mp&s=160"},"body":"(Resending without HTML).\n\nOn Wed, Oct 9, 2013 at 12:20 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:\n>\n> On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso\n> <p.giarrusso@gmail.com> wrote:\n> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> > index 7d7af03..ebfb78f 100755\n> > --- a/contrib/subtree/git-subtree.sh\n> > +++ b/contrib/subtree/git-subtree.sh\n> > @@ -592,7 +592,9 @@ cmd_split()\n> >         eval \"$grl\" |\n> >         while read rev parents; do\n> >                 revcount=$(($revcount + 1))\n> > -               say -n \"$revcount/$revmax ($createcount)\n> > \"\n> > +               if [ -z \"$quiet\" ]; then\n> > +                       printf \"%s\" \"$revcount/$revmax ($createcount)\n> > \" >&2\n\nAn additional note for reviewers and appliers: the original and the\npatched codeboth embed a literal ^M, not a new line, go to back to the\nbeginning of the line and overwrite it, so the above is not a\nconsequence of line-wrap.\n\nI used git-format-patch and git-send-email, and the ^M is visible in\nVim in the exported patch (that's why I didn't remark on it).\nSeeing the email, I wonder whether there's hope something like that\ncan be preserved in an email, and whether the code should use some\nescape sequence instead.\n\n> > +               fi\n>\n> Reviewers might wish to know that \"say\" in git-subtree is defined as\n>\n>         say()\n>         {\n>                 if [ -z \"$quiet\" ]; then\n>                         echo \"$@\" >&2\n>                fi\n>         }\n>\n> Hence the \"if\" and the redirect.\n\nIndeed. I considered having a variant of `say` instead of inlining and\ncustomizing it, but for once I decided to keep this simple, since this\nvariant of `say` is currently used only once. Otherwise, one could\nchange say to use printf, but that's more invasive.\n\nCheers,\n-- \nPaolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg\nhttp://www.informatik.uni-marburg.de/~pgiarrusso/\n"},{"id":"228640","messageId":"52553417.2090903@viscovery.net","threadId":"35088","inReplyTo":"CAAcnjCQ+c=8wYZkqS7VzZ_bDDDkSwdQ3qdz77osrSpKDS9ON7Q@mail.gmail.com","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-10-09T10:46:47Z","receivedAt":"2013-10-09T10:46:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/9/2013 12:32, schrieb Paolo Giarrusso:\n> On Wed, Oct 9, 2013 at 12:20 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:\n>> On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso\n>> <p.giarrusso@gmail.com> wrote:\n>>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>>> index 7d7af03..ebfb78f 100755\n>>> --- a/contrib/subtree/git-subtree.sh\n>>> +++ b/contrib/subtree/git-subtree.sh\n>>> @@ -592,7 +592,9 @@ cmd_split()\n>>>         eval \"$grl\" |\n>>>         while read rev parents; do\n>>>                 revcount=$(($revcount + 1))\n>>> -               say -n \"$revcount/$revmax ($createcount)\n>>> \"\n>>> +               if [ -z \"$quiet\" ]; then\n>>> +                       printf \"%s\" \"$revcount/$revmax ($createcount)\n>>> \" >&2\n> \n> An additional note for reviewers and appliers: the original and the\n> patched codeboth embed a literal ^M,\n>...\n> whether the code should use some\n> escape sequence instead.\n\nAs you are using printf, you can easily do:\n\n\t\t\tprintf '%s\\r' \"...\"\n\nwithout using ^M.\n\n-- Hannes\n"},{"id":"228641","messageId":"vpqy562y9b8.fsf@anie.imag.fr","threadId":"35088","inReplyTo":"CAAcnjCQ+c=8wYZkqS7VzZ_bDDDkSwdQ3qdz77osrSpKDS9ON7Q@mail.gmail.com","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-09T11:26:19Z","receivedAt":"2013-10-09T11:26:19Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Paolo Giarrusso <p.giarrusso@gmail.com> writes:\n\n> Otherwise, one could\n> change say to use printf, but that's more invasive.\n\n\"invasive\" in the sense that it impacts indirectly more callers, but are\nthere really cases where \"echo\" is needed when calling \"say\"? Aren't\nthere other potential bugs when arbitrary strings are passed to \"say\",\nthat would be fixed by using printf once and for all?\n\nThe patch would look like the one I did in 89b0230a20 (Wed Aug 7 2013,\ndie_with_status: use \"printf '%s\\n'\", not \"echo\").\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228642","messageId":"CAAcnjCQCJYbYUkTK+ZM6xFe=u1mj9iHetaG--yg3Qzn0_Ty0hg@mail.gmail.com","threadId":"35088","inReplyTo":"vpqy562y9b8.fsf@anie.imag.fr","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Paolo Giarrusso","fromEmail":"p.giarrusso@gmail.com","sentAt":"2013-10-09T12:03:24Z","receivedAt":"2013-10-09T12:03:24Z","isPatch":true,"sender":{"key":"p.giarrusso@gmail.com","avatar":"https://gravatar.com/avatar/323ea8c76793d9193ad88ccff30f44d2ee0a51c63abe58d950b6454859e09bb6?d=mp&s=160"},"body":"On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Paolo Giarrusso <p.giarrusso@gmail.com> writes:\n>\n>> Otherwise, one could\n>> change say to use printf, but that's more invasive.\n>\n> \"invasive\" in the sense that it impacts indirectly more callers, but are\n> there really cases where \"echo\" is needed when calling \"say\"? Aren't\n> there other potential bugs when arbitrary strings are passed to \"say\",\n> that would be fixed by using printf once and for all?\n\n(1) Changing the implementation of say to use printf \"%s\\n\" would be\ntrivial, and I think would address your concerns.\n\nBut I was concerned about code duplication; one could additionally\nmake say reusable in this single call site, instead of inlining and\ncustomizing it by replacing the \"\\n\" with \"\\r\". But for that, you need\nto either\n(2) add an explicit \\n to all callers (invasive & error prone), or\n(3) make `say` parse the `-n` option and conditionally add \"\\n\" to the\nformat string or to a final argument, if -n is not specified; this\nwould affect no current caller, but complicate the implementation of\nsay. Doing that for just one call site has too much potential for\nbreakage, so I'm not sure I'd do it. (I'm not even sure on what should\n`say` do when `-n` is not the first argument).\n\nOptions (1), (2) and (3) are mutually alternative; my favorite is (1).\n\nI can see your points about opportunity, especially after looking at\nthe commit message of the patch of yours you linked.\n\n> The patch would look like the one I did in 89b0230a20 (Wed Aug 7 2013,\n> die_with_status: use \"printf '%s\\n'\", not \"echo\").\n\nI see your point. But note that using printf like in die_with_status\nafter that commit wouldn't be reusable here in all call sites, because\nit always prints a newline.\n\nCheers,\n-- \nPaolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg\nhttp://www.informatik.uni-marburg.de/~pgiarrusso/\n"},{"id":"228673","messageId":"20131009194820.GB3767@sigill.intra.peff.net","threadId":"35088","inReplyTo":"CAAcnjCQCJYbYUkTK+ZM6xFe=u1mj9iHetaG--yg3Qzn0_Ty0hg@mail.gmail.com","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-09T19:48:20Z","receivedAt":"2013-10-09T19:48:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 09, 2013 at 02:03:24PM +0200, Paolo Giarrusso wrote:\n\n> On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy\n> <Matthieu.Moy@grenoble-inp.fr> wrote:\n> > Paolo Giarrusso <p.giarrusso@gmail.com> writes:\n> >\n> >> Otherwise, one could\n> >> change say to use printf, but that's more invasive.\n> >\n> > \"invasive\" in the sense that it impacts indirectly more callers, but are\n> > there really cases where \"echo\" is needed when calling \"say\"? Aren't\n> > there other potential bugs when arbitrary strings are passed to \"say\",\n> > that would be fixed by using printf once and for all?\n> \n> (1) Changing the implementation of say to use printf \"%s\\n\" would be\n> trivial, and I think would address your concerns.\n\nYeah, I think we should do that. I had the same thought as Matthieu when\nI read your initial patch; there are real portability bugs caused by\nusing \"echo\" that would be fixed.\n\nHowever, that is somewhat orthogonal to the bug you are fixing. For\nhandling this one site, I think it would be fine to just convert it to\nuse printf, as your patch did. As you noted, the alternatives:\n\n> (2) add an explicit \\n to all callers (invasive & error prone), or\n> (3) make `say` parse the `-n` option and conditionally add \"\\n\" to the\n> format string or to a final argument, if -n is not specified; this\n> would affect no current caller, but complicate the implementation of\n> say. Doing that for just one call site has too much potential for\n> breakage, so I'm not sure I'd do it. (I'm not even sure on what should\n> `say` do when `-n` is not the first argument).\n\n...are either annoying or complicated (and in particular, parsing \"-n\"\nmeans that callers need to be aware that 'say \"$foo\"' might accidentally\ntrigger \"-n\" if $foo comes from the user). So the sanest interface is\nprobably \"say_nonl\" or something similar. But since there would only be\none caller, I don't see much point in factoring it out.\n\n> Options (1), (2) and (3) are mutually alternative; my favorite is (1).\n\nMe too. :)\n\n-Peff\n"},{"id":"228674","messageId":"20131009211101.GM9464@google.com","threadId":"35088","inReplyTo":"CAAcnjCQ+c=8wYZkqS7VzZ_bDDDkSwdQ3qdz77osrSpKDS9ON7Q@mail.gmail.com","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-09T21:11:01Z","receivedAt":"2013-10-09T21:11:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Paolo Giarrusso wrote:\n\n> Seeing the email, I wonder whether there's hope something like that\n> can be preserved in an email, and whether the code should use some\n> escape sequence instead.\n\nYes, please.  Mind if I amend it to\n\n\tprintf \"%s\\r\" \"$revcount/$revmax ($createcount)\" >&2\n\n?\n\n[...]\n>>         say()\n>>         {\n>>                 if [ -z \"$quiet\" ]; then\n>>                         echo \"$@\" >&2\n>>                fi\n>>         }\n\nI agree with the other reviewers that this should be fixed to use\nprintf, too, but that's another topic.\n\nThanks,\nJonathan\n"},{"id":"228728","messageId":"CAAcnjCRVUDYvBqB++AjTdY+9gBLECLsV+bbU9z5JJPS5=tG1rA@mail.gmail.com","threadId":"35088","inReplyTo":"20131009211101.GM9464@google.com","subject":"Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly","fromName":"Paolo Giarrusso","fromEmail":"p.giarrusso@gmail.com","sentAt":"2013-10-11T09:32:11Z","receivedAt":"2013-10-11T09:32:11Z","isPatch":true,"sender":{"key":"p.giarrusso@gmail.com","avatar":"https://gravatar.com/avatar/323ea8c76793d9193ad88ccff30f44d2ee0a51c63abe58d950b6454859e09bb6?d=mp&s=160"},"body":"On Wed, Oct 9, 2013 at 11:11 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Paolo Giarrusso wrote:\n>\n>> Seeing the email, I wonder whether there's hope something like that\n>> can be preserved in an email, and whether the code should use some\n>> escape sequence instead.\n>\n> Yes, please.  Mind if I amend it to\n>\n>         printf \"%s\\r\" \"$revcount/$revmax ($createcount)\" >&2\n>\n> ?\n\nPlease do go ahead, by all means (arguably as a different commit, but\nthose are minor details).\n\n> [...]\n>>>         say()\n>>>         {\n>>>                 if [ -z \"$quiet\" ]; then\n>>>                         echo \"$@\" >&2\n>>>                fi\n>>>         }\n>\n> I agree with the other reviewers that this should be fixed to use\n> printf, too, but that's another topic.\nSeconded.\n\n-- \nPaolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg\nhttp://www.informatik.uni-marburg.de/~pgiarrusso/\n"}]}