threads / patch / 25926

patchRe: [PATCH] fast-import: add 'ls' command

Subject: Re: [PATCH] fast-import: add 'ls' command

## tl;dr

24 messages between Dec 2, 2010 and Jan 28, 2011. Diffs are folded; open one to read it.

replies: 23people: 6as markdown or json

David Barr· Dec 2, 2010, 10:40 UTC · lore

[PATCH/RFC] fast-import: add 'ls' command

This patch is by no means complete - I still need to consider the edge cases. It does achieve the basic requirements for simplifying svn-fe. The vcs-svn library currently maintains an in-memory index of all paths in all revisions. Introducing an `ls` command to fast-import allows this responsibility to be delegated. Most importantly, it will allow access to the tree data on demand which is needed for incremental imports.

The two features that svn-fe will need are access the the current in-flight commit and to previous commits by mark.

 fast-import.c |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 125 insertions(+), 2 deletions(-)

David Barr· Dec 2, 2010, 10:40 UTC · re: David Barr · lore

[PATCH] fast-import: add 'ls' command

There are two forms of the 'ls' command, one that takes a tree-ish and one relative to the index. Allow the tree-ish variant to be used anywhere a comment is allowed. Allow the index variant to be used within a commit where file change commands would be used.

The syntax is as such:

 'ls' SP <dataref> SP path LF

and

 'ls' SP 'index' SP path LF

Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: David Barr <david.barr@cordelta.com>
---
 fast-import.c |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 125 insertions(+), 2 deletions(-)

Show changes to fast-import.c +125 −2

diff --git a/fast-import.c b/fast-import.c
index fbc70cd..854398a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -24,10 +24,12 @@ Format of STDIN stream:
     commit_msg
     ('from' sp committish lf)?
     ('merge' sp committish lf)*
-    file_change*
+    (file_change | ls)*
     lf?;
   commit_msg ::= data;
 
+  ls ::= 'ls' sp 'index' sp path_str lf;
+
   file_change ::= file_clr
     | file_del
     | file_rnm
@@ -132,7 +134,7 @@ Format of STDIN stream:
   ts    ::= # time since the epoch in seconds, ascii base10 notation;
   tz    ::= # GIT style timezone;
 
-     # note: comments and cat requests may appear anywhere
+     # note: comments, ls and cat requests may appear anywhere
      # in the input, except within a data command.  Any form
      # of the data command always escapes the related input
      # from comment processing.
@@ -141,7 +143,9 @@ Format of STDIN stream:
      # must be the first character on that line (an lf
      # preceded it).
      #
+
   cat_blob ::= 'cat-blob' sp (hexsha1 | idnum) lf;
+  ls_tree  ::= 'ls' sp (hexsha1 | idnum) sp path_str lf;
 
   comment ::= '#' not_lf* lf;
   not_lf  ::= # Any byte that is not ASCII newline (LF);
@@ -369,6 +373,7 @@ static int cat_blob_fd = STDOUT_FILENO;
 
 static void parse_argv(void);
 static void parse_cat_blob(void);
+static void parse_ls(struct branch *b);
 
 /* Signal handling */
 static volatile sig_atomic_t checkpoint_requested;
@@ -2607,6 +2612,8 @@ static void parse_new_commit(void)
 			note_change_n(b, prev_fanout);
 		else if (!strcmp("deleteall", command_buf.buf))
 			file_change_deleteall(b);
+		else if (!prefixcmp(command_buf.buf, "ls "))
+			parse_ls(b);
 		else {
 			unread_command_buf = 1;
 			break;
@@ -2830,6 +2837,120 @@ static void parse_cat_blob(void)
 	cat_blob(oe, sha1);
 }
 
