Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder <jrnieder@gmail.com>]

(+cc: Sverre)
David Barr wrote:

> When the following command is sent to fast-import:
>
>  'ls' SP ':1' SP LF
>
> The expected output is:
>
>  '040000' SP 'tree' SP <dataref> HT LF
>
> The actual output is:
>
>  'missing' SP LF
>
> This is because tree_content_get() is called but expects a non-empty
> path. Instead, copy the root entry and force the mode to S_IFDIR.
>
> Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
> Signed-off-by: David Barr <davidbarr@google.com>

For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks very much for taking care of it.

> [Subject: fast-import: fix ls command with empty path]

I would s/fix/accept/ to be more precise about the nature of the
breakage.  (In other words: rather than mishandling ls with an empty
path, fast-import was not handling it at all.)

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -3019,7 +3019,12 @@ static void parse_ls(struct branch *b)
>  			die("Garbage after path in: %s", command_buf.buf);
>  		p = uq.buf;
>  	}
> -	tree_content_get(root, p, &leaf);
> +	if (*p) {
> +		tree_content_get(root, p, &leaf);
> +	} else {
> +		leaf = *root;
> +		leaf.versions[1].mode = S_IFDIR;
> +	}

If this special case were implemented in tree_content_get(), we would
get support for paths with a trailing '/' (allows useless
incompatibility, bad) and support for requests like

	C "" some/subdir

(good).  What do you think?

-- >8 --
Subject: fast-import: allow filecopy to copy from root

Some subversion users apparently use "svn copy $SVN_ROOT
$SVN_ROOT/subdirectory" from time to time.  svn-fe handles this fine
already since it translates the subversion copy instruction to

	ls ""
	M 040000 <returned tree name> subdirectory

We can easily imagine an alternate importer that would write

	C "" subdirectory

instead, so handle that, too.

A naive implementation would also mean gaining support for copies
where the source has a trailing '/', as in

	C onedir/ anotherdir

but in the spirit of 34215783 (fast-import: tighten M 040000 syntax,
2010-10-17), this patch is careful to reject that syntax to avoid
making it too easy for frontends to introduce unnecessary
incompatibilities with git fast-import 1.7.8 and older and other
fast-import consumers.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c          |   32 ++++++++++++++-----------
 t/t9300-fast-import.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 8dbfd4cc..5ce61bca 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1636,6 +1636,10 @@ static int tree_content_get(
 	unsigned int i, n;
 	struct tree_entry *e;
 
+	if (!*p) {
+		e = root;
+		goto last_component;
+	}
 	slash1 = strchr(p, '/');
 	if (slash1)
 		n = slash1 - p;
@@ -1648,14 +1652,10 @@ static int tree_content_get(
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
-			if (!slash1) {
-				memcpy(leaf, e, sizeof(*leaf));
-				if (e->tree && is_null_sha1(e->versions[1].sha1))
-					leaf->tree = dup_tree_content(e->tree);
-				else
-					leaf->tree = NULL;
-				return 1;
-			}
+			if (!slash1)
+				goto last_component;
+			if (!slash1[1])	/* paths with trailing '/' do not match */
+				return 0;
 			if (!S_ISDIR(e->versions[1].mode))
 				return 0;
 			if (!e->tree)
@@ -1664,6 +1664,14 @@ static int tree_content_get(
 		}
 	}
 	return 0;
+
+last_component:
+	memcpy(leaf, e, sizeof(*leaf));
+	if (e->tree && is_null_sha1(e->versions[1].sha1))
+		leaf->tree = dup_tree_content(e->tree);
+	else
+		leaf->tree = NULL;
+	return 1;
 }
 
 static int update_branch(struct branch *b)
@@ -3005,6 +3013,7 @@ static void parse_ls(struct branch *b)
 		struct object_entry *e = parse_treeish_dataref(&p);
 		root = new_tree_entry();
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		root->versions[1].mode = S_IFDIR;
 		load_tree(root);
 		if (*p++ != ' ')
 			die("Missing space after tree-ish: %s", command_buf.buf);
@@ -3019,12 +3028,7 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	if (*p) {
-		tree_content_get(root, p, &leaf);
-	} else {
-		leaf = *root;
-		leaf.versions[1].mode = S_IFDIR;
-	}
+	tree_content_get(root, p, &leaf);
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2558a2ed..5316b73c 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1047,6 +1047,66 @@ test_expect_success \
 	 git diff-tree -C --find-copies-harder -r N1^ N1 >actual &&
 	 compare_diff_raw expect actual'
 
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+copy to subdir
+COMMIT
+
+from refs/heads/branch^0
+C "" subdir
+
+INPUT_END
+
+cat >expect <<\EOF
+:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	subdir/file2/newf
+:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	subdir/file2/oldf
+:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	subdir/file4
+:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	subdir/newdir/exec.sh
+:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	subdir/newdir/interesting
+EOF
+test_expect_success \
+	'N: copy with empty source path' \
+	'git fast-import <input &&
+	 git diff-tree -C -C -r --no-commit-id N-root-to-subdir >actual &&
+	 compare_diff_raw expect actual'
+
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-unquoted-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+increase nesting
+COMMIT
+
+from refs/heads/branch^0
+C  subdir
+
+INPUT_END
+test_expect_success \
+	'N: copy with unquoted empty source path' \
+	'git fast-import <input &&
+	 git diff --exit-code N-root-to-subdir N-unquoted-root-to-subdir'
+
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-trailing-slash-in-src
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+copy newdir to newerdir
+COMMIT
+
+from refs/heads/branch^0
+C newdir/ newerdir
+
+INPUT_END
+test_expect_success \
+	'N: reject foo/ as source path' \
+	'# fatal: path not in branch
+	 test_must_fail git fast-import <input'
+
 cat >input <<INPUT_END
 commit refs/heads/N2
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
@@ -1293,7 +1353,7 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
-	'N: reject foo/ syntax' \
+	'N: filemodify: reject foo/ syntax' \
 	'subdir=$(git rev-parse refs/heads/branch^0:file2) &&
 	 test_must_fail git fast-import <<-INPUT_END
 	commit refs/heads/N5B
-- 
1.7.9.2