Re: [PATCH v4 1/8] fast-import: tighten path unquoting
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 12, 2024, 16:34 UTC
- Message-ID
- <xmqqle5io7zf.fsf@gitster.g>
- In-Reply-To
- <d6ea8aca465eaed4b18b546b5d484bc98fdaa377.1712907684.git.thalia@archibald.dev>
Thalia Archibald <thalia@archibald.dev> writes:
Show 13 quoted lines
> 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.
Show 14 quoted lines
> +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.
Show 12 quoted lines
> +/*
> + * 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.
Show 13 quoted lines
> +/*
> + * 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.