Re: [PATCH] t2400: Fix test failures when using grep 2.5

On 23/07/15 06:08PM, Junio C Hamano wrote:

Jacob Abel [off-list ref] writes:

quoted

quoted

quoted

@@ -998,8 +998,8 @@ test_dwim_orphan () {
  					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&

I'm a bit confused by the --sq here - why does it need to be shell
quoted when it is always used inside double quotes?

To be honest I can't remember if this specifically needs to be in
quotes or not however I had a lot of trouble during the development of
that patchset with things escaping quotes and causing breakages in the
tests so if it isn't currently harmful I'd personally prefer to leave
it as is.

Quoting is sometimes tricky enough that "this happens to work for me
but I do not know why it works" is asking for trouble in somebody
else's environment.  If the form in the patch is correct, but tricky
for others to understand, you'd need to pick it apart and document
how it works (and if you cannot do so, ask for help by somebody who
can, or simplify it enough so that you can explain it yourself).

Yes Apologies. That was kind of a cop-out on my part as I was hesitant
to add additional changes that could potentially introduce new issues
to this patch as it is already addressing a fairly obscure issue.

    headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&

In this case, "--sq" is a noop that only confuses readers, I think,
and I would drop it if I were you.  "--git-path HEAD" is given by
this call chain:

   builtin/rev-parse.c:cmd_rev_parse()
   -> builtin/rev-parse.c:print_path()
      -> transform path depending on the path format
         -> puts()

and nowhere in this chain "output_sq" (which is set by "--sq") is
even checked.  The transformations are all about relative, prefix,
etc., and never about quoting.

Understood. I tried running it with `--sq` removed and it seems to
work as you and Phillip expected so I'm adding that to v2.

The original test script t2400 (before your patch) does look crappy
with full of long lines and coding style violations (none of which
is your fault), and it may need to be cleaned up once this patch
settles.

Thanks.

I may give that cleanup a shot some time down the line if nobody else
takes a crack at it first.