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

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 11, 2020, 14:38 UTC
Message-ID: <42037753-c8c1-629c-3d99-d54842686b2e@gmail.com>
In-Reply-To: <20201102204344.342633-7-newren@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 }"?

Show 11 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.)

Show 5 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);
> +		}

Show 11 quoted lines

>  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