Thalia Archibald <thalia@xxxxxxxxxxxxx> 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.
