On Wed, Nov 11, 2020 at 6:38 AM Derrick Stolee <stolee@xxxxxxxxx> 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.
