git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Mar 8, 2012, 07:09 UTC
Message-ID
<20120308070951.GA2181@burratino>
In-Reply-To
<1331184656-98629-1-git-send-email-davidbarr@google.com>

(+cc: Sverre) David Barr wrote:

Show 17 quoted lines
> 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.)

[...]
Show 13 quoted lines
> --- 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
Previous: David BarrNext: Sverre Rabbelier
Message 3 of 23 in “[BUG] fast-import: ls command on commit root returns missing (was: Bug in svn-fe: copying the root directory acts as if it's an empty directory)”
  1. David BarrMar 8, 2012
  2. fast-import: fix ls command with empty pathDavid Barr, Mar 8, 2012
  3. Jonathan NiederMar 8, 2012
  4. Sverre RabbelierMar 8, 2012
  5. Junio C HamanoMar 8, 2012
  6. Dmitry IvankovMar 8, 2012
  7. Jonathan NiederMar 8, 2012
  8. Jonathan NiederMar 10, 2012
  9. Jonathan NiederMar 8, 2012
  10. Jonathan NiederMar 10, 2012
  11. fast-import: leakfix for 'ls' of dirty treesJonathan Nieder, Mar 10, 2012
  12. fast-import: don't allow 'ls' of path with empty componentsJonathan Nieder, Mar 10, 2012
  13. [PULL maint] two fast-import "ls" fixesJonathan Nieder, Mar 10, 2012
  14. vcs-svn: avoid 'ls' and filedelete with empty pathJonathan Nieder, Mar 10, 2012
  15. Dave AbrahamsJun 21, 2013
  16. 0/2 Re: fast-import: fix ls command with empty pathJonathan Nieder, Mar 8, 2012
  17. 1/2 fast-import: plug leak of dirty trees in 'ls' commandJonathan Nieder, Mar 8, 2012
  18. 2/2 fast-import: teach ls command to accept empty pathJonathan Nieder, Mar 8, 2012
  19. David BarrMar 9, 2012
  20. Jonathan NiederMar 9, 2012
  21. fast-import: allow 'ls' and filecopy to read the rootJonathan Nieder, Mar 10, 2012
  22. Jonathan NiederMar 10, 2012
  23. 2/1 fixup! fast-import: allow 'ls' and filecopy to read the rootJonathan Nieder, Mar 10, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.