+static struct object_entry *parse_treeish_dataref(const char** r)
+{
+	unsigned char sha1[20];
+	struct object_entry *e;
+
+	if (**r == ':') {
+		char *x;
+		e = find_mark(strtoumax(*r + 1, &x, 10));
+		if (x == *r + 1)
+			die("Invalid mark: %s", command_buf.buf);
+		if (!e)
+			die("Unknown mark: %s", command_buf.buf);
+		*r = x;
+		hashcpy(sha1, e->idx.sha1);
+	} else {
+		if (get_sha1_hex(*r, sha1))
+			die("Invalid SHA1: %s", command_buf.buf);
+		e = find_object(sha1);
+		*r += 40;
+	}
+
+	for (;;) {
+		unsigned long size;
+		void *buf;
+		if (!e) {
+			enum object_type type = sha1_object_info(sha1, NULL);
+			if (type < 0)
+				die("object not found: %s", sha1_to_hex(sha1));
+			e = insert_object(sha1);
+			e->type = type;
+			e->pack_id = MAX_PACK_ID;
+			e->idx.offset = 1;
+		}
+		if (e->type == OBJ_TREE)
+			break;
+
+		if (e->type != OBJ_COMMIT && e->type != OBJ_TAG)
+			die("Not a treeish: %s", command_buf.buf);
+
+		if (e->pack_id != MAX_PACK_ID) {
+			buf = gfi_unpack_entry(e, &size);
+		} else {
+			enum object_type type;
+			buf = read_sha1_file(sha1, &type, &size);
+		}
+		if (!buf)
+			die("Can't load object %s", sha1_to_hex(sha1));
+
+		if (e->type == OBJ_COMMIT) {
+			if (size < 40 + strlen("tree ") ||
+			    get_sha1_hex(buf + strlen("tree "), sha1))
+				die("Invalid SHA1 in commit: %s", command_buf.buf);
+		} else {
+			if (size < 40 + strlen("object ") ||
+			    get_sha1_hex(buf + strlen("object "), sha1))
+				die("Invalid SHA1 in tag: %s", command_buf.buf);
+		}
+		free(buf);
+		e = find_object(sha1);
+	}
+
+	return e;
+}
+
+static void print_ls(int mode, unsigned char *sha1, char *path)
+{
+	enum object_type type;
+	struct strbuf line = STRBUF_INIT;
+	type = sha1_object_info(sha1, NULL);
+	/* mode SP type SP object_name TAB path LF */
+	strbuf_addf(&line, "%o %s %s\t%s\n",
+			mode, typename(type), sha1_to_hex(sha1), path);
+	cat_blob_write(line.buf, line.len);
+	strbuf_release(&line);
+}
+
+static void parse_ls(struct branch *b)
+{
+	const char *p;
+	struct strbuf uq = STRBUF_INIT;
+	struct tree_entry *root = NULL;
+	struct tree_entry leaf = {0};
+
+	/* ls SP <treeish> SP <path> */
+	p = command_buf.buf + strlen("ls ");
+	if(!prefixcmp(p, "index")) {
+		p += strlen("index");
+		if (!b)
+			die("Not in a commit: %s", command_buf.buf);
+		root = &b->branch_tree;
+	} else {
+		struct object_entry *e = parse_treeish_dataref(&p);
+		root = new_tree_entry();
+		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		load_tree(root);
+	}
+	if (*p++ != ' ')
+		die("Missing space after SHA1: %s", command_buf.buf);
+	if (unquote_c_style(&uq, p, &p))
+		die("Invalid path: %s", command_buf.buf);
+	if (*p)
+		die("Garbage after path: %s", command_buf.buf);
+	tree_content_get(root, uq.buf, &leaf);
+	if (!leaf.versions[1].mode)
+		die("Path %s not in branch", uq.buf);
+	/* Allow new trees to be listed. */
+	if (S_ISDIR(leaf.versions[1].mode))
+		store_tree(&leaf);
+	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, uq.buf);
+	strbuf_release(&uq);
+	if (!b || root != &b->branch_tree)
+		release_tree_entry(root);
+}
+
 static void checkpoint(void)
 {
 	checkpoint_requested = 0;
@@ -3128,6 +3249,8 @@ int main(int argc, const char **argv)
 	while (read_next_command() != EOF) {
 		if (!strcmp("blob", command_buf.buf))
 			parse_new_blob();
+		else if (!prefixcmp(command_buf.buf, "ls "))
+			parse_ls(NULL);
 		else if (!prefixcmp(command_buf.buf, "commit "))
 			parse_new_commit();
 		else if (!prefixcmp(command_buf.buf, "tag "))

-- 
1.7.3

Sverre Rabbelier· Dec 2, 2010, 12:37 UTC · re: David Barr · lore

Heya,

On Thu, Dec 2, 2010 at 11:40, David Barr <david.barr@cordelta.com> wrote:

> There are two forms of the 'ls' command, one that takes a tree-ish and
> one relative to the index. Allow the tree-ish variant to be used anywhere
> a comment is allowed. Allow the index variant to be used within a commit
> where file change commands would be used.

The commit message doesn't explain why the index variant isn't allowed anywhere a comment is allowed. I assume that's because there's a half-constructed index if you're in the middle of a modify operation or such?

-- 
Cheers,

Sverre Rabbelier

David Michael Barr· Dec 2, 2010, 12:57 UTC · re: Sverre Rabbelier · lore

Hi Sverre,

Show 9 quoted lines

>> There are two forms of the 'ls' command, one that takes a tree-ish and
>> one relative to the index. Allow the tree-ish variant to be used anywhere
>> a comment is allowed. Allow the index variant to be used within a commit
>> where file change commands would be used.
> 
> The commit message doesn't explain why the index variant isn't allowed
> anywhere a comment is allowed. I assume that's because there's a
> half-constructed index if you're in the middle of a modify operation
> or such?

The index variant is allowed between file change commands, at which the index should be consistent albeit deferred for hashing. I believe this is the only place where there's naturally an implicit active tree.

Simply, "relative to the index" only makes sense when describing a commit.

Unfortunately, the list server dropped my summary email :( The patch was supposed to be marked as a RFC and I gave an outline of the intended use.

The key features I need for svn-fe are access to the index for the current commit and access to the content of older marked commits.

NB: This version doesn't support unquoted paths.

-- David Barr

Jonathan Nieder· Dec 2, 2010, 17:37 UTC · re: Sverre Rabbelier · lore

Sverre Rabbelier wrote:

> On Thu, Dec 2, 2010 at 11:40, David Barr <david.barr@cordelta.com> wrote:

Show 9 quoted lines

>> There are two forms of the 'ls' command, one that takes a tree-ish and
>> one relative to the index. Allow the tree-ish variant to be used anywhere
>> a comment is allowed. Allow the index variant to be used within a commit
>> where file change commands would be used.
>
> The commit message doesn't explain why the index variant isn't allowed
> anywhere a comment is allowed. I assume that's because there's a
> half-constructed index if you're in the middle of a modify operation
> or such?

I somewhat agree. Actually I would go further: the word "index" brings to mind .git/index and its in-core counterpart, so at the same time as documenting it better, we might look into making it more self- explanatory. Maybe a syntax like

	ls current "path/to/entry"

would make it clearer that this is about directory entries in fast-import's active commit and not necessarily the usual index file?

I am not sure what syntax other vcs-es use for tree-ishes. To avoid name clashes (what if 'current' is the low-level name of a tree-ish?), an alternative might be

	ls-tree :11 "path/to/historical/entry"
	ls "path/to/current/entry"

Hm (just musing). Jonathan

Junio C Hamano· Dec 2, 2010, 19:20 UTC · re: Jonathan Nieder · lore

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 9 quoted lines

> I somewhat agree.  Actually I would go further: the word "index" brings
> to mind .git/index and its in-core counterpart, so at the same time as
> documenting it better, we might look into making it more self-
> explanatory.  Maybe a syntax like
>
> 	ls current "path/to/entry"
>
> would make it clearer that this is about directory entries in
> fast-import's active commit and not necessarily the usual index file?

I think that explains the feature better. I was wondering if the stream somehow wanted to access the state the index of the repository happens to be.

Show 6 quoted lines

> I am not sure what syntax other vcs-es use for tree-ishes.  To avoid
> name clashes (what if 'current' is the low-level name of a tree-ish?),
> an alternative might be
>
> 	ls-tree :11 "path/to/historical/entry"
> 	ls "path/to/current/entry"

Is it an option to use "ls" for both cases and treat one-arg and two-arg cases differently?

David Barr· Dec 2, 2010, 22:51 UTC · re: Junio C Hamano · lore

Show 23 quoted lines

> > I somewhat agree.  Actually I would go further: the word "index" brings
> > to mind .git/index and its in-core counterpart, so at the same time as
> > documenting it better, we might look into making it more self-
> > explanatory.  Maybe a syntax like
> >
> >       ls current "path/to/entry"
> >
> > would make it clearer that this is about directory entries in
> > fast-import's active commit and not necessarily the usual index file?
> 
> I think that explains the feature better.  I was wondering if the stream
> somehow wanted to access the state the index of the repository happens to
> be.
> 
> > I am not sure what syntax other vcs-es use for tree-ishes.  To avoid
> > name clashes (what if 'current' is the low-level name of a tree-ish?),
> > an alternative might be
> >
> >       ls-tree :11 "path/to/historical/entry"
> >       ls "path/to/current/entry"
> 
> Is it an option to use "ls" for both cases and treat one-arg and two-arg
> cases differently?

I like that idea, its not hard to implement if we keep quoting mandatory.

-- David Barr.

---

Show changes to fast-import.c +3 −4

diff --git a/fast-import.c b/fast-import.c
index 854398a..0b0f2a1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2922,8 +2922,7 @@ static void parse_ls(struct branch *b)
 
 	/* ls SP <treeish> SP <path> */
 	p = command_buf.buf + strlen("ls ");
-	if(!prefixcmp(p, "index")) {
-		p += strlen("index");
+	if(*p == '"') {
 		if (!b)
 			die("Not in a commit: %s", command_buf.buf);
 		root = &b->branch_tree;
@@ -2932,9 +2931,9 @@ static void parse_ls(struct branch *b)
 		root = new_tree_entry();
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
 		load_tree(root);
+		if (*p++ != ' ')
+			die("Missing space after tree-ish: %s", command_buf.buf);
 	}
-	if (*p++ != ' ')
-		die("Missing space after SHA1: %s", command_buf.buf);
 	if (unquote_c_style(&uq, p, &p))
 		die("Invalid path: %s", command_buf.buf);
 	if (*p)

Jonathan Nieder· Jan 3, 2011, 08:01 UTC · re: David Barr · lore

[PATCH/RFC v2 0/3] fast-import: add 'ls' command

David Barr wrote:

> This patch is by no means complete - I still need to consider the edge cases.
> It does achieve the basic requirements for simplifying svn-fe.

It really does do that.  About time for a reroll.

Patches 1 and 2 are nearby fixes noticed while hacking at this. Changes in patch 3 from v1 will be mentioned in the same message as the patch.

Thoughts, improvements, especially tests welcome. Let's get this feature ready for wide use.

David Barr (1):
  fast-import: add 'ls' command

Jonathan Nieder (2):
  fast-import: clarify handling of cat-blob feature
  fast-import: treat filemodify with empty tree as delete

 Documentation/git-fast-import.txt |   49 ++++++++++-
 fast-import.c                     |  181 +++++++++++++++++++++++++++++++++++--
 t/t9300-fast-import.sh            |  158 ++++++++++++++++++++++++++++++--
 3 files changed, 371 insertions(+), 17 deletions(-)

-- 
1.7.4.rc0

Jonathan Nieder· Jan 3, 2011, 08:22 UTC · re: Jonathan Nieder · lore

[PATCH 1/3] fast-import: clarify handling of cat-blob feature

Date: Thu Dec 9 14:45:21 2010 -0600

Remove the undocumented --cat-blob command line option. It used to be a no-op.

While at it, move parsing of --cat-blob-fd to parse_one_feature; this makes the parse_argv loop a little easier to read and puts the code implementing 'feature cat-blob' and --cat-blob-fd closer to each other.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Forgot to mention: these are based against v1.7.4-rc0~24 (t9300: use
perl "head -c" clone in place of "dd bs=1 count=16000" kluge,
2010-12-13) but I wouldn't be surprised if they apply cleanly to other
commits, too. ;-)

 fast-import.c          |    9 +++------
 t/t9300-fast-import.sh |    9 +++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

Show changes to 2 files +12 −6

fast-import.c, t/t9300-fast-import.sh

diff --git a/fast-import.c b/fast-import.c
index 7857760..a5cea45 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2977,8 +2977,10 @@ static int parse_one_feature(const char *feature, int from_stream)
 		option_import_marks(feature + 13, from_stream);
 	} else if (!prefixcmp(feature, "export-marks=")) {
 		option_export_marks(feature + 13);
-	} else if (!strcmp(feature, "cat-blob")) {
+	} else if (from_stream && !strcmp(feature, "cat-blob")) {
 		; /* Don't die - this feature is supported */
+	} else if (!from_stream && !prefixcmp(feature, "cat-blob-fd=")) {
+		option_cat_blob_fd(feature + strlen("cat-blob-fd="));
 	} else if (!prefixcmp(feature, "relative-marks")) {
 		relative_marks_paths = 1;
 	} else if (!prefixcmp(feature, "no-relative-marks")) {
@@ -3073,11 +3075,6 @@ static void parse_argv(void)
 		if (parse_one_feature(a + 2, 0))
 			continue;
 
-		if (!prefixcmp(a + 2, "cat-blob-fd=")) {
-			option_cat_blob_fd(a + 2 + strlen("cat-blob-fd="));
-			continue;
-		}
-
 		die("unknown option %s", a);
 	}
 	if (i != global_argc)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 222d105..53aad51 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1769,10 +1769,19 @@ test_expect_success 'R: feature cat-blob supported' '
 	git fast-import
 '
 
+test_expect_success 'R: no command line option for cat-blob feature' '
+	test_must_fail git fast-import --cat-blob <empty
+'
+
 test_expect_success 'R: cat-blob-fd must be a nonnegative integer' '
 	test_must_fail git fast-import --cat-blob-fd=-1 </dev/null
 '
 
+test_expect_success 'R: cat-blob-fd cannot be specified in stream' '
+	echo "feature cat-blob-fd=1" |
+	test_must_fail git fast-import
+'
+
 test_expect_success 'R: print old blob' '
 	blob=$(echo "yes it can" | git hash-object -w --stdin) &&
 	cat >expect <<-EOF &&

-- 
1.7.4.rc0.580.g89dc.dirty

Jonathan Nieder· Jan 3, 2011, 08:24 UTC · re: Jonathan Nieder · lore

[PATCH 2/3] fast-import: treat filemodify with empty tree as delete

Date: Sat, 11 Dec 2010 16:42:28 -0600

Traditionally, git trees do not contain entries for empty subdirectories. Generally speaking, subtrees are not created or destroyed explicitly; instead, they automatically appear when needed to hold regular files, symlinks, and submodules.

v1.7.3-rc0~75^2 (Teach fast-import to import subtrees named by tree id, 2010-06-30) changed that, by allowing an empty subtree to be included in a fast-import stream explicitly:

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 subdir

That was unintentional. Better and more closely analogous to "git read-tree --prefix" to treat such an input line as a request to delete ("to empty") subdir.

Noticed-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
If this seems like a good idea it might be a candidate for v1.7.4.x.
Perhaps fsck.c should learn a "no empty trees" rule, too.

 fast-import.c          |   10 ++++++++
 t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 60 insertions(+), 8 deletions(-)

Show changes to 2 files +60 −8

fast-import.c, t/t9300-fast-import.sh

diff --git a/fast-import.c b/fast-import.c
index a5cea45..385d12d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2231,6 +2231,16 @@ static void file_change_m(struct branch *b)
 		p = uq.buf;
 	}
 
+	/*
+	 * Git does not track empty, non-toplevel directories.
+	 */
+	if (S_ISDIR(mode) &&
+	    !memcmp(sha1, (const unsigned char *) EMPTY_TREE_SHA1_BIN, 20) &&
+	    *p) {
+		tree_content_remove(&b->branch_tree, p, NULL);
+		return;
+	}
+
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
 			die("Git links cannot be specified 'inline': %s",
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 53aad51..b9aa3f0 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -42,6 +42,14 @@ echo "$@"'
 
 >empty
 
+test_expect_success 'setup: have pipes?' '
+	rm -f frob &&
+	if mkfifo frob
+	then
+		test_set_prereq PIPE
+	fi
+'
+
 ###
 ### series A
 ###
@@ -899,6 +907,48 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
+	'N: delete directory by copying' \
+	'cat >expect <<-\EOF &&
+	OBJID
+	:100644 000000 OBJID OBJID D	foo/bar/qux
+	OBJID
+	:000000 100644 OBJID OBJID A	foo/bar/baz
+	:000000 100644 OBJID OBJID A	foo/bar/qux
+	EOF
+	 empty_tree=$(git mktree </dev/null) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	collect data to be deleted
+	COMMIT
+
+	deleteall
+	M 100644 inline foo/bar/baz
+	data <<DATA_END
+	hello
+	DATA_END
+	C "foo/bar/baz" "foo/bar/qux"
+	C "foo/bar/baz" "foo/bar/quux/1"
+	C "foo/bar/baz" "foo/bar/quuux"
+	M 040000 $empty_tree foo/bar/quux
+	M 040000 $empty_tree foo/bar/quuux
+
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	delete subdirectory
+	COMMIT
+
+	M 040000 $empty_tree foo/bar/qux
+	INPUT_END
+	 git fast-import <input &&
+	 git rev-list N-delete |
+		git diff-tree -r --stdin --root --always |
+		sed -e "s/$_x40/OBJID/g" >actual &&
+	 test_cmp expect actual'
+
+test_expect_success \
 	'N: copy root directory by tree hash' \
 	'cat >expect <<-\EOF &&
 	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf
@@ -1898,14 +1948,6 @@ test_expect_success 'R: print two blobs to stdout' '
 	test_cmp expect actual
 '
 
-test_expect_success 'setup: have pipes?' '
-	rm -f frob &&
-	if mkfifo frob
-	then
-		test_set_prereq PIPE
-	fi
-'
-
 test_expect_success PIPE 'R: copy using cat-file' '
 	expect_id=$(git hash-object big) &&
 	expect_len=$(wc -c <big) &&

-- 
1.7.4.rc0.580.g89dc.dirty

Jonathan Nieder· Jan 26, 2011, 22:41 UTC · re: Jonathan Nieder · lore

[PATCH v2] fast-import: treat filemodify with empty tree as delete

Date: Sat, 11 Dec 2010 16:42:28 -0600

Normal git processes do not allow one to build a tree with an empty subtree entry without trying hard at it. This is in keeping with the general UI philosophy: git tracks content, not empty directories.

v1.7.3-rc0~75^2 (2010-06-30) changed that by making it easy to include an empty subtree in fast-import's active commit:

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 subdir

It is easy to trigger this by accident by reading an empty tree (for example, the tree corresponding to an empty root commit) and trying to move it to a subtree. It is better and more closely analogous to "git read-tree --prefix" to treat such commands as a request to remove ("to empty") the subdir.

Noticed-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Change since v1: commit message.

Resubmitting this fix separately from the 3-part series it came from. Seems to work okay. :)

 fast-import.c          |   10 ++++++++
 t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 60 insertions(+), 8 deletions(-)

Show changes to 2 files +60 −8

fast-import.c, t/t9300-fast-import.sh

diff --git a/fast-import.c b/fast-import.c
index 7857760..8b19d87 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2231,6 +2231,16 @@ static void file_change_m(struct branch *b)
 		p = uq.buf;
 	}
 
