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

[Derrick Stolee <stolee@gmail.com>]

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 }"?

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

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

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.

Thanks,
-Stolee