Re: [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info()
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 11, 2020, 17:02 UTC
- Message-ID
- <CABPp-BFGs-uMNT3Ge6ZttMTxNEu-S3cdwU74XjNYYent3PQi6g@mail.gmail.com>
- In-Reply-To
- <42037753-c8c1-629c-3d99-d54842686b2e@gmail.com>
On Wed, Nov 11, 2020 at 6:38 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 12 quoted lines
>
> 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 }.
Show 22 quoted lines
> > + 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.
Show 26 quoted lines
> > + 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 + /gIn 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.