+	/*
+	 * Git does not track empty, non-toplevel directories.
+	 */
+	if (S_ISDIR(mode) &&
+	    !memcmp(sha1, (const unsigned char *) EMPTY_TREE_SHA1_BIN, 20) &&
+	    *p) {
+		tree_content_remove(&b->branch_tree, p, NULL);
+		return;
+	}
+
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
 			die("Git links cannot be specified 'inline': %s",
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 222d105..80ddfe0 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -42,6 +42,14 @@ echo "$@"'
 
 >empty
 
+test_expect_success 'setup: have pipes?' '
+	rm -f frob &&
+	if mkfifo frob
+	then
+		test_set_prereq PIPE
+	fi
+'
+
 ###
 ### series A
 ###
@@ -899,6 +907,48 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
+test_expect_success \
+	'N: delete directory by copying' \
+	'cat >expect <<-\EOF &&
+	OBJID
+	:100644 000000 OBJID OBJID D	foo/bar/qux
+	OBJID
+	:000000 100644 OBJID OBJID A	foo/bar/baz
+	:000000 100644 OBJID OBJID A	foo/bar/qux
+	EOF
+	 empty_tree=$(git mktree </dev/null) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	collect data to be deleted
+	COMMIT
+
+	deleteall
+	M 100644 inline foo/bar/baz
+	data <<DATA_END
+	hello
+	DATA_END
+	C "foo/bar/baz" "foo/bar/qux"
+	C "foo/bar/baz" "foo/bar/quux/1"
+	C "foo/bar/baz" "foo/bar/quuux"
+	M 040000 $empty_tree foo/bar/quux
+	M 040000 $empty_tree foo/bar/quuux
+
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	delete subdirectory
+	COMMIT
+
+	M 040000 $empty_tree foo/bar/qux
+	INPUT_END
+	 git fast-import <input &&
+	 git rev-list N-delete |
+		git diff-tree -r --stdin --root --always |
+		sed -e "s/$_x40/OBJID/g" >actual &&
+	 test_cmp expect actual'
+
 test_expect_success \
 	'N: copy root directory by tree hash' \
 	'cat >expect <<-\EOF &&
 	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf
@@ -1889,14 +1939,6 @@ test_expect_success 'R: print two blobs to stdout' '
 	test_cmp expect actual
 '
 
-test_expect_success 'setup: have pipes?' '
-	rm -f frob &&
-	if mkfifo frob
-	then
-		test_set_prereq PIPE
-	fi
-'
-
 test_expect_success PIPE 'R: copy using cat-file' '
 	expect_id=$(git hash-object big) &&
 	expect_len=$(wc -c <big) &&

-- 
1.7.4.rc3

Sverre Rabbelier· Jan 26, 2011, 22:45 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] fast-import: treat filemodify with empty tree as delete

