Re: [PATCH 1/2] completion: quote arguments of test and [

[Felipe Contreras <felipe.contreras@gmail.com>]

Junio C Hamano wrote:
> Koichi Murase <myoga.murase@gmail.com> writes:
> 
> > The raw command substitutions $v in the arguments of the test command
> > and the [ command are subject to word splitting and pathname
> > expansions. Even when it is ensured that the variable is not empty and
> > does not contain whitespaces and glob characters, it can fail when IFS
> > is set to non-trivial values containing letters and digits.
> 
> The above sounded good before I looked at the patch, and it still is
> good in theory, but it start to look mostly academic especially with
> enclosing $# inside a pair of double-quotes, and the variable would
> have only digits.  The same for $i and $j that appear in the loop
> control "for ((i=0, j=0; ...)); do".  The story is pretty much the
> same for local variables we set outselves to signal our findings,
> like $pcmode that is only set to either 'yes' or 'no'.

I do have the same opinion.

Although the result seems more proper, I fail to see the actual value of doing
all these changes everywhere.

On the other hand I do see the very real harm that they would break the
git-completion branch everywhere. Rebasing those 50-so patches would not be
very pleasant.

> In other words, this patch looks way too noisy to be reviewed to
> discover its real worth.

Agreed.

-- 
Felipe Contreras