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

11 messages, 4 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:15

Jonathan Nieder [off-list ref] writes:
For what it's worth,
Acked-by: Jonathan Nieder <redacted>

Thanks very much for taking care of it.
quoted
[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.)

...
(good).  What do you think?

-- >8 --
Subject: fast-import: allow filecopy to copy from root
So what do you guys want to do with topic?  My gut feeling is that
this is not a new regression and can wait until the next cycle.  I
could certainly carry David's patch in 'pu' if doing so helps the
discussion to come up with the right solution, though.

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

From: Dmitry Ivankov <hidden>
Date: 2016-06-15 22:53:15

Don't quite have the time to run tests, but maybe the issue is solved
 in a stalled series [1]. At least [1] is worth looking at with regard
 to this bug.

 One more quick thought. "force the root mode to S_IFDIR" part doesn't
 look obviously good for me. First, isn't it already ensured that the
 root is a directory? Second, if it is allowed to be a file I'm not
 sure it's ok to silently make it a directory on a root-to-subtree
 operation, do we do it for subtree-to-subsubtree?

 P.S. looking at [1] now I'd say the commit messages could be improved there

 [1] http://thread.gmane.org/gmane.comp.version-control.git/179426

On Thu, Mar 8, 2012 at 10:39 PM, Junio C Hamano [off-list ref] wrote:
Jonathan Nieder [off-list ref] writes:
quoted
For what it's worth,
Acked-by: Jonathan Nieder <redacted>

Thanks very much for taking care of it.
quoted
[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.)

...
(good).  What do you think?

-- >8 --
Subject: fast-import: allow filecopy to copy from root
So what do you guys want to do with topic?  My gut feeling is that
this is not a new regression and can wait until the next cycle.  I
could certainly carry David's patch in 'pu' if doing so helps the
discussion to come up with the right solution, though.

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:15

Hi,

Junio C Hamano wrote:
So what do you guys want to do with topic?  My gut feeling is that
this is not a new regression and can wait until the next cycle.
Yes, I think you are very right.

. I have a vague fear that my "allow filecopy to copy from root" patch
  on top of David's is missing some handling of the empty src case,
  along the same lines as 8fe533f6 (fast-import: treat filemodify with
  empty tree as delete.

. After looking closer at David's patch, it does not seem to handle
  'ls <tree> ""' carefully enough.  It probably needs something
  like Dmitry's [1].

So please backburner this, and we can try for something better by
next cycle.

Thanks for a sanity check.
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/179426/focus=179425

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:15

Dmitry Ivankov wrote:
 One more quick thought. "force the root mode to S_IFDIR" part doesn't
 look obviously good for me.
It was just a problematic and incomplete version of what your "be
saner with temporary trees" does properly. ;-)

[...]
 P.S. looking at [1] now I'd say the commit messages could be improved there

 [1] http://thread.gmane.org/gmane.comp.version-control.git/179426
Yes, please.  Or patch 2/2 could be split into multiple patches,
perhaps along the following lines:

 - ls "" support, as in David's patch
 - ls <dataref> "" support, which requires the "be saner with
   temporaries" fix
 - D "" support, maybe.  (As you mentioned, we have deleteall so
   compatibility would dictate not supporting it unless some frontend
   is using it already by mistake in some circumstance.)
 - C "" <dest> support, as in my reply to David's patch
 - R "" <dest> support

Thanks,
Jonathan

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
quoted
[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.)
[...]
So what do you guys want to do with topic?  My gut feeling is that
this is not a new regression and can wait until the next cycle.
Thanks again for the advice so far.

After sleeping on it, here are two patches for 'maint'.  One plugs a
memory leak.  The other makes my above comment actually true, so
trying to use this missing feature results in an error message that
can help the frontend author instead of the silently broken conversion
Andrew found.

Then we can carefully add 'ls ""' support in 1.7.11.

svn-fe should probably also be tweaked to handle this case without
demanding support for the (nice) 'ls empty path' extension in
fast-import backends, and this could even happen before 1.7.10.
I can't promise to get to that quickly enough, though.

Sensible?

Jonathan

[PATCH maint-1.7.6] fast-import: leakfix for 'ls' of dirty trees

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

When the chosen directory has changed since it was last written to
pack, "tree_content_get" makes a deep copy of its content to scribble
on while computing the tree name, which we forgot to free.

This leak has been present since the 'ls' command was introduced in
v1.7.5-rc0~3^2~33 (fast-import: add 'ls' command, 2010-12-02).

Signed-off-by: Jonathan Nieder <redacted>
---
Sorry to have missed this buglet for so long.  (Doubly so because it
was noticed and commented on half a year ago before being promptly
forgotten.)  Patch is against commit 8dc6a373d2 which introduced the
leaky 'ls' support.

 fast-import.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/fast-import.c b/fast-import.c
index 6c37b840..fff285cd 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2987,6 +2987,8 @@ static void parse_ls(struct branch *b)
 		store_tree(&leaf);
 
 	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
+	if (leaf.tree)
+		release_tree_content_recursive(leaf.tree);
 	if (!b || root != &b->branch_tree)
 		release_tree_entry(root);
 }
