On Fri, Mar 22, 2024 at 12:03:18AM +0000, Thalia Archibald wrote:
> Path parsing in fast-import is inconsistent and many unquoting errors
> are suppressed.
> 
> `<path>` appears in the grammar in these places:
> 
>     filemodify ::= 'M' SP <mode> (<dataref> | 'inline') SP <path> LF
>     filedelete ::= 'D' SP <path> LF
>     filecopy   ::= 'C' SP <path> SP <path> LF
>     filerename ::= 'R' SP <path> SP <path> LF
>     ls         ::= 'ls' SP <dataref> SP <path> LF
>     ls-commit  ::= 'ls' SP <path> LF
> 
> and fast-import.c parses them in five different ways:
> 
> 1. For filemodify and filedelete:
>    If `<path>` is a valid quoted string, unquote it;
>    otherwise, treat it as literal bytes (including any number of SP).
> 2. For filecopy (source) and filerename (source):
>    If `<path>` is a valid quoted string, unquote it;
>    otherwise, treat it as literal bytes until the next SP.
> 3. For filecopy (dest) and filerename (dest):
>    Like 1., but an unquoted empty string is an error.
> 4. For ls:
>    If `<path>` starts with `"`, unquote it and report parse errors;
>    otherwise, treat it as literal bytes (including any number of SP).
> 5. For ls-commit:
>    Unquote `<path>` and report parse errors.
>    (It must start with `"` to disambiguate from ls.)
> 
> In the first three, any errors from trying to unquote a string are
> suppressed, so a quoted string that contains invalid escapes would be
> interpreted as literal bytes. For example, `"\xff"` would fail to
> unquote (because hex escapes are not supported), and it would instead be
> interpreted as the byte sequence `"` `\` `x` `f` `f` `"`, which is
> certainly not intended. Some front-ends erroneously use their language's
> standard quoting routine and could silently introduce escapes that would
> be incorrectly parsed due to this.
> 
> The documentation states that “To use a source path that contains SP the
> path must be quoted.”, so it is expected that some implementations
> depend on spaces being allowed in paths in the final position. Thus we
> have two documented ways to parse paths, so simplify the implementation
> to that.
> 
> Now we have:
> 
> 1. `parse_path_eol` for filemodify, filedelete, filecopy (dest),
>    filerename (dest), ls, and ls-commit:
> 
>    If `<path>` starts with `"`, unquote it and report parse errors;
>    otherwise, treat it as literal bytes (including any number of SP).
>    Garbage after a quoted string or an unquoted empty string are errors.
>    (In ls-commit, it must be quoted to disambiguate from ls.)
> 
> 2. `parse_path_space` for filecopy (source) and filerename (source):
> 
>    If `<path>` starts with `"`, unquote it and report parse errors;
>    otherwise, treat it as literal bytes until the next SP.
>    It must be followed by a SP. An unquoted empty string is an error.
> 
> Signed-off-by: Thalia Archibald <thalia@archibald.dev>
> ---
>  Documentation/git-fast-import.txt |   3 +-
>  builtin/fast-import.c             | 115 ++++++++------
>  t/t9300-fast-import.sh            | 252 +++++++++++++++++++++++++++++-
>  3 files changed, 316 insertions(+), 54 deletions(-)
> 
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index b2607366b9..271bd63a10 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -649,7 +649,8 @@ The value of `<path>` must be in canonical form. That is it must not:
>  * contain the special component `.` or `..` (e.g. `foo/./bar` and
>    `foo/../bar` are invalid).
>  
> -The root of the tree can be represented by an empty string as `<path>`.
> +The root of the tree can be represented by a quoted empty string (`""`)
> +as `<path>`.

So this is part of the "filemodify" section with the following syntax:

    'M' SP <mode> SP <dataref> SP <path> LF