Heya,

On Wed, Jan 26, 2011 at 23:41, Jonathan Nieder <jrnieder@gmail.com> wrote:

> Seems to work okay. :)

Should it go on maint now that it's factored out, since it shipped in 1.7.3, or just master?

-- 
Cheers,

Sverre Rabbelier

Jonathan Nieder· Jan 26, 2011, 23:06 UTC · re: Sverre Rabbelier · lore

[PATCH jn/fast-import-fix v3] fast-import: treat filemodify with empty tree as delete

Date: Sat, 11 Dec 2010 16:42:28 -0600

Unfortunately, v1.7.3-rc0~75^2 (2010-06-30) changed that by making it easy to include an empty subtree in fast-import's active commit:

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 subdir

It is easy to trigger this by accident by reading an empty tree (for example, the tree corresponding to an empty root commit) and trying to move it to a subtree. It would be better and more closely analogous to "git read-tree --prefix" to treat such commands as a request to remove ("to empty") the subdir.

Noticed-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sverre Rabbelier wrote:

> Should it go on maint now that it's factored out, since it shipped in
> 1.7.3, or just master?

Hmm.  I suppose on top of b2124125 (jn/fast-import-fix).

While applying it there I noticed that the change to t9300 includes an unrelated change (residue of an old rebase). Here's a fixed version.

 fast-import.c          |   10 ++++++++++
 t/t9300-fast-import.sh |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 0 deletions(-)

Show changes to 2 files +52 −0

fast-import.c, t/t9300-fast-import.sh

diff --git a/fast-import.c b/fast-import.c
index d881630..9cf26f1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2194,6 +2194,16 @@ static void file_change_m(struct branch *b)
 		p = uq.buf;
 	}
 
+	/*
+	 * Git does not track empty, non-toplevel directories.
+	 */
+	if (S_ISDIR(mode) &&
+	    !memcmp(sha1, (const unsigned char *) EMPTY_TREE_SHA1_BIN, 20) &&
+	    *p) {
+		tree_content_remove(&b->branch_tree, p, NULL);
+		return;
+	}
+
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
 			die("Git links cannot be specified 'inline': %s",
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index dd90a09..ef3a347 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -875,6 +875,48 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
+test_expect_success \
+	'N: delete directory by copying' \
+	'cat >expect <<-\EOF &&
+	OBJID
+	:100644 000000 OBJID OBJID D	foo/bar/qux
+	OBJID
+	:000000 100644 OBJID OBJID A	foo/bar/baz
+	:000000 100644 OBJID OBJID A	foo/bar/qux
+	EOF
+	 empty_tree=$(git mktree </dev/null) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	collect data to be deleted
+	COMMIT
+
+	deleteall
+	M 100644 inline foo/bar/baz
+	data <<DATA_END
+	hello
+	DATA_END
+	C "foo/bar/baz" "foo/bar/qux"
+	C "foo/bar/baz" "foo/bar/quux/1"
+	C "foo/bar/baz" "foo/bar/quuux"
+	M 040000 $empty_tree foo/bar/quux
+	M 040000 $empty_tree foo/bar/quuux
+
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	delete subdirectory
+	COMMIT
+
+	M 040000 $empty_tree foo/bar/qux
+	INPUT_END
+	 git fast-import <input &&
+	 git rev-list N-delete |
+		git diff-tree -r --stdin --root --always |
+		sed -e "s/$_x40/OBJID/g" >actual &&
+	 test_cmp expect actual'
+
 test_expect_success \
 	'N: copy root directory by tree hash' \
 	'cat >expect <<-\EOF &&
 	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf

-- 
1.7.4.rc3

Junio C Hamano· Jan 27, 2011, 00:04 UTC · re: Jonathan Nieder · lore

Re: [PATCH jn/fast-import-fix v3] fast-import: treat filemodify with empty tree as delete

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 6 quoted lines

> Sverre Rabbelier wrote:
>
>> Should it go on maint now that it's factored out, since it shipped in
>> 1.7.3, or just master?
>
> Hmm.  I suppose on top of b2124125 (jn/fast-import-fix).

Hmm, why not on top of v1.7.3-rc0~75^2 aka 334fba6 (Teach fast-import to import subtrees named by tree id, 2010-06-30) then?

> While applying it there I noticed that the change to t9300 includes an
> unrelated change (residue of an old rebase).  Here's a fixed version.

Show 13 quoted lines

> diff --git a/fast-import.c b/fast-import.c
> index d881630..9cf26f1 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2194,6 +2194,16 @@ static void file_change_m(struct branch *b)
>  		p = uq.buf;
>  	}
>  
> +	/*
> +	 * Git does not track empty, non-toplevel directories.
> +	 */
> +	if (S_ISDIR(mode) &&
> +	    !memcmp(sha1, (const unsigned char *) EMPTY_TREE_SHA1_BIN, 20) &&

Do you need this cast?

Jonathan Nieder· Jan 27, 2011, 00:26 UTC · re: Junio C Hamano · lore

Re: [PATCH jn/fast-import-fix v3] fast-import: treat filemodify with empty tree as delete

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Hmm.  I suppose on top of b2124125 (jn/fast-import-fix).
>
> Hmm, why not on top of v1.7.3-rc0~75^2 aka 334fba6 (Teach fast-import to
> import subtrees named by tree id, 2010-06-30) then?

That could work, too. ;-)

I was too lazy to check if the test case happens to work in the absence of the fixes from the fast-import-fix branch.

Show 12 quoted lines

>> +++ b/fast-import.c
>> @@ -2194,6 +2194,16 @@ static void file_change_m(struct branch *b)
>>  		p = uq.buf;
>>  	}
>>  
>> +	/*
>> +	 * Git does not track empty, non-toplevel directories.
>> +	 */
>> +	if (S_ISDIR(mode) &&
>> +	    !memcmp(sha1, (const unsigned char *) EMPTY_TREE_SHA1_BIN, 20) &&
>
> Do you need this cast?

No, it's not needed.

(EMPTY_TREE_SHA1_BIN is a string constant, originally intended for use in initializers like

	static const unsigned char empty_tree_sha1[20] = EMPTY_TREE_SHA1_BIN;

memcmp does not care about such considerations.)

Jonathan Nieder· Jan 27, 2011, 06:07 UTC · re: Junio C Hamano · lore

[PATCH v4] fast-import: treat filemodify with empty tree as delete

v1.7.3-rc0~75^2 (2010-06-30) changed that by making it easy to include an empty subtree in fast-import's active commit:

	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 subdir

One can trigger this by reading an empty tree (for example, the tree corresponding to an empty root commit) and trying to move it to a subtree. It is better and more closely analogous to 'git read-tree --prefix' to treat such commands as requests to remove the subtree.

Noticed-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> Hmm, why not on top of v1.7.3-rc0~75^2 aka 334fba6 (Teach fast-import to
> import subtrees named by tree id, 2010-06-30) then?

Okay, I found time to try it. Some other small simplifications while at it.

 fast-import.c          |    6 ++++++
 t/t9300-fast-import.sh |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

Show changes to 2 files +48 −0

fast-import.c, t/t9300-fast-import.sh

diff --git a/fast-import.c b/fast-import.c
index ad6843a..cd9310d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2163,6 +2163,12 @@ static void file_change_m(struct branch *b)
 		p = uq.buf;
 	}
 
