Re: [PATCH 2/6] quote_path: give flags parameter to quote_path()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2020, 15:04 UTC
Message-ID: <xmqqo8mdr7ve.fsf@gitster.c.googlers.com>
In-Reply-To: <20200910122111.GA57925@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 15 quoted lines

> On Tue, Sep 08, 2020 at 01:52:20PM -0700, Junio C Hamano wrote:
>
>> -char *quote_path(const char *in, const char *prefix, struct strbuf *out)
>> +char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
>>  {
>>  	struct strbuf sb = STRBUF_INIT;
>>  	const char *rel = relative_path(in, prefix, &sb);
>>  	strbuf_reset(out);
>> -	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
>> +	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
>>  	strbuf_release(&sb);
>
> Here we take "flags", but then we pass it along in place of
> quote_c_style_counted()'s "no_dq" parameter. That seems unlikely to be
> the right thing (and indeed, it's reverted in the next commit).

You are seeing a rebase artifact. It needs to be corrected. Thanks for noticing.

As you seem to have guessed correctly, the initial plan was to have the new logic in quote-c-style-counted and make anything at higher level of the callchain just relay the "SP must be quoted" bit, and the final two patches that are no longer necessary in the current series were placed much earlier in an earlier draft of the series as preparatory steps for that endgame.

But it turned out that the main loop of quote-c-style-counted needs a surgery that is a bit larger than I would have liked, so the final series consumes the "SP must be quoted" bit in quote_path() without telling quote-c-style-counted about it.

The problem with the main loop of quote-style-counted I saw was that the next_quote_pos() is designed to return the position of the byte that must be prefixed with a backslash, and the machinery to help it (namely, cq_must_quote() and cq_lookup[]) are written with that in mind quite firmly. The handling of SP we would be optionally adding to the mix is somewhat different---it forces the end result to be enclosed within a dq-pair, but it byitself does not need backslash quoting. Which means cq_lookup[] table needs to somehow encode a bit per byte that says "byte with this value itself does not need to be backslash-quoted, but the entire thing needs to be placed in a dq-pair", and/or next_quote_pos() needs be able to say "here is a byte to cause us to enclose the whole thing in a dq-pair, but the byte itself need not be backslash-quoted".

Of course none of the above becomes unnecessary if we scan the whole string for SP before the main loop in quote-c-style-counted, but the function was written to process the input in a single pass and such a change defeats its design. If we need to do it in two passes, we can have the caller do so anyway, at least for now. That thinking lead to the final organization of the series, with two steps that used to be preparatory for passing the flag down thru to the bottom layer rebased out as a discardable appendix at the end.

Thanks.