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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2023, 01:08 UTC
Message-ID: <xmqqilakll2m.fsf@gitster.g>
In-Reply-To: <vn5sylull5lqpitsanlyan5fafxj5dhrxgo6k65c462dhqjbno@uwghfyfdixtk>

Jacob Abel <jacobabel@nullpo.dev> writes:

Show 11 quoted lines

>> > @@ -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).

    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.

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.