The way I interpret this change is that <path> could previously be empty
(`SP LF`), but now it needs to be quoted (`SP '"' '"' LF). This seems to
be related to cases (1) and (3) of your commit messages, where
"filemodify" could contain an unquoted empty string whereas "filecopy"
and "filerename" would complain about such an unquoted string.

In any case I don't see a strong argument why exactly it should be
forbidden to have an unquoted empty path here, and I do wonder whether
it would break existing writers of the format when we retroactively
tighten this case. Isn't it possible to instead loosen it such that all
three of the above actions know to handle unquoted empty paths?

>  It is recommended that `<path>` always be encoded using UTF-8.
>  
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 71a195ca22..b2adec8d9a 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2224,7 +2224,7 @@ static int parse_mapped_oid_hex(const char *hex, struct object_id *oid, const ch
>   *
>   *   idnum ::= ':' bigint;
>   *
> - * Return the first character after the value in *endptr.
> + * Update *endptr to point to the first character after the value.

I think it would make sense to put these improvements for comments into
a separate commit. Otherwise it makes you wonder whether this behaviour
is new now.

>   * Complain if the following character is not what is expected,
>   * either a space or end of the string.
> @@ -2257,8 +2257,8 @@ static uintmax_t parse_mark_ref_eol(const char *p)
>  }
>  
>  /*
> - * Parse the mark reference, demanding a trailing space.  Return a
> - * pointer to the space.
> + * Parse the mark reference, demanding a trailing space. Update *p to
> + * point to the first character after the space.
>   */

Same.

>  static uintmax_t parse_mark_ref_space(const char **p)
>  {
> @@ -2272,10 +2272,57 @@ 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 `allow_spaces` is nonzero.
> + */
> +static void parse_path(struct strbuf *sb, const char *p, const char **endp, int allow_spaces, const char *field)
> +{
> +	strbuf_reset(sb);

It's not all that customary in our codebase to have the function reset
the `strbuf` for the caller because it does make the function less
flexible. I would either keep the `strbuf_reset()` on the caller side or
at least document this behaviour in the comment.

> +	if (*p == '"') {
> +		if (unquote_c_style(sb, p, endp))
> +			die("Invalid %s: %s", field, command_buf.buf);
> +	} else {
> +		if (allow_spaces)
> +			*endp = p + strlen(p);

I wonder whether `stop_at_unquoted_space` might be more telling. It's
not like we disallow spaces here, it's that we treat them as the
separator to the next field.

> +		else
> +			*endp = strchr(p, ' ');
> +		if (*endp == p)
> +			die("Missing %s: %s", field, command_buf.buf);

Error messages should start with a lower-case letter and be
translateable. But these are simply moved over from the previous code,
so I don't mind much if you want to keep them as-is.

> +		strbuf_add(sb, p, *endp - p);
> +	}
> +}
> +
> +/*
> + * 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);
> +}
> +
> +/*
> + * 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)++;
> +}
> +
>  static void file_change_m(const char *p, struct branch *b)
>  {
>  	static struct strbuf uq = STRBUF_INIT;
> -	const char *endp;
>  	struct object_entry *oe;
>  	struct object_id oid;
>  	uint16_t mode, inline_data = 0;
> @@ -2312,12 +2359,8 @@ static void file_change_m(const char *p, struct branch *b)
>  			die("Missing space after SHA1: %s", command_buf.buf);
>  	}
>  
> -	strbuf_reset(&uq);
> -	if (!unquote_c_style(&uq, p, &endp)) {
> -		if (*endp)
> -			die("Garbage after path in: %s", command_buf.buf);
> -		p = uq.buf;
> -	}
> +	parse_path_eol(&uq, p, "path");
> +	p = uq.buf;

This is loosening the condition so that we also accept unquoted paths
now. Okay.

>  
>  	/* Git does not track empty, non-toplevel directories. */
>  	if (S_ISDIR(mode) && is_empty_tree_oid(&oid) && *p) {
> @@ -2381,48 +2424,23 @@ static void file_change_m(const char *p, struct branch *b)
>  static void file_change_d(const char *p, struct branch *b)
>  {
>  	static struct strbuf uq = STRBUF_INIT;
> -	const char *endp;
>  
> -	strbuf_reset(&uq);
> -	if (!unquote_c_style(&uq, p, &endp)) {
> -		if (*endp)
> -			die("Garbage after path in: %s", command_buf.buf);
> -		p = uq.buf;
> -	}
> +	parse_path_eol(&uq, p, "path");
> +	p = uq.buf;
>  	tree_content_remove(&b->branch_tree, p, NULL, 1);
>  }

Same.

> -static void file_change_cr(const char *s, struct branch *b, int rename)
> +static void file_change_cr(const char *p, struct branch *b, int rename)
>  {
> -	const char *d;
> +	const char *s, *d;
>  	static struct strbuf s_uq = STRBUF_INIT;
>  	static struct strbuf d_uq = STRBUF_INIT;
> -	const char *endp;
>  	struct tree_entry leaf;
>  
> -	strbuf_reset(&s_uq);
> -	if (!unquote_c_style(&s_uq, s, &endp)) {
> -		if (*endp != ' ')
> -			die("Missing space after source: %s", command_buf.buf);
> -	} else {
> -		endp = strchr(s, ' ');
> -		if (!endp)
> -			die("Missing space after source: %s", command_buf.buf);
> -		strbuf_add(&s_uq, s, endp - s);
> -	}
> +	parse_path_space(&s_uq, p, &p, "source");
> +	parse_path_eol(&d_uq, p, "dest");
>  	s = s_uq.buf;
> -
> -	endp++;
> -	if (!*endp)
> -		die("Missing dest: %s", command_buf.buf);
> -
> -	d = endp;
> -	strbuf_reset(&d_uq);
> -	if (!unquote_c_style(&d_uq, d, &endp)) {
> -		if (*endp)
> -			die("Garbage after dest in: %s", command_buf.buf);
> -		d = d_uq.buf;
> -	}
> +	d = d_uq.buf;

Nice simplification. The source path should behave the same, and parsing
of the destination path has been loosened to also allow unquoted paths.

>  	memset(&leaf, 0, sizeof(leaf));
>  	if (rename)
> @@ -3168,6 +3186,7 @@ static void parse_ls(const char *p, struct branch *b)
>  {
>  	struct tree_entry *root = NULL;
>  	struct tree_entry leaf = {NULL};
> +	static struct strbuf uq = STRBUF_INIT;

I know the code had this as a static variable before, as well. But is
this really necessary? Can't we leave it as non-static and then release
the buffer at the end of this function?

>  	/* ls SP (<tree-ish> SP)? <path> */
>  	if (*p == '"') {
> @@ -3182,16 +3201,8 @@ static void parse_ls(const char *p, struct branch *b)
>  			root->versions[1].mode = S_IFDIR;
>  		load_tree(root);
>  	}
> -	if (*p == '"') {
> -		static struct strbuf uq = STRBUF_INIT;
> -		const char *endp;
> -		strbuf_reset(&uq);
> -		if (unquote_c_style(&uq, p, &endp))
> -			die("Invalid path: %s", command_buf.buf);
> -		if (*endp)
> -			die("Garbage after path in: %s", command_buf.buf);
> -		p = uq.buf;
> -	}
> +	parse_path_eol(&uq, p, "path");
> +	p = uq.buf;

And this case should behave the same.

>  	tree_content_get(root, p, &leaf, 1);
>  	/*
>  	 * A directory in preparation would have a sha1 of zero
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index dbb5042b0b..ef04b43f46 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -2146,6 +2146,7 @@ test_expect_success 'Q: deny note on empty branch' '
>  	EOF
>  	test_must_fail git fast-import <input
>  '
> +
>  ###
>  ### series R (feature and option)
>  ###
> @@ -2794,7 +2795,7 @@ test_expect_success 'R: blob appears only once' '
>  '
>  
>  ###
> -### series S
> +### series S (mark and path parsing)
>  ###
>  #
>  # Make sure missing spaces and EOLs after mark references
> @@ -3064,6 +3065,255 @@ test_expect_success 'S: ls with garbage after sha1 must fail' '
>  	test_grep "space after tree-ish" err
>  '
>  
> +#
> +# Path parsing
> +#
> +# There are two sorts of ways a path can be parsed, depending on whether it is
> +# the last field on the line. Additionally, ls without a <tree-ish> has a
> +# special case. Test every occurrence of <path> in the grammar against every
> +# error case.
> +#
> +
> +#
> +# Valid paths at the end of a line: filemodify, filedelete, filecopy (dest),
> +# filerename (dest), and ls.
> +#
> +# commit :301 from root -- modify hello.c
> +# commit :302 from :301 -- modify $path
> +# commit :303 from :302 -- delete $path
> +# commit :304 from :301 -- copy hello.c $path
> +# commit :305 from :301 -- rename hello.c $path
> +# ls :305 $path
> +#
> +test_path_eol_success () {
> +	test="$1" path="$2" unquoted_path="$3"

Should these variables be local?

> +	test_expect_success "S: paths at EOL with $test must work" '
> +		git fast-import --export-marks=marks.out <<-EOF >out 2>err &&
> +		blob
> +		mark :401
> +		data <<BLOB
> +		hello world
> +		BLOB
> +
> +		blob
> +		mark :402
> +		data <<BLOB
> +		hallo welt
> +		BLOB
> +
> +		commit refs/heads/path-eol
> +		mark :301
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		initial commit
> +		COMMIT
> +		M 100644 :401 hello.c
> +
> +		commit refs/heads/path-eol
> +		mark :302
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filemodify
> +		COMMIT
> +		from :301
> +		M 100644 :402 '"$path"'
> +
> +		commit refs/heads/path-eol
> +		mark :303
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filedelete
> +		COMMIT
> +		from :302
> +		D '"$path"'
> +
> +		commit refs/heads/path-eol
> +		mark :304
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filecopy dest
> +		COMMIT
> +		from :301
> +		C hello.c '"$path"'
> +
> +		commit refs/heads/path-eol
> +		mark :305
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filerename dest
> +		COMMIT
> +		from :301
> +		R hello.c '"$path"'
> +
> +		ls :305 '"$path"'
> +		EOF
> +
> +		commit_m=$(grep :302 marks.out | cut -d\  -f2) &&
> +		commit_d=$(grep :303 marks.out | cut -d\  -f2) &&
> +		commit_c=$(grep :304 marks.out | cut -d\  -f2) &&
> +		commit_r=$(grep :305 marks.out | cut -d\  -f2) &&
> +		blob1=$(grep :401 marks.out | cut -d\  -f2) &&
> +		blob2=$(grep :402 marks.out | cut -d\  -f2) &&
> +
> +		( printf "100644 blob $blob2\t'"$unquoted_path"'\n" &&
> +		  printf "100644 blob $blob1\thello.c\n" ) | sort >tree_m.exp &&
> +		git ls-tree $commit_m | sort >tree_m.out &&
> +		test_cmp tree_m.exp tree_m.out &&
> +
> +		printf "100644 blob $blob1\thello.c\n" >tree_d.exp &&
> +		git ls-tree $commit_d >tree_d.out &&
> +		test_cmp tree_d.exp tree_d.out &&
> +
> +		( printf "100644 blob $blob1\t'"$unquoted_path"'\n" &&
> +		  printf "100644 blob $blob1\thello.c\n" ) | sort >tree_c.exp &&
> +		git ls-tree $commit_c | sort >tree_c.out &&
> +		test_cmp tree_c.exp tree_c.out &&
> +
> +		printf "100644 blob $blob1\t'"$unquoted_path"'\n" >tree_r.exp &&
> +		git ls-tree $commit_r >tree_r.out &&
> +		test_cmp tree_r.exp tree_r.out &&
> +
> +		test_cmp out tree_r.exp &&
> +
> +		git branch -D path-eol
> +	'
> +}
> +
> +test_path_eol_success 'quoted spaces'   '" hello world.c "' ' hello world.c '
> +test_path_eol_success 'unquoted spaces' ' hello world.c '   ' hello world.c '
> +
> +#
> +# Valid paths before a space: filecopy (source) and filerename (source).
> +#
> +# commit :301 from root -- modify $path
> +# commit :302 from :301 -- copy $path hello2.c
> +# commit :303 from :301 -- rename $path hello2.c
> +#
> +test_path_space_success () {
> +	test="$1" path="$2" unquoted_path="$3"

Same question here, should these be local?

> +	test_expect_success "S: paths before space with $test must work" '
> +		git fast-import --export-marks=marks.out <<-EOF 2>err &&
> +		blob
> +		mark :401
> +		data <<BLOB
> +		hello world
> +		BLOB
> +
> +		commit refs/heads/path-space
> +		mark :301
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		initial commit
> +		COMMIT
> +		M 100644 :401 '"$path"'
> +
> +		commit refs/heads/path-space
> +		mark :302
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filecopy source
> +		COMMIT
> +		from :301
> +		C '"$path"' hello2.c
> +
> +		commit refs/heads/path-space
> +		mark :303
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit filerename source
> +		COMMIT
> +		from :301
> +		R '"$path"' hello2.c
> +
> +		EOF
> +
> +		commit_c=$(grep :302 marks.out | cut -d\  -f2) &&
> +		commit_r=$(grep :303 marks.out | cut -d\  -f2) &&
> +		blob=$(grep :401 marks.out | cut -d\  -f2) &&
> +
> +		( printf "100644 blob $blob\t'"$unquoted_path"'\n" &&
> +		  printf "100644 blob $blob\thello2.c\n" ) | sort >tree_c.exp &&
> +		git ls-tree $commit_c | sort >tree_c.out &&
> +		test_cmp tree_c.exp tree_c.out &&
> +
> +		printf "100644 blob $blob\thello2.c\n" >tree_r.exp &&
> +		git ls-tree $commit_r >tree_r.out &&
> +		test_cmp tree_r.exp tree_r.out &&
> +
> +		git branch -D path-space
> +	'
> +}
> +
> +test_path_space_success 'quoted spaces'      '" hello world.c "' ' hello world.c '
> +test_path_space_success 'no unquoted spaces' 'hello_world.c'     'hello_world.c'
> +
> +#
> +# Test a single commit change with an invalid path. Run it with all occurrences
> +# of <path> in the grammar against all error kinds.
> +#
> +test_path_fail () {
> +	what="$1" path="$2" err_grep="$3"

Same.

> +	test_expect_success "S: $change with $what must fail" '
> +		test_must_fail git fast-import <<-EOF 2>err &&
> +		blob
> +		mark :1
> +		data <<BLOB
> +		hello world
> +		BLOB
> +
> +		commit refs/heads/S-path-fail
> +		mark :2
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit setup
> +		COMMIT
> +		M 100644 :1 hello.c
> +
> +		commit refs/heads/S-path-fail
> +		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +		data <<COMMIT
> +		commit with bad path
> +		COMMIT
> +		from :2
> +		'"$prefix$path$suffix"'
> +		EOF
> +
> +		test_grep '"'$err_grep'"' err
> +	'
> +}
> +
> +test_path_base_fail () {
> +	test_path_fail 'unclosed " in '"$field"          '"hello.c'    "Invalid $field"
> +	test_path_fail "invalid escape in quoted $field" '"hello\xff"' "Invalid $field"
> +}
> +test_path_eol_quoted_fail () {
> +	test_path_base_fail
> +	test_path_fail "garbage after quoted $field" '"hello.c"x' "Garbage after $field"
> +	test_path_fail "space after quoted $field"   '"hello.c" ' "Garbage after $field"
> +}
> +test_path_eol_fail () {
> +	test_path_eol_quoted_fail
> +	test_path_fail 'empty unquoted path' '' "Missing $field"
> +}
> +test_path_space_fail () {
> +	test_path_base_fail
> +	test_path_fail 'empty unquoted path' '' "Missing $field"
> +	test_path_fail "missing space after quoted $field" '"hello.c"x' "Missing space after $field"
> +}
> +
> +change=filemodify       prefix='M 100644 :1 ' field=path   suffix=''         test_path_eol_fail
> +change=filedelete       prefix='D '           field=path   suffix=''         test_path_eol_fail
> +change=filecopy         prefix='C '           field=source suffix=' world.c' test_path_space_fail
> +change=filecopy         prefix='C hello.c '   field=dest   suffix=''         test_path_eol_fail
> +change=filerename       prefix='R '           field=source suffix=' world.c' test_path_space_fail
> +change=filerename       prefix='R hello.c '   field=dest   suffix=''         test_path_eol_fail
> +change='ls (in commit)' prefix='ls :2 '       field=path   suffix=''         test_path_eol_fail

This is quite confusing because you now mix two different styles, where
some of the functions take arguments while others pass arguments via
variables. I think it would be preferable to pass all arguments as
proper function arguments.

Patrick

> +# When 'ls' has no <tree-ish>, the <path> must be quoted.
> +change='ls (without tree-ish in commit)' prefix='ls ' field=path suffix='' \
> +test_path_eol_quoted_fail &&
> +test_path_fail 'empty unquoted path' '' "Invalid dataref"
> +
>  ###
>  ### series T (ls)
>  ###
> -- 
> 2.44.0
> 
> 
>