-- 
1.7.9.2

[PATCH maint-1.7.6] fast-import: don't allow 'ls' of path with empty components

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

As the fast-import manual explains:

	The value of <path> must be in canonical form. That is it must
	not:
	. contain an empty directory component (e.g. foo//bar is invalid),
	. end with a directory separator (e.g. foo/ is invalid),
	. start with a directory separator (e.g. /foo is invalid),

Unfortunately the "ls" command accepts these invalid syntaxes and
responds by declaring that the indicated path is missing.  This is too
subtle and causes importers to silently misbehave; better to error out
so the operator knows what's happening.

The C, R, and M commands already error out for such paths.

Based on initial analysis by David Barr.

Reported-by: Andrew Sayers <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Also against on 8dc6a373d (fast-import: add 'ls' command, 2010-12-02).

 fast-import.c          |    2 ++
 t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
diff --git a/fast-import.c b/fast-import.c
index fff285cd..47f61f3c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1640,6 +1640,8 @@ static int tree_content_get(
 		n = slash1 - p;
 	else
 		n = strlen(p);
+	if (!n)
+		die("Empty path component found in input");
 
 	if (!root->tree)
 		load_tree(root);
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 6b1ba6c8..2cd0f061 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1088,6 +1088,45 @@ test_expect_success \
 	INPUT_END'
 
+test_expect_success \
+	'N: reject foo/ syntax in copy source' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5C
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	C file2/ file3
+	INPUT_END'
+
+test_expect_success \
+	'N: reject foo/ syntax in rename source' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5D
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	rename with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	R file2/ file3
+	INPUT_END'
+
+test_expect_success \
+	'N: reject foo/ syntax in ls argument' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5E
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	ls "file2/"
+	INPUT_END'
+
 test_expect_success \
 	'N: copy to root by id and modify' \
 	'echo "hello, world" >expect.foo &&
 	 echo hello >expect.bar &&
-- 
1.7.9.2

[PULL maint] two fast-import "ls" fixes

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

Jonathan Nieder wrote:
After sleeping on it, here are two patches for 'maint'.
For your convenience, these changes can also be found at:

  git://repo.or.cz/git/jrn.git tags/fast-import-ls-fixes

      fast-import: leakfix for 'ls' of dirty trees
      fast-import: don't allow 'ls' of path with empty components

 fast-import.c          |    4 ++++
 t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

[NON-PATCH] vcs-svn: avoid 'ls' and filedelete with empty path

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

Jonathan Nieder wrote:
svn-fe should probably also be tweaked to handle this case without
demanding support for the (nice) 'ls empty path' extension in
fast-import backends, and this could even happen before 1.7.10.
I can't promise to get to that quickly enough, though.
On second thought, svn-fe will need the 'ls empty path' extension
before it can handle this case again. :(

-- >8 --
There is a pathological Subversion operation that svn-fe handles
incorrectly due to an unexpected response from fast-import:

  svn cp $SVN_ROOT $SVN_ROOT/subdirectory

svn-fe tries to handle this as a copy from "" to "subdirectory", in
two steps: first, "ls :1 " to retrieve a <dataref> for the root, and
then "M 040000 <dataref> subdirectory" to use it in the active commit.

In git 1.7.9.3 and earlier, the fast-import "ls" command does not
understand that by the empty path we mean the root of the tree.  The
unrecognized path is reported as "missing", so svn-fe emits "D
subdirectory" and continues unaware of the miscommunication that has
taken place.

This is a regression introduced by commit 723b7a27 (vcs-svn: eliminate
repo_tree structure, 2010-12-10).

A patch in flight teaches fast-import to error out, which is a little
better, but still does not win us a successful and accurate import.
Because the meaning of empty paths was not specified in the
fast-import manual until recently, other backends are likely to handle
this construct inconsistently, too.

svn-fe never actually needs to pass "" as an argument to 'ls',
'C', 'R', or 'D'. (*)  There is always another way to spell what it is
trying to do:

. Making a 'ls <foo> ""' request and waiting for a response is a
  complicated way to spell the identity operation.  When <foo> is
  a commit, <foo> can be used directly to name the root of the
  corresponding tree. (*)

. Emitting 'ls ""' to get a name for the root of the current tree
  would be useful in general and there is no other command that
  does that.  Svn-fe never does that: translating non-copy nodes from
  the Subversion dump format does not involve retrieving current
  directory listings to apply a delta to them, and for copies, the
  copyfrom information refers to a specific previous revision.

. svn-fe does not use the filerename (R) and filecopy (C) commands.

. The intent of the command 'D ""' is more clearly written as
  'deleteall'.

Reported-by: Andrew Sayers <redacted>
Explained-by: David Barr [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>

Toy patch, not intended for application.

(*) Lies.
---
 t/t9010-svn-fe.sh     |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/fast_export.c |   25 +++++++++++++++++-
 vcs-svn/fast_export.h |    2 +-
 vcs-svn/repo_tree.c   |    6 +++--
 vcs-svn/repo_tree.h   |    2 +-
 vcs-svn/svndump.c     |    3 ++-
 6 files changed, 101 insertions(+), 6 deletions(-)
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed248..45706bde 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -271,6 +271,75 @@ test_expect_success PIPE 'directory with files' '
 	test_cmp hi directory/file2
 '
 
+test_expect_success PIPE 'copy from root to directory' '
+	reinit_git &&
+	echo hello >hello &&
+	hello_blob=$(git hash-object -w -t blob hello) &&
+	subtree=$(
+		echo "100644 blob $hello_blob	README.txt" |
+		git mktree
+	) &&
+	expect=$(
+		git mktree <<-EOF
+			100644 blob $hello_blob	README.txt
+			040000 tree $subtree	trunk
+		EOF
+	) &&
+
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "2012-10-10T00:01:003.000000Z" \
+			svn:log "created README.txt" &&
+		echo PROPS-END
+	} >r1.props &&
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "2012-10-10T00:02:005.000000Z" \
+			svn:log "created trunk" &&
+		echo PROPS-END
+	} >r2.props &&
+	{
+		cat <<-EOF &&
+		SVN-fs-dump-format-version: 3
+
+		Revision-number: 1
+		EOF
+		echo Prop-content-length: $(wc -c <r1.props) &&
+		echo Content-length: $(wc -c <r1.props) &&
+		echo &&
+		cat r1.props &&
+		cat <<-\EOF &&
+
+		Node-path: README.txt
+		Node-kind: file
+		Node-action: add
+		EOF
+		text_no_props hello &&
+		echo Revision-number: 2
+		echo Prop-content-length: $(wc -c <r2.props) &&
+		echo Content-length: $(wc -c <r2.props) &&
+		echo &&
+		cat r2.props &&
+		sed -e "s/X\$//" <<-\EOF
+
+		Node-path: trunk
+		Node-kind: dir
+		Node-action: add
+		Node-copyfrom-rev: 1
+		Node-copyfrom-path: X
+		Prop-content-length: 10
+		Content-length: 10
+
+		PROPS-END
+		EOF
+	} >copy-root.dump &&
+	try_dump copy-root.dump &&
+
+	git diff-tree --exit-code $expect HEAD
+'
+
 test_expect_success PIPE 'branch name with backslash' '
 	reinit_git &&
 	sort <<-\EOF >expect.branch-files &&
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34c..24232618 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -49,6 +49,12 @@ void fast_export_reset(void)
 
 void fast_export_delete(const char *path)
 {
+	/* delete("") means to return to a clean slate. */
+	if (!*path) {
+		printf("deleteall\n");
+		return;
+	}
+
 	putchar('D');
 	putchar(' ');
 	quote_c_style(path, NULL, stdout, 0);
@@ -113,6 +119,8 @@ void fast_export_end_commit(uint32_t revision)
 
 static void ls_from_rev(uint32_t rev, const char *path)
 {
+	assert(*path);
+
 	/* ls :5 path/to/old/file */
 	printf("ls :%"PRIu32" ", rev);
 	quote_c_style(path, NULL, stdout, 0);
@@ -122,6 +130,8 @@ static void ls_from_rev(uint32_t rev, const char *path)
 
 static void ls_from_active_commit(const char *path)
 {
+	assert(*path);
+
 	/* ls "path/to/file" */
 	printf("ls \"");
 	quote_c_style(path, NULL, stdout, 1);
@@ -285,12 +295,25 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 int fast_export_ls_rev(uint32_t rev, const char *path,
 				uint32_t *mode, struct strbuf *dataref)
 {
+	if (!*path) {
+		/*
+		 * The easy case: when path is "", the caller can use
+		 * :<rev> directly to refer to the root of the tree. (*)
+		 */
+		strbuf_addf(dataref, ":%"PRIu32, rev);
+		*mode = REPO_MODE_DIR;
+		return 0;
+	}
+
 	ls_from_rev(rev, path);
 	return parse_ls_response(get_response_line(), mode, dataref);
 }
 
-int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)
+int fast_export_ls_nonroot(const char *path, uint32_t *mode,
+				struct strbuf *dataref)
 {
+	assert(*path);
+
 	ls_from_active_commit(path);
 	return parse_ls_response(get_response_line(), mode, dataref);
 }
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b65..53e208e2 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -22,7 +22,7 @@ void fast_export_blob_delta(uint32_t mode,
 /* If there is no such file at that rev, returns -1, errno == ENOENT. */
 int fast_export_ls_rev(uint32_t rev, const char *path,
 			uint32_t *mode_out, struct strbuf *dataref_out);
-int fast_export_ls(const char *path,
+int fast_export_ls_nonroot(const char *path,
 			uint32_t *mode_out, struct strbuf *dataref_out);
 
 #endif
diff --git a/vcs-svn/repo_tree.c b/vcs-svn/repo_tree.c
index 67d27f0b..1547b8ce 100644
--- a/vcs-svn/repo_tree.c
+++ b/vcs-svn/repo_tree.c
@@ -8,13 +8,15 @@
 #include "repo_tree.h"
 #include "fast_export.h"
 
-const char *repo_read_path(const char *path, uint32_t *mode_out)
+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out)
 {
 	int err;
 	static struct strbuf buf = STRBUF_INIT;
 
+	assert(*path);
+
 	strbuf_reset(&buf);
-	err = fast_export_ls(path, mode_out, &buf);
+	err = fast_export_ls_nonroot(path, mode_out, &buf);
 	if (err) {
 		if (errno != ENOENT)
 			die_errno("BUG: unexpected fast_export_ls error");
diff --git a/vcs-svn/repo_tree.h b/vcs-svn/repo_tree.h
index 889c6a3c..466d5a63 100644
--- a/vcs-svn/repo_tree.h
+++ b/vcs-svn/repo_tree.h
@@ -11,7 +11,7 @@ struct strbuf;
 uint32_t next_blob_mark(void);
 void repo_copy(uint32_t revision, const char *src, const char *dst);
 void repo_add(const char *path, uint32_t mode, uint32_t blob_mark);
-const char *repo_read_path(const char *path, uint32_t *mode_out);
+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out);
 void repo_delete(const char *path);
 void repo_commit(uint32_t revision, const char *author,
 		const struct strbuf *log, const char *uuid, const char *url,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ca63760f..d610184d 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -248,7 +248,8 @@ static void handle_node(void)
 		old_data = NULL;
 	} else if (node_ctx.action == NODEACT_CHANGE) {
 		uint32_t mode;
-		old_data = repo_read_path(node_ctx.dst.buf, &mode);
+		assert(*node_ctx.dst.buf);
+		old_data = repo_read_nonroot_path(node_ctx.dst.buf, &mode);
 		if (mode == REPO_MODE_DIR && type != REPO_MODE_DIR)
 			die("invalid dump: cannot modify a directory into a file");
 		if (mode != REPO_MODE_DIR && type == REPO_MODE_DIR)
-- 
1.7.9.2

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

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:53:16

Jonathan Nieder wrote:
Dmitry Ivankov wrote:
quoted
 One more quick thought. "force the root mode to S_IFDIR" part doesn't
 look obviously good for me. First, isn't it already ensured that the
 root is a directory?
[...]
It was just a problematic and incomplete version of what your "be
saner with temporary trees" does properly. 
To tie up this loose end: looks like David's patch was ok in this
respect and my worries unfounded.  What I was missing is that
store_tree() does nothing unless its tree argument is dirty, and the
temporary tree used to repesent <treeish> in "ls <treeish> <path>" is
never dirty.

Of course, the reminder of the "be saner" patch and the tree delta
discussion was still very useful.

Thanks for your thoughtfulness.
Jonathan

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

From: Dave Abrahams <hidden>
Date: 2016-06-15 22:57:52

Jonathan Nieder <jrnieder <at> gmail.com> writes:
After sleeping on it, here are two patches for 'maint'.  One plugs a
memory leak.  The other makes my above comment actually true, so
trying to use this missing feature results in an error message that
can help the frontend author instead of the silently broken conversion
Andrew found.

Then we can carefully add 'ls ""' support in 1.7.11.
The support for 'ls ""' was nevre actually added, was it?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help