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

Re: fast-import bug?

From
John Keeping <john@keeping.me.uk>
Date
Jun 23, 2013, 11:09 UTC
Message-ID
<20130623110933.GG4676@serenity.lan>
In-Reply-To
<m2txkp1shb.fsf@cube.gateway.2wire.net>
On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:
Show 50 quoted lines
> 
> on Sat Jun 22 2013, John Keeping <john-AT-keeping.me.uk> wrote:
> 
> > On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:
> >> The docs for fast-import seem to imply that I can use "ls" to get the
> >> SHA1 of a commit for which I have a mark:
> >> 
> >>        Reading from a named tree
> >>            The <dataref> can be a mark reference (:<idnum>) or the full 40-byte
> >
> >>            SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to
> >>            be written. The path is relative to the top level of the tree named by
> >>            <dataref>.
> >> 
> >>                        'ls' SP <dataref> SP <path> LF
> >> 
> >>        See filemodify above for a detailed description of <path>.
> >> 
> >>        Output uses the same format as git ls-tree <tree> -- <path>:
> >> 
> >>            <mode> SP ('blob' | 'tree' | 'commit') SP <dataref> HT <path> LF
> >> 
> >>        The <dataref> represents the blob, tree, or commit object at <path> and
> >>                                                    ^^^^^^
> >>        can be used in later cat-blob, filemodify, or ls commands.
> >> 
> >> but I can't get it to work.  It's not entirely clear it's supposed to
> >> work.  What path would I pass?  Passing an empty path simply causes git
> >> to report "missing ".
> >
> > Which version of Git are you using?  
> 
> ,----[ git --version ]
> | git version 1.8.3.1
> `----
> 
> > I just tried this and get the error
> > "fatal: Empty path component found in input", 
> 
> I get that too.
> 
> > which seems to be from commit 178e1de (fast-import: don't allow 'ls'
> > of path with empty components, 2012-03-09), which is included in Git
> > 1.7.9.5.
> 
> Yes, that's at least part of the issue.  I notice git-fast-import
> rejects the root path "" for other commands, e.g. when used as the
> source of a filecopy we get the same issue.  I also note that the docs
> don't make it clear that quoting the path is mandatory if it might turn
> out to be empty.

Interesting. There are two places that can produce this error message, tree_content_get and tree_content_set, but I wonder if this means that tree_content_get should not be doing this check. The two places that call it are:

1) "parse_ls" as discussed here
2) "file_change_cr" which deals with file copy and rename.

My patch in the previous message only changes the behaviour for the parse_ls case, but it seems that you have a valid use case for removing this check in the file_change_cr case as well.

>                                              I also note that the docs
> don't make it clear that quoting the path is mandatory if it might turn
> out to be empty.

That's not quite the case. It looks to me like quoting the path is mandatory if no "<dataref>" is given, and indeed the documentation says:

   Reading from the active commit
       This form can only be used in the middle of a commit. The path
       names a directory entry within fast-import’s active commit. The
       path must be quoted in this case.
               'ls' SP <path> LF
Show 20 quoted lines
> > It seems to be slightly more complicated than that though, because after
> > allowing empty trees I get the "missing" message for the root tree.
> 
> Yeah, I've tried to patch Git to solve this but ran into that problem
> and gave up.
> 
> > This seems to be because its mode is 0 and not S_IFDIR.
> 
> Aha.
> 
> > With the patch below, things are working as I expect 
> 
> Awesome; works for me, too!
> 
> > but I don't understand why the mode of the root is not set correctly
> > at this point.  Perhaps someone more familiar with fast-import will
> > have some insight...
> 
> Yeah... there's no bug tracker for Git, right?  So if nobody pays
> attention to this thread, the problem will persist?

Yes, but I don't see that happening particularly often. In the worst case issues are normally documented by a failing test case.

In this case, I think I do now understand why the mode is 0: in parse_ls a new tree object is created and the SHA1 of the original is copied in but the mode is left blank; clearly this should be set to S_IFDIR when the SHA1 is non-null.

I think the patch I now have is correct (and addresses the "copy from root" scenario), but I need to spend some time understanding t9300 so that I can add suitable test cases.

-- >8 --
diff --git a/fast-import.c b/fast-import.c
index 23f625f..e2c9d50 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1629,7 +1629,8 @@ del_entry:
 static int tree_content_get(
 	struct tree_entry *root,
 	const char *p,
-	struct tree_entry *leaf)
+	struct tree_entry *leaf,
+	int allow_root)
 {
 	struct tree_content *t;
 	const char *slash1;
@@ -1641,31 +1642,39 @@ static int tree_content_get(
 		n = slash1 - p;
 	else
 		n = strlen(p);
-	if (!n)
+	if (!n && !allow_root)
 		die("Empty path component found in input");
 
 	if (!root->tree)
 		load_tree(root);
+
+	if (!n) {
+		e = root;
+		goto found_entry;
+	}
+
 	t = root->tree;
 	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 found_entry;
 			if (!S_ISDIR(e->versions[1].mode))
 				return 0;
 			if (!e->tree)
 				load_tree(e);
-			return tree_content_get(e, slash1 + 1, leaf);
+			return tree_content_get(e, slash1 + 1, leaf, 0);
 		}
 	}
 	return 0;
+
+found_entry:
+	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)
@@ -2415,7 +2424,7 @@ static void file_change_cr(struct branch *b, int rename)
 	if (rename)
 		tree_content_remove(&b->branch_tree, s, &leaf);
 	else
-		tree_content_get(&b->branch_tree, s, &leaf);
+		tree_content_get(&b->branch_tree, s, &leaf, 1);
 	if (!leaf.versions[1].mode)
 		die("Path %s not in branch", s);
 	if (!*d) {	/* C "path/to/subdir" "" */
@@ -3051,6 +3060,8 @@ 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);
+		if (!is_null_sha1(root->versions[1].sha1))
+			root->versions[1].mode = S_IFDIR;
 		load_tree(root);
 		if (*p++ != ' ')
 			die("Missing space after tree-ish: %s", command_buf.buf);
@@ -3065,7 +3076,7 @@ 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);
+	tree_content_get(root, p, &leaf, 1);
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
Previous: Dave AbrahamsNext: Dave Abrahams
Message 4 of 6 in “fast-import bug?”
  1. Dave AbrahamsJun 21, 2013
  2. John KeepingJun 22, 2013
  3. Dave AbrahamsJun 23, 2013
  4. John KeepingJun 23, 2013
  5. Dave AbrahamsJun 23, 2013
  6. John KeepingJun 23, 2013

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.