Re: [PATCH] fast-import: Allow filemodify to set the root

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 11, 2010, 06:34 UTC
Message-ID: <20101011063429.GA32034@burratino>
In-Reply-To: <1286681415-1831-1-git-send-email-david.barr@cordelta.com>

David Barr wrote:

Show 9 quoted lines

> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -524,6 +524,9 @@ start with double quote (`"`).
>  If an `LF` or double quote must be encoded into `<path>` shell-style
>  quoting should be used, e.g. `"path/with\n and \" in it"`.
>  
> +Additionally, in `040000` mode, `<path>` may also be an empty string
> +(`""`) to specify the root of the tree.
> +

Ideally this would be not so much "Additionally" as "For example". Maybe just:

	An empty path ("") refers to the toplevel directory of
	the tracked tree.

Show 17 quoted lines

> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1454,6 +1454,15 @@ static int tree_content_set(
>  		n = slash1 - p;
>  	else
>  		n = strlen(p);
> +	if (!slash1 && !n) {
> +		if (!S_ISDIR(mode))
> +			die("Root cannot be a non-directory");
> +		hashcpy(root->versions[1].sha1, sha1);
> +		if (root->tree)
> +			release_tree_content_recursive(root->tree);
> +		root->tree = subtree;
> +		return 1;
> +	}
>  	if (!n)
>  		die("Empty path component found in input");

Background for the curious: tree_content_set() is a recursive function to modify a tree-in-the-making by changing the entry at path p to refer to some specified content with a given mode. The recursion works as one might expect:

	tree_content_set(root, "foo/bar/baz", ...) ->
	 tree_content_set(root:foo, "bar/baz", ...) ->
	  et c

The "if (!n)" check introduced in v1.5.1.3~11^2~1 (Don't allow empty pathnames in fast-import, 2007-04-28) ensures fast-import doesn't end up creating a subdirectory corresponding to an empty path component in a pathname like "foo//bar/baz".

With this patch, an empty path component is allowed again, but only as the last path component. It is used to modify directories. So, for example,

	tree_content_set(root, "foo/bar/", sha1, S_IFDIR)

becomes an almost-synonym for

	tree_content_set(root, "foo/bar", sha1, S_IFDIR)

and

	tree_content_set(root, "foo/bar/", sha1, S_IFREG | 0644)

is rejected.

Why do I say almost-synonym? Because as Ram pointed out, you are not invalidating the parent tree hash, because there may not even _be_ a parent tree.

In other words, with this patch, I worry that a

	M 040000 ...sha1... "foo/bar/"

line would be sometimes ignored and sometimes not.  Confusing.

Would it make sense to just handle the empty-path case in the callers (file_change_m(), file_change_cr()) to avoid this?