+	/* Git does not track empty, non-toplevel directories. */
+	if (S_ISDIR(mode) && !memcmp(sha1, EMPTY_TREE_SHA1_BIN, 20) && *p) {
+		tree_content_remove(&b->branch_tree, p, NULL);
+		return;
+	}
+
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
 			die("Git links cannot be specified 'inline': %s",
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 50d5913..8487734 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -818,6 +818,48 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
+test_expect_success \
+	'N: delete directory by copying' \
+	'cat >expect <<-\EOF &&
+	OBJID
+	:100644 000000 OBJID OBJID D	foo/bar/qux
+	OBJID
+	:000000 100644 OBJID OBJID A	foo/bar/baz
+	:000000 100644 OBJID OBJID A	foo/bar/qux
+	EOF
+	 empty_tree=$(git mktree </dev/null) &&
+	 cat >input <<-INPUT_END &&
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	collect data to be deleted
+	COMMIT
+
+	deleteall
+	M 100644 inline foo/bar/baz
+	data <<DATA_END
+	hello
+	DATA_END
+	C "foo/bar/baz" "foo/bar/qux"
+	C "foo/bar/baz" "foo/bar/quux/1"
+	C "foo/bar/baz" "foo/bar/quuux"
+	M 040000 $empty_tree foo/bar/quux
+	M 040000 $empty_tree foo/bar/quuux
+
+	commit refs/heads/N-delete
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	delete subdirectory
+	COMMIT
+
+	M 040000 $empty_tree foo/bar/qux
+	INPUT_END
+	 git fast-import <input &&
+	 git rev-list N-delete |
+		git diff-tree -r --stdin --root --always |
+		sed -e "s/$_x40/OBJID/g" >actual &&
+	 test_cmp expect actual'
+
 test_expect_success \
 	'N: modify copied tree' \
 	'cat >expect <<-\EOF &&
 	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/file5

-- 
1.7.4.rc3

Peter Baumann· Jan 27, 2011, 19:33 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4] fast-import: treat filemodify with empty tree as delete

On Thu, Jan 27, 2011 at 12:07:49AM -0600, Jonathan Nieder wrote:

Show 17 quoted lines

> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index 50d5913..8487734 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -818,6 +818,48 @@ test_expect_success \
>  	 compare_diff_raw expect actual'
>  
> +test_expect_success \
> +	'N: delete directory by copying' \
> +	'cat >expect <<-\EOF &&
> +	OBJID
> +	:100644 000000 OBJID OBJID D	foo/bar/qux
> +	OBJID
> +	:000000 100644 OBJID OBJID A	foo/bar/baz
> +	:000000 100644 OBJID OBJID A	foo/bar/qux
> +	EOF
> +	 empty_tree=$(git mktree </dev/null) &&

[ Feel free to ignore me ... ]

Just a (stupid?) suggestion: Why not put a $EMPTY_TREE definiton in test-lib.sh (or any other global file sourced in the tests) so if another caller needs this definition it won't waste cpu cycles doing the calculation via mktree < /dev/null again?

Jonathan Nieder· Jan 27, 2011, 19:48 UTC · re: Peter Baumann · lore

Re: [PATCH v4] fast-import: treat filemodify with empty tree as delete

Peter Baumann wrote:

> On Thu, Jan 27, 2011 at 12:07:49AM -0600, Jonathan Nieder wrote:

Show 21 quoted lines

>> +++ b/t/t9300-fast-import.sh
>> @@ -818,6 +818,48 @@ test_expect_success \
>>  	 compare_diff_raw expect actual'
>>  
>> +test_expect_success \
>> +	'N: delete directory by copying' \
>> +	'cat >expect <<-\EOF &&
>> +	OBJID
>> +	:100644 000000 OBJID OBJID D	foo/bar/qux
>> +	OBJID
>> +	:000000 100644 OBJID OBJID A	foo/bar/baz
>> +	:000000 100644 OBJID OBJID A	foo/bar/qux
>> +	EOF
>> +	 empty_tree=$(git mktree </dev/null) &&
>
> [ Feel free to ignore me ... ]
>
> Just a (stupid?) suggestion: Why not put a $EMPTY_TREE definiton in test-lib.sh
> (or any other global file sourced in the tests) so if another caller needs this
> definition it won't waste cpu cycles doing the calculation via mktree < /dev/null
> again?

Might be a good idea. Note, though, that that would mean more cpu cycles used rather than less, unless we hardcode the object name (which I prefer not to do).

One possibility would be a lib-object-names.sh defining EMPTY_BLOB and EMPTY_TREE to be sourced by tests that need it.

Peter Baumann· Jan 27, 2011, 20:46 UTC · re: Jonathan Nieder · lore

Re: [PATCH v4] fast-import: treat filemodify with empty tree as delete

On Thu, Jan 27, 2011 at 01:48:45PM -0600, Jonathan Nieder wrote:

Show 29 quoted lines

> Peter Baumann wrote:
> > On Thu, Jan 27, 2011 at 12:07:49AM -0600, Jonathan Nieder wrote:
> 
> >> +++ b/t/t9300-fast-import.sh
> >> @@ -818,6 +818,48 @@ test_expect_success \
> >>  	 compare_diff_raw expect actual'
> >>  
> >> +test_expect_success \
> >> +	'N: delete directory by copying' \
> >> +	'cat >expect <<-\EOF &&
> >> +	OBJID
> >> +	:100644 000000 OBJID OBJID D	foo/bar/qux
> >> +	OBJID
> >> +	:000000 100644 OBJID OBJID A	foo/bar/baz
> >> +	:000000 100644 OBJID OBJID A	foo/bar/qux
> >> +	EOF
> >> +	 empty_tree=$(git mktree </dev/null) &&
> >
> > [ Feel free to ignore me ... ]
> >
> > Just a (stupid?) suggestion: Why not put a $EMPTY_TREE definiton in test-lib.sh
> > (or any other global file sourced in the tests) so if another caller needs this
> > definition it won't waste cpu cycles doing the calculation via mktree < /dev/null
> > again?
> 
> Might be a good idea.  Note, though, that that would mean more cpu
> cycles used rather than less, unless we hardcode the object name
> (which I prefer not to do).
>

Wny not? It *is* already hardcoded in the GIT source code (see grep -a1 cache.h output).

> One possibility would be a lib-object-names.sh defining EMPTY_BLOB and
> EMPTY_TREE to be sourced by tests that need it.

Hm. Might be a possibility, but if this file only contains 2 hardcoded variables I would prefer putting it into test-list.sh instead of an extra file.

Peter Baumann· Jan 27, 2011, 20:48 UTC · re: Peter Baumann · lore

Re: [PATCH v4] fast-import: treat filemodify with empty tree as delete

On Thu, Jan 27, 2011 at 09:46:50PM +0100, Peter Baumann wrote:

Show 32 quoted lines

