Re: [PATCH v4 1/8] fast-import: tighten path unquoting

[Junio C Hamano <gitster@pobox.com>]

Thalia Archibald <thalia@archibald.dev> writes:

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 782bda007c..ce9231afe6 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2258,10 +2258,56 @@ static uintmax_t parse_mark_ref_space(const char **p)
>  	return mark;
>  }
>  
> +/*
> + * Parse the path string into the strbuf. It may be quoted with escape sequences
> + * or unquoted without escape sequences. When unquoted, it may only contain a
> + * space if `include_spaces` is nonzero.
> + */

It took me three reads to understand the last sentence.  It would
have been easier if it were written as "it may contain a space only
if ...".  I'd also named it "allow_unquoted_spaces"---it is not like
this function includes extra spaces on top of whatever was given.

> +static void parse_path(struct strbuf *sb, const char *p, const char **endp,
> +		int include_spaces, const char *field)
> +{
> +	if (*p == '"') {
> +		if (unquote_c_style(sb, p, endp))
> +			die("Invalid %s: %s", field, command_buf.buf);
> +	} else {
> +		if (include_spaces)
> +			*endp = p + strlen(p);
> +		else
> +			*endp = strchrnul(p, ' ');
> +		strbuf_add(sb, p, *endp - p);
> +	}
> +}

A very straigtht-forward implementation.  Makes sense.

> +/*
> + * Parse the path string into the strbuf, and complain if this is not the end of
> + * the string. It may contain spaces even when unquoted.
> + */
> +static void parse_path_eol(struct strbuf *sb, const char *p, const char *field)
> +{
> +	const char *end;
> +
> +	parse_path(sb, p, &end, 1, field);
> +	if (*end)
> +		die("Garbage after %s: %s", field, command_buf.buf);
> +}

OK.

> +/*
> + * Parse the path string into the strbuf, and ensure it is followed by a space.
> + * It may not contain spaces when unquoted. Update *endp to point to the first
> + * character after the space.
> + */
> +static void parse_path_space(struct strbuf *sb, const char *p,
> +		const char **endp, const char *field)
> +{
> +	parse_path(sb, p, endp, 0, field);
> +	if (**endp != ' ')
> +		die("Missing space after %s: %s", field, command_buf.buf);
> +	(*endp)++;
> +}

OK.

The updated callers that use the above helper functions do read a
lot more easily, while filling the gaps in the original
implementation.  Very nicely done.

Thanks.