On 23/07/15 06:08PM, Junio C Hamano wrote:
> Jacob Abel <jacobabel@xxxxxxxxxx> writes:
>
> >> > @@ -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.