> On Thu, Jan 27, 2011 at 01:48:45PM -0600, Jonathan Nieder wrote:
> > Peter Baumann wrote:
> > > On Thu, Jan 27, 2011 at 12:07:49AM -0600, Jonathan Nieder wrote:
> > 
> > >> +++ b/t/t9300-fast-import.sh
> > >> @@ -818,6 +818,48 @@ test_expect_success \
> > >>  	 compare_diff_raw expect actual'
> > >>  
> > >> +test_expect_success \
> > >> +	'N: delete directory by copying' \
> > >> +	'cat >expect <<-\EOF &&
> > >> +	OBJID
> > >> +	:100644 000000 OBJID OBJID D	foo/bar/qux
> > >> +	OBJID
> > >> +	:000000 100644 OBJID OBJID A	foo/bar/baz
> > >> +	:000000 100644 OBJID OBJID A	foo/bar/qux
> > >> +	EOF
> > >> +	 empty_tree=$(git mktree </dev/null) &&
> > >
> > > [ Feel free to ignore me ... ]
> > >
> > > Just a (stupid?) suggestion: Why not put a $EMPTY_TREE definiton in test-lib.sh
> > > (or any other global file sourced in the tests) so if another caller needs this
> > > definition it won't waste cpu cycles doing the calculation via mktree < /dev/null
> > > again?
> > 
> > Might be a good idea.  Note, though, that that would mean more cpu
> > cycles used rather than less, unless we hardcode the object name
> > (which I prefer not to do).
> > 
> 
> Wny not? It *is* already hardcoded in the GIT source code (see grep -a1   cache.h

                                                                          ^
                                                                        EMPTY

Show 12 quoted lines

> output).
> 
> > One possibility would be a lib-object-names.sh defining EMPTY_BLOB and
> > EMPTY_TREE to be sourced by tests that need it.
> 
> Hm. Might be a possibility, but if this file only contains 2 hardcoded variables
> I would prefer putting it into test-list.sh instead of an extra file.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Jonathan Nieder· Jan 28, 2011, 17:13 UTC · re: Peter Baumann · lore

Re: [PATCH v4] fast-import: treat filemodify with empty tree as delete

Peter Baumann wrote:

> On Thu, Jan 27, 2011 at 01:48:45PM -0600, Jonathan Nieder wrote:
>>> On Thu, Jan 27, 2011 at 12:07:49AM -0600, Jonathan Nieder wrote:

>>>> +	 empty_tree=$(git mktree </dev/null) &&

[...]

Show 5 quoted lines

>>                               unless we hardcode the object name
>> (which I prefer not to do).
>
> Wny not? It *is* already hardcoded in the GIT source code (see
> grep -a1 EMPTY cache.h output).

I think it is okay for the git implementation to rely on an implementation detail. ;-) Likewise, t0000 checks that the empty tree has id 4b825dc6. Meanwhile I would like to see people's scripts and other tests using the $(git mktree </dev/null) form, since it is more self-explanatory and avoids hardcoding an implementation detail.

Of course this is not an absolute thing.

Hope that helps, Jonathan

Further reading: t0000-basic.h --help:

	Note that this test *deliberately* hard-codes many expected object
	IDs.  When object ID computation changes, like in the previous case of
	swapping compression and hashing order, the person who is making the
	modification *should* take notice and update the test vectors here.

"Tips for Writing Tests" in t/README:

	However, other tests that simply rely on basic parts of the core
	GIT working properly should not have that level of intimate
	knowledge of the core GIT internals.  If all the test scripts
	hardcoded the object IDs like t0000-basic.sh does, that defeats
	the purpose of t0000-basic.sh, which is to isolate that level of
	validation in one place.  Your test also ends up needing
	updating when such a change to the internal happens, so do _not_
	do it and leave the low level of validation to t0000-basic.sh.

Jonathan Nieder· Jan 3, 2011, 08:37 UTC · re: Jonathan Nieder · lore

[PATCH 3/3] fast-import: add 'ls' command

From: David Barr <david.barr@cordelta.com>
Date: Thu, 2 Dec 2010 21:40:20 +1100

The vcs-svn library currently maintains an in-core index of all paths in all revisions. Introducing an `ls` command to fast-import would allow this responsibility to be delegated; and reading this information from the target repository instead of an in-core data structure would result in support for resuming an import partway through (i.e., incremental imports) for free.

There are two forms of the 'ls' command: the two-argument form prints the entry at <path> for the tree underlying the tree, commit, or tag named by <dataref>:

	'ls' SP <dataref> SP <path> LF

The one-argument form prints the entry at <path> in fast-import's active commit.

	'ls' SP <path> LF

Output uses ls-tree format.

Dirty hack: missing paths are assumed to represent the empty subtree and are printed as

 040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904	path/to/nowhere

to avoid confusing frontends that inserted such a path before. But frontends should also be prepared to accept

 missing path/to/nowhere

from backends that (unlike git) distinguish between empty subtrees and nonentities.

Signed-off-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
The empty tree handling is an evil hack.  One of the tests illustrates
the kind of operation this is meant to support.  It would be easy to
convince me that some other evil hack is better.

This doesn't have tests for the basic functionality. Maybe there should be a new t9302-fast-import-bidi.sh so there is less to read to get started?

No new "feature" for this. Frontends can easily make a feature test for themselves if they need it. ;-) And I still have plans for "feature command ls" et al, as part of a series including Tomas's simplified command dispatch.

Only compile tested. (Something similar to this is very well tested but that is not enough to prevent accidents.)

