Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
(Resending without HTML, so that it reaches the ML).
On Fri, Oct 11, 2013 at 11:32 AM, Paolo Giarrusso wrote:
>
> On Wed, Oct 9, 2013 at 11:11 PM, Jonathan Nieder wrote:
> > Paolo Giarrusso wrote:
> >
> >> Seeing the email, I wonder whether there's hope something like that
> >> can be preserved in an email, and whether the code should use some
> >> escape sequence instead.
> >
> > Yes, please. Mind if I amend it to
> >
> > printf "%s\r" "$revcount/$revmax ($createcount)" >&2
> >
> > ?
>
> Please do go ahead, by all means (arguably as a different commit, but
> those are minor details).
What happened? Did you go ahead, as you wrote? Is the patch somewhere?
Arguably it should go into the maint branch, but I think it didn't —
otherwise https://github.com/git/git/pull/61 should have stopped being
mergeable.
This also makes me wonder whether you use any tracker at all — but
unless there is one that I missed, that's a separate discussion.
> > [...]
> >>> say()
> >>> {
> >>> if [ -z "$quiet" ]; then
> >>> echo "$@" >&2
> >>>fi
> >>> }
> >
> > I agree with the other reviewers that this should be fixed to use
> > printf, too, but that's another topic.
> Seconded.
--
Paolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg
http://www.informatik.uni-marburg.de/~pgiarrusso/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
On Wed, Oct 9, 2013 at 11:11 PM, Jonathan Nieder wrote:
> Paolo Giarrusso wrote:
>
>> Seeing the email, I wonder whether there's hope something like that
>> can be preserved in an email, and whether the code should use some
>> escape sequence instead.
>
> Yes, please. Mind if I amend it to
>
> printf "%s\r" "$revcount/$revmax ($createcount)" >&2
>
> ?
Please do go ahead, by all means (arguably as a different commit, but
those are minor details).
> [...]
>>> say()
>>> {
>>> if [ -z "$quiet" ]; then
>>> echo "$@" >&2
>>>fi
>>> }
>
> I agree with the other reviewers that this should be fixed to use
> printf, too, but that's another topic.
Seconded.
--
Paolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg
http://www.informatik.uni-marburg.de/~pgiarrusso/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
Paolo Giarrusso wrote:
> Seeing the email, I wonder whether there's hope something like that
> can be preserved in an email, and whether the code should use some
> escape sequence instead.
Yes, please. Mind if I amend it to
printf "%s\r" "$revcount/$revmax ($createcount)" >&2
?
[...]
>> say()
>> {
>> if [ -z "$quiet" ]; then
>> echo "$@" >&2
>>fi
>> }
I agree with the other reviewers that this should be fixed to use
printf, too, but that's another topic.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
On Wed, Oct 09, 2013 at 02:03:24PM +0200, Paolo Giarrusso wrote: > On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy > wrote: > > Paolo Giarrusso writes: > > > >> Otherwise, one could > >> change say to use printf, but that's more invasive. > > > > "invasive" in the sense that it impacts indirectly more callers, but are > > there really cases where "echo" is needed when calling "say"? Aren't > > there other potential bugs when arbitrary strings are passed to "say", > > that would be fixed by using printf once and for all? > > (1) Changing the implementation of say to use printf "%s\n" would be > trivial, and I think would address your concerns. Yeah, I think we should do that. I had the same thought as Matthieu when I read your initial patch; there are real portability bugs caused by using "echo" that would be fixed. However, that is somewhat orthogonal to the bug you are fixing. For handling this one site, I think it would be fine to just convert it to use printf, as your patch did. As you noted, the alternatives: > (2) add an explicit \n to all callers (invasive & error prone), or > (3) make `say` parse the `-n` option and conditionally add "\n" to the > format string or to a final argument, if -n is not specified; this > would affect no current caller, but complicate the implementation of > say. Doing that for just one call site has too much potential for > breakage, so I'm not sure I'd do it. (I'm not even sure on what should > `say` do when `-n` is not the first argument). ...are either annoying or complicated (and in particular, parsing "-n" means that callers need to be aware that 'say "$foo"' might accidentally trigger "-n" if $foo comes from the user). So the sanest interface is probably "say_nonl" or something similar. But since there would only be one caller, I don't see much point in factoring it out. > Options (1), (2) and (3) are mutually alternative; my favorite is (1). Me too. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy wrote: > Paolo Giarrusso writes: > >> Otherwise, one could >> change say to use printf, but that's more invasive. > > "invasive" in the sense that it impacts indirectly more callers, but are > there really cases where "echo" is needed when calling "say"? Aren't > there other potential bugs when arbitrary strings are passed to "say", > that would be fixed by using printf once and for all? (1) Changing the implementation of say to use printf "%s\n" would be trivial, and I think would address your concerns. But I was concerned about code duplication; one could additionally make say reusable in this single call site, instead of inlining and customizing it by replacing the "\n" with "\r". But for that, you need to either (2) add an explicit \n to all callers (invasive & error prone), or (3) make `say` parse the `-n` option and conditionally add "\n" to the format string or to a final argument, if -n is not specified; this would affect no current caller, but complicate the implementation of say. Doing that for just one call site has too much potential for breakage, so I'm not sure I'd do it. (I'm not even sure on what should `say` do when `-n` is not the first argument). Options (1), (2) and (3) are mutually alternative; my favorite is (1). I can see your points about opportunity, especially after looking at the commit message of the patch of yours you linked. > The patch would look like the one I did in 89b0230a20 (Wed Aug 7 2013, > die_with_status: use "printf '%s\n'", not "echo"). I see your point. But note that using printf like in die_with_status after that commit wouldn't be reusable here in all call sites, because it always prints a newline. Cheers, -- Paolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg http://www.informatik.uni-marburg.de/~pgiarrusso/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
Paolo Giarrusso writes: > Otherwise, one could > change say to use printf, but that's more invasive. "invasive" in the sense that it impacts indirectly more callers, but are there really cases where "echo" is needed when calling "say"? Aren't there other potential bugs when arbitrary strings are passed to "say", that would be fixed by using printf once and for all? The patch would look like the one I did in 89b0230a20 (Wed Aug 7 2013, die_with_status: use "printf '%s\n'", not "echo"). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
Am 10/9/2013 12:32, schrieb Paolo Giarrusso: > On Wed, Oct 9, 2013 at 12:20 PM, Tay Ray Chuan wrote: >> On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso >> wrote: >>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh >>> index 7d7af03..ebfb78f 100755 >>> --- a/contrib/subtree/git-subtree.sh >>> +++ b/contrib/subtree/git-subtree.sh >>> @@ -592,7 +592,9 @@ cmd_split() >>> eval "$grl" | >>> while read rev parents; do >>> revcount=$(($revcount + 1)) >>> - say -n "$revcount/$revmax ($createcount) >>> " >>> + if [ -z "$quiet" ]; then >>> + printf "%s" "$revcount/$revmax ($createcount) >>> " >&2 > > An additional note for reviewers and appliers: the original and the > patched codeboth embed a literal ^M, >... > whether the code should use some > escape sequence instead. As you are using printf, you can easily do: printf '%s\r' "..." without using ^M. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly
(Resending without HTML).
On Wed, Oct 9, 2013 at 12:20 PM, Tay Ray Chuan wrote:
>
> On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso
> wrote:
> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> > index 7d7af03..ebfb78f 100755
> > --- a/contrib/subtree/git-subtree.sh
> > +++ b/contrib/subtree/git-subtree.sh
> > @@ -592,7 +592,9 @@ cmd_split()
> > eval "$grl" |
> > while read rev parents; do
> > revcount=$(($revcount + 1))
> > - say -n "$revcount/$revmax ($createcount)
> > "
> > + if [ -z "$quiet" ]; then
> > + printf "%s" "$revcount/$revmax ($createcount)
> > " >&2
An additional note for reviewers and appliers: the original and the
patched codeboth embed a literal ^M, not a new line, go to back to the
beginning of the line and overwrite it, so the above is not a
consequence of line-wrap.
I used git-format-patch and git-send-email, and the ^M is visible in
Vim in the exported patch (that's why I didn't remark on it).
Seeing the email, I wonder whether there's hope something like that
can be preserved in an email, and whether the code should use some
escape sequence instead.
> > + fi
>
> Reviewers might wish to know that "say" in git-subtree is defined as
>
> say()
> {
> if [ -z "$quiet" ]; then
> echo "$@" >&2
>fi
> }
>
> Hence the "if" and the redirect.
Indeed. I considered having a variant of `say` instead of inlining and
customizing it, but for once I decided to keep this simple, since this
variant of `say` is currently used only once. Otherwise, one could
change say to use printf, but that's more invasive.
Cheers,
--
Paolo G. Giarrusso - Ph.D. Student, Philipps-University Marburg
http://www.informatik.uni-marburg.de/~pgiarrusso/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-subtree: Avoid using echo -n even indirectly
On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso
wrote:
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 7d7af03..ebfb78f 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -592,7 +592,9 @@ cmd_split()
> eval "$grl" |
> while read rev parents; do
> revcount=$(($revcount + 1))
> - say -n "$revcount/$revmax ($createcount)
> "
> + if [ -z "$quiet" ]; then
> + printf "%s" "$revcount/$revmax ($createcount)
> " >&2
> + fi
Reviewers might wish to know that "say" in git-subtree is defined as
say()
{
if [ -z "$quiet" ]; then
echo "$@" >&2
fi
}
Hence the "if" and the redirect.
--
Cheers,
Ray Chuan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-subtree: Avoid using echo -n even indirectly
Since say uses echo, this uses echo -n, which is not portable - see 19c3c5fdcb35b66b792534c5dc4e8d87a3952d2a. Without this commit, the output looks like: ... -n 1891/1936 (1883) -n 1892/1936 (1884) -n 1893/1936 (1885) ... Signed-off-by: Paolo G. Giarrusso --- Please CC me on replies, as I am not subscribed to this mailing list. I am tracking this submission via https://github.com/git/git/pull/61, which I'll duly close myself when the discussion is resolved. contrib/subtree/git-subtree.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 7d7af03..ebfb78f 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -592,7 +592,9 @@ cmd_split() eval "$grl" | while read rev parents; do revcount=$(($revcount + 1)) - say -n "$revcount/$revmax ($createcount) " + if [ -z "$quiet" ]; then + printf "%s" "$revcount/$revmax ($createcount) " >&2 + fi debug "Processing commit: $rev" exists=$(cache_get $rev) if [ -n "$exists" ]; then -- 1.8.4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
