Re: [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info()

[Elijah Newren <newren@gmail.com>]

On Wed, Nov 11, 2020 at 6:38 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/2/2020 3:43 PM, Elijah Newren wrote:
> > +     /* +1 in both of the following lines to include the NUL byte */
> > +     fullpath = xmalloc(len+1);
> > +     make_traverse_path(fullpath, len+1, info, p->path, p->pathlen);
>
> nit: s/len+1/len + 1/g
>
> > +             void *buf[3] = {NULL,};
>
> This "{NULL,}" seems odd to me. I suppose there is a reason why it
> isn't "{ NULL, NULL, NULL }"?

Probably because I was copying from unpack-trees.c, which deals with a
variable number of trees instead of always exactly 3.  But yeah, it'd
probably be more straightforward as { NULL, NULL, NULL }.

> > +             const char *original_dir_name;
> > +             int i, ret;
> > +
> > +             ci->match_mask &= filemask;
> > +             newinfo = *info;
> > +             newinfo.prev = info;
> > +             newinfo.name = p->path;
> > +             newinfo.namelen = p->pathlen;
> > +             newinfo.pathlen = st_add3(newinfo.pathlen, p->pathlen, 1);
> > +
> > +             for (i = 0; i < 3; i++, dirmask >>= 1) {
>
> This multi-action iterator borders on "too clever". It seems like
> placing "dirmask >>= 1;" or "dirmask = dirmask >> 1;" at the end
> of the block would be equivalent and less jarring to a reader.
>
> I was thinking it doesn't really matter, except that dirmask is not
> in the initializer or sentinel of the for(), so having it here does
> not immediately make sense.
>
> (This has been too much writing for such an inconsequential line
> of code. Sorry.)

Yeah, copied from unpack-trees.c:traverse_trees_recursive().  The
newinfo variable name and a bunch of the surrounding lines were copied
from there too.  I can switch it, though, if it makes it easier.

> > +                     const struct object_id *oid = NULL;
> > +                     if (dirmask & 1)
> > +                             oid = &names[i].oid;
> > +                     buf[i] = fill_tree_descriptor(opt->repo, t + i, oid);
> > +             }
>
>
> >  static int collect_merge_info(struct merge_options *opt,
> >                             struct tree *merge_base,
> >                             struct tree *side1,
> >                             struct tree *side2)
> >  {
> > -     /* TODO: Implement this using traverse_trees() */
> > -     die("Not yet implemented.");
> > +     int ret;
> > +     struct tree_desc t[3];
> > +     struct traverse_info info;
> > +     char *toplevel_dir_placeholder = "";
>
> It seems like this should be "const char *"
>
> > +     init_tree_desc(t+0, merge_base->buffer, merge_base->size);
> > +     init_tree_desc(t+1, side1->buffer, side1->size);
> > +     init_tree_desc(t+2, side2->buffer, side2->size);
>
> More space issues: s/t+/t + /g

In my defense:

$ git grep init_tree_desc.*t.*\+ | grep -v merge-ort
builtin/merge.c: init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
builtin/read-tree.c: init_tree_desc(t+i, tree->buffer, tree->size);
merge-recursive.c: init_tree_desc_from_tree(t+0, common);
merge-recursive.c: init_tree_desc_from_tree(t+1, head);
merge-recursive.c: init_tree_desc_from_tree(t+2, merge);
merge.c: init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);

None of which blames to me.  :-)

I can fix it up, though...at least the merge-ort one.  Someone else
can go through existing code if they so desire.

> I'm only really able to engage in this at a surface level, it
> seems, but maybe I'll have more to say as the implementation
> grows.

It _might_ be helpful to compare to unpack-trees.c's unpack_callback()
and traverse_trees_recursive(), but there's so much unrelated stuff
there that it's possible that just gets in the way more than it helps.
Regardless, thanks for taking a look and spotting little fixes; every
bit helps.