on Sun Jun 23 2013, John Keeping <john-AT-keeping.me.uk> wrote:

> On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:
>>                                              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

Oops; good eye.

>> > 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.

The reason I ask is because from scouring this list it looks like
there's a history of people having issues with this, and someone
intended to get to a fix in sometime around 1.17.10, but nothing ever
happened.

> 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.

t9300?  

Thanks; I'll try this one too.

> -- >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.

-- 
Dave Abrahams
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to