Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly

2013-10-28 Thread Paolo Giarrusso
(Resending without HTML, so that it reaches the ML).

On Fri, Oct 11, 2013 at 11:32 AM, Paolo Giarrusso p.giarru...@gmail.com wrote:

 On Wed, Oct 9, 2013 at 11:11 PM, Jonathan Nieder jrnie...@gmail.com 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

2013-10-09 Thread Johannes Sixt
Am 10/9/2013 12:32, schrieb Paolo Giarrusso:
 On Wed, Oct 9, 2013 at 12:20 PM, Tay Ray Chuan rcta...@gmail.com wrote:
 On Wed, Oct 9, 2013 at 11:57 AM, Paolo G. Giarrusso
 p.giarru...@gmail.com 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


Re: Fwd: [PATCH] git-subtree: Avoid using echo -n even indirectly

2013-10-09 Thread Matthieu Moy
Paolo Giarrusso p.giarru...@gmail.com 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

2013-10-09 Thread Paolo Giarrusso
On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Paolo Giarrusso p.giarru...@gmail.com 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

2013-10-09 Thread Jeff King
On Wed, Oct 09, 2013 at 02:03:24PM +0200, Paolo Giarrusso wrote:

 On Wed, Oct 9, 2013 at 1:26 PM, Matthieu Moy
 matthieu@grenoble-inp.fr wrote:
  Paolo Giarrusso p.giarru...@gmail.com 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