Changes from v1:
 - new documentation and demo (tests)
 - refactored peel-to-tree routines
 - mode is always 6 digits
 - path output uses quoting (especially important for filenames
   with \n [though that wouldn't come up in the svn-fe case])
 - persistent buffers to avoid allocation overhead
 - the empty tree hackery
 - mode is based on type, not based on extracting the object itself
 - path after <dataref> does not have to be quoted
 - no-<dataref> form is 'ls "<path>"' instead of 'ls index "<path>"'

Thanks for the original patch and a lot of help improving it go to David.

'night, Jonathan

 Documentation/git-fast-import.txt |   49 +++++++++++-
 fast-import.c                     |  162 ++++++++++++++++++++++++++++++++++++-
 t/t9300-fast-import.sh            |   91 +++++++++++++++++++++
 3 files changed, 299 insertions(+), 3 deletions(-)

Show changes to 3 files +299 −3

Documentation/git-fast-import.txt, fast-import.c, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f56dfca..3957f70 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -192,7 +192,8 @@ especially when a higher level language such as Perl, Python or
 Ruby is being used.
 
 fast-import is very strict about its input.  Where we say SP below we mean
-*exactly* one space.  Likewise LF means one (and only one) linefeed.
+*exactly* one space.  Likewise LF means one (and only one) linefeed
+and HT one (and only one) horizontal tab.
 Supplying additional whitespace characters will cause unexpected
 results, such as branch names or file names with leading or trailing
 spaces in their name, or early termination of fast-import when it encounters
@@ -330,6 +331,11 @@ and control the current import process.  More detailed discussion
 	format to the file descriptor set with `--cat-blob-fd` or
 	`stdout` if unspecified.
 
+`ls`::
+	Causes fast-import to print a directory entry in 'ls-tree'
+	format to the file descriptor set with `--cat-blob-fd` or
+	`stdout` if unspecified.
+
 `feature`::
 	Require that fast-import supports the specified feature, or
 	abort if it does not.
@@ -916,6 +922,47 @@ This command can be used anywhere in the stream that comments are
 accepted.  In particular, the `cat-blob` command can be used in the
 middle of a commit but not in the middle of a `data` command.
 
+`ls`
+~~~~
+Prints a directory entry to a file descriptor previously arranged with
+the `--cat-blob-fd` argument.  In the current implementation, if that
+entry represents a subdirectory in the current commit, it will be
+stored in the object database, but it is not advisable to rely on this
+detail since it maybe change.
+
+....
+	'ls' (SP <dataref>)? SP <path> LF
+....
+
+The `<dataref>` can be either a mark reference (`:<idnum>`) or a full
+40-byte SHA-1 of a Git tag, commit, or tree object, preexisting or
+waiting to be written.  The directory entry printed is that named by
+the path, relative to the top level of that tree.
+
+The `ls` command can be used anywhere in the stream that comments are
+accepted, including the middle of a commit.
+
+In the middle of a `commit`, the `<dataref>` part of the command can
+be omitted, in which case the path names a directory entry within
+fast-import's active commit.  The path must be quoted in this case.
+
+Output uses the same format as `git ls-tree <tree> -- <path>`:
+
+====
+	<mode> SP ('blob' | 'tree') SP <dataref> HT <path> LF
+====
+
+Since git repositories do not distinguish between missing paths and
+empty subtrees, if a path is not found it will be reported as an
+empty tree.  Backends for version control systems that do have a
+notion of empty trees may write
+
+====
+	missing SP <path> LF
+====
+
+for paths that do not correspond to a blob or subtree.
+
 `feature`
 ~~~~~~~~~
 Require that fast-import supports the specified feature, or abort if
diff --git a/fast-import.c b/fast-import.c
index 385d12d..21cb109 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -24,10 +24,12 @@ Format of STDIN stream:
     commit_msg
     ('from' sp committish lf)?
     ('merge' sp committish lf)*
-    file_change*
+    (file_change | ls)*
     lf?;
   commit_msg ::= data;
 
+  ls ::= 'ls' sp '"' quoted(path) '"' lf;
+
   file_change ::= file_clr
     | file_del
     | file_rnm
@@ -132,7 +134,7 @@ Format of STDIN stream:
   ts    ::= # time since the epoch in seconds, ascii base10 notation;
   tz    ::= # GIT style timezone;
 
-     # note: comments and cat requests may appear anywhere
+     # note: comments, ls and cat requests may appear anywhere
      # in the input, except within a data command.  Any form
      # of the data command always escapes the related input
      # from comment processing.
@@ -141,7 +143,9 @@ Format of STDIN stream:
      # must be the first character on that line (an lf
      # preceded it).
      #
+
   cat_blob ::= 'cat-blob' sp (hexsha1 | idnum) lf;
+  ls_tree  ::= 'ls' sp (hexsha1 | idnum) sp path_str lf;
 
   comment ::= '#' not_lf* lf;
   not_lf  ::= # Any byte that is not ASCII newline (LF);
@@ -373,6 +377,7 @@ static int cat_blob_fd = STDOUT_FILENO;
 
 static void parse_argv(void);
 static void parse_cat_blob(void);
+static void parse_ls(struct branch *b);
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -2613,6 +2618,8 @@ static void parse_new_commit(void)
 			note_change_n(b, prev_fanout);
 		else if (!strcmp("deleteall", command_buf.buf))
 			file_change_deleteall(b);
+		else if (!prefixcmp(command_buf.buf, "ls "))
+			parse_ls(b);
 		else {
 			unread_command_buf = 1;
 			break;
@@ -2836,6 +2843,155 @@ static void parse_cat_blob(void)
 	cat_blob(oe, sha1);
 }
 
+static struct object_entry *dereference(struct object_entry *oe,
+					unsigned char sha1[20])
+{
+	unsigned long size;
+	void *buf = NULL;
+	if (!oe) {
+		enum object_type type = sha1_object_info(sha1, NULL);
+		if (type < 0)
+			die("object not found: %s", sha1_to_hex(sha1));
+		/* cache it! */
+		oe = insert_object(sha1);
+		oe->type = type;
+		oe->pack_id = MAX_PACK_ID;
+		oe->idx.offset = 1;
+	}
+	switch (oe->type) {
+	case OBJ_TREE:	/* easy case. */
+		return oe;
+	case OBJ_COMMIT:
+	case OBJ_TAG:
+		break;
+	default:
+		die("Not a treeish: %s", command_buf.buf);
+	}
+
+	if (oe->pack_id != MAX_PACK_ID) {	/* in a pack being written */
+		buf = gfi_unpack_entry(oe, &size);
+	} else {
+		enum object_type unused;
+		buf = read_sha1_file(sha1, &unused, &size);
+	}
+	if (!buf)
+		die("Can't load object %s", sha1_to_hex(sha1));
+
+	/* Peel one layer. */
+	switch (oe->type) {
+	case OBJ_TAG:
+		if (size < 40 + strlen("object ") ||
+		    get_sha1_hex(buf + strlen("object "), sha1))
+			die("Invalid SHA1 in tag: %s", command_buf.buf);
+		break;
+	case OBJ_COMMIT:
+		if (size < 40 + strlen("tree ") ||
+		    get_sha1_hex(buf + strlen("tree "), sha1))
+			die("Invalid SHA1 in commit: %s", command_buf.buf);
+	}
+
+	free(buf);
+	return find_object(sha1);
+}
+
+static struct object_entry *parse_treeish_dataref(const char **p)
+{
+	unsigned char sha1[20];
+	struct object_entry *e;
+
+	if (**p == ':') {	/* <mark> */
+		char *endptr;
+		e = find_mark(strtoumax(*p + 1, &endptr, 10));
+		if (endptr == *p + 1)
+			die("Invalid mark: %s", command_buf.buf);
+		if (!e)
+			die("Unknown mark: %s", command_buf.buf);
+		*p = endptr;
+		hashcpy(sha1, e->idx.sha1);
+	} else {	/* <sha1> */
+		if (get_sha1_hex(*p, sha1))
+			die("Invalid SHA1: %s", command_buf.buf);
+		e = find_object(sha1);
+		*p += 40;
+	}
+
+	while (!e || e->type != OBJ_TREE)
+		e = dereference(e, sha1);
+	return e;
+}
+
+static void print_ls(int mode, const unsigned char *sha1, const char *path)
+{
+	static struct strbuf line = STRBUF_INIT;
+
+	/* See show_tree(). */
+	const char *type =
+		S_ISGITLINK(mode) ? commit_type :
+		S_ISDIR(mode) ? tree_type :
+		blob_type;
+
+	/* mode SP type SP object_name TAB path LF */
+	strbuf_reset(&line);
+	strbuf_addf(&line, "%06o %s %s\t",
+			mode, type, sha1_to_hex(sha1));
+	quote_c_style(path, &line, NULL, 0);
+	strbuf_addch(&line, '\n');
+	cat_blob_write(line.buf, line.len);
+}
+
+static void parse_ls(struct branch *b)
+{
+	const char *p;
+	struct tree_entry *root = NULL;
+	struct tree_entry leaf = {0};
+
+	/* ls SP (<treeish> SP)? <path> */
+	p = command_buf.buf + strlen("ls ");
+	if (*p == '"') {
+		if (!b)
+			die("Not in a commit: %s", command_buf.buf);
+		root = &b->branch_tree;
+	} else {
+		struct object_entry *e = parse_treeish_dataref(&p);
+		root = new_tree_entry();
+		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		load_tree(root);
+		if (*p++ != ' ')
+			die("Missing space after tree-ish: %s", command_buf.buf);
+	}
+	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;
+	}
+	tree_content_get(root, p, &leaf);
+	if (!leaf.versions[1].mode) {
+		/*
+		 * Missing path?  Must be an empty subtree!
+		 *
+		 * When git learns to track empty directories, we can report
+		 * this by saying 'missing "path/to/directory"' instead.
+		 */
+		print_ls(S_IFDIR, (const unsigned char *) EMPTY_TREE_SHA1_BIN, p);
+	} else {
+		/*
+		 * A directory in preparation would have a sha1 of zero
+		 * until it is saved.  Save, for simplicity.
+		 */
+		if (S_ISDIR(leaf.versions[1].mode))
+			store_tree(&leaf);
+
+		print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
+	}
+	if (!b || root != &b->branch_tree)
+		release_tree_entry(root);
+}
+
 static void checkpoint(void)
 {
 	checkpoint_requested = 0;
@@ -3131,6 +3287,8 @@ int main(int argc, const char **argv)
 	while (read_next_command() != EOF) {
 		if (!strcmp("blob", command_buf.buf))
 			parse_new_blob();
+		else if (!prefixcmp(command_buf.buf, "ls "))
+			parse_ls(NULL);
 		else if (!prefixcmp(command_buf.buf, "commit "))
 			parse_new_commit();
 		else if (!prefixcmp(command_buf.buf, "tag "))
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index b9aa3f0..6842b1f 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -906,6 +906,97 @@ test_expect_success \
 	 git diff-tree -C --find-copies-harder -r N4^ N4 >actual &&
 	 compare_diff_raw expect actual'
 
+test_expect_success PIPE 'N: read and copy directory' '
+	cat >expect <<-\EOF
+	:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	file3/newf
+	:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	file3/oldf
+	EOF
+	git update-ref -d refs/heads/N4 &&
+	rm -f backflow &&
+	mkfifo backflow &&
+	(
+		exec <backflow &&
+		cat <<-EOF &&
+		commit refs/heads/N4
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy by tree hash, part 2
+		COMMIT
+
+		from refs/heads/branch^0
+		ls "file2"
+		EOF
+		read mode type tree filename &&
+		echo "M 040000 $tree file3"
+	) |
+	git fast-import --cat-blob-fd=3 3>backflow &&
+	git diff-tree -C --find-copies-harder -r N4^ N4 >actual &&
+	compare_diff_raw expect actual
+'
+
+test_expect_success PIPE 'N: read and copy "empty" directory' '
+	cat <<-\EOF >expect &&
+	OBJNAME
+	:000000 100644 OBJNAME OBJNAME A	greeting
+	OBJNAME
+	:100644 000000 OBJNAME OBJNAME D	unrelated
+	OBJNAME
+	:000000 100644 OBJNAME OBJNAME A	unrelated
+	EOF
+	git update-ref -d refs/heads/copy-empty &&
+	rm -f backflow &&
+	mkfifo backflow &&
+	(
+		exec <backflow &&
+		cat <<-EOF &&
+		commit refs/heads/copy-empty
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy "empty" (missing) directory
+		COMMIT
+
+		M 100644 inline src/greeting
+		data <<BLOB
+		hello
+		BLOB
+		C src/greeting dst1/non-greeting
+		C src/greeting unrelated
+		# leave behind "empty" src directory
+		D src/greeting
+		ls "src"
+		EOF
+		read mode type tree filename &&
+		sed -e "s/X\$//" <<-EOF
+		M $mode $tree dst1
+		M $mode $tree dst2
+
+		commit refs/heads/copy-empty
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy empty directory to root
+		COMMIT
+
+		M $mode $tree X
+
+		commit refs/heads/copy-empty
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		add another file
+		COMMIT
+
+		M 100644 inline greeting
+		data <<BLOB
+		hello
+		BLOB
+		EOF
+	) |
+	git fast-import --cat-blob-fd=3 3>backflow &&
+	git rev-list copy-empty |
+	git diff-tree -r --root --stdin |
+	sed "s/$_x40/OBJNAME/g" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success \
 	'N: delete directory by copying' \
 	'cat >expect <<-\EOF &&

-- 
1.7.4.rc0.580.g89dc.dirty

Jonathan Nieder· Jan 26, 2011, 21:39 UTC · re: Jonathan Nieder · lore

[RFC] fast-import: 'cat-blob' and 'ls' commands

Hi fast importers,

I would like your thoughts on a few developments in fast-import protocol (thanks to David, Ram, Sverre, Tomas, and Sam for work so far). If they seem good, I'd be happy to help make patches to other backends so these can be implemented widely.

Contents: cat-blob command, filemodify (M) with trees, ls command.

cat-blob command ----------------

fast-import 1.7.4-rc0 added a new "cat-blob" feature. It is meant to allow exporters that receiving changes in delta form to avoid having to remember the full text of blobs already exported or re-retrieve them from the source repository.

It works like this:

1. Out of band, the fast-import frontend and backend negotiate a
   channel for the backend to send replies to the frontend.  In
   git fast-import, this is a file descriptor, defaulting to
   stdout.  So you can do:

	mkfifo replies &&

	$frontend <replies |
	git fast-import --cat-blob-fd=3 3>replies

   The intent is that stdin would typically be a socket and this file
   descriptor would point to that.

2. The frontend (optionally) declares use of this feature by putting

	feature cat-blob

   at the beginning of the stream.

3. When the frontend needs a previously exported blob to use as delta
   preimage, it uses the cat-blob command.

	cat-blob :3

   The backend replies with something like

	7c8987a987ca98c blob 6
	hello

   More precisely, the output format is

	<dataref> SP 'blob' SP <length> LF
	<full text of blob> LF

   The <dataref> can be any text not including whitespace.

   The frontend can rely on a little buffering if it wants to print a
   command after the "cat-blob", but it must read the reply in its
   entirety if it expects the backend to act on later commands.  In
   other words, the cat-blob command is not guaranteed to be
   asynchronous.

This protocol is used by the svn-fe[1] tool to handle Subversion dump files in version 3 (--deltas) format and seems to work ok.

Does this look sane or does it need tweaking or more detailed specification to be widely useful? Even once git 1.7.4 is out, it should be possible to make improvements using a new "feature" name.

filemodify (M) with trees -------------------------

fast-import 1.7.3-rc0 introduced the ability for a filemodify (M) command to place a tree named by mark or other <dataref> at a given path, replacing whatever was there before. The implementation had some kinks, which fast-import 1.7.4-rc0 ironed out.

Without some way to specify marks or learn tree names out of band, it is not very useful. With some way to learn tree names, it can be used, for example, to rewrite revision metadata while reusing the old tree data:

	commit refs/heads/master
	mark :11
	committer A U Thor <author@example.com> Wed, 26 Jan 2011 15:14:11 -0600
	data <<EOF
	New change description
	EOF
	M 040000 4b825dc642cb6eb9a060e54bf8d69288fbee4904 ""

There is no "feature" name for this. Corner case: a command to replace a path with the empty tree is interpreted[2] as meaning to remove that file or subtree, because git does not track empty directories.

Do the semantics seem reasonable? Should this get a corresponding "feature"?

ls command ----------

A patch in flight[3] introduces an "ls" command to read directory entries from the active commit or a named commit. This allows printing a blob from the active commit or copying a blob or tree from a previous commit for use in the current one.

It works like so:

1. Frontend writes

	'ls' SP <path> LF

or

	'ls' SP <dataref> SP <path> LF

  In the first form, the <path> _must_ be surrounded in quotes
  and quoted C-style.  In the second form, the <dataref> can refer
  to a tag, commit, or tree.

2. Backend replies through the cat-blob channel:

	<mode> SP <type> SP <dataref> HT <path> LF

   <mode> is a 6-digit octal mode: 040000, 100644, 100755,
   120000, or 160000 for a directory, regular file, executable file,
   symlink, or submodule, respectively.

   <type> is 'blob', 'tree', or 'commit'.

   <dataref> represents the corresponding blob, tree, or commit
   object.

   <path> is the path in question.  It can be quoted C-style and
   must be if the path starts with '"' or contains a newline.

3. Frontend reads the reply.  The frontend might use that <dataref> in
   a later filemodify (M) and cat-blob command.

Proposed updates to svn-fe[1] use this heavily and work well.

One ugly corner case: although it is intended to allow "missing <path>" as a reply when the path is missing, the proposed patch makes git fast-import use an empty tree to signal that case, to ensure that, for example,

	ls ""
	M <mode> <dataref> ""

is always a non-operation.

No "feature" name yet. Even better, it's not part of git yet so I invite to nitpick to your heart's content. Maybe you'd rather the command be called "ls-tree" instead of "ls"? Ask away. :)

Thoughts welcome, as always. Jonathan

[1] http://repo.or.cz/w/git/jrn.git/blob/refs/heads/vcs-svn-pu:/vcs-svn/svndump.c [2] Or rather, is not interpreted but ought to be, or else fast-import will make it too easy to produce invalid commits. One of the patches in series [3] fixes it. [3] http://thread.gmane.org/gmane.comp.version-control.git/162698/focus=164448

Sam Vilain· Jan 26, 2011, 23:46 UTC · re: Jonathan Nieder · lore

Re: [RFC] fast-import: 'cat-blob' and 'ls' commands

On 27/01/11 07:39, Jonathan Nieder wrote:

Show 8 quoted lines

> Hi fast importers,
>
> I would like your thoughts on a few developments in fast-import
> protocol (thanks to David, Ram, Sverre, Tomas, and Sam for work so
> far).  If they seem good, I'd be happy to help make patches to other
> backends so these can be implemented widely.
>
> Contents: cat-blob command, filemodify (M) with trees, ls command.

Ok. My first thoughts here are to be careful about the design: this fast-import protocol is fast becoming close to getting an RFC, having multiple interoperable implementations available, so do consider whether all syntax will be cleanly extensible to eventually support full basic plumbing requirements.

ie, using the command 'cat-blob' instead of a 'cat' command with 'blob' as an argument as git cat-file currently works seems to be an inflexibility and may eventually be considered legacy.

Otherwise it looks fine, seems to support all the file types etc. Thanks for keeping the work up!

Cheers, Sam