Re: [PATCH v2 04/17] merge-ort: add outline for computing directory renames

[Taylor Blau <me@xxxxxxxxxxxx>]

On Thu, Jan 07, 2021 at 09:35:52PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> Port some directory rename handling changes from merge-recursive.c's
> detect_and_process_renames() to the same-named function of merge-ort.c.

Thanks, having the source be explicitly named makes it much easier to
check over the reimplementation.

> This does not yet add any use or handling of directory renames, just the
> outline for where we start to compute them.  Thus, a future patch will
> add port additional changes to merge-ort's detect_and_process_renames().

Noted.

> @@ -1086,13 +1098,24 @@ static int detect_and_process_renames(struct merge_options *opt,
>  {
>  	struct diff_queue_struct combined;
>  	struct rename_info *renames = &opt->priv->renames;
> -	int s, clean = 1;
> +	int need_dir_renames, s, clean = 1;
>
>  	memset(&combined, 0, sizeof(combined));
>
>  	detect_regular_renames(opt, merge_base, side1, MERGE_SIDE1);
>  	detect_regular_renames(opt, merge_base, side2, MERGE_SIDE2);
>
> +	need_dir_renames =
> +	  !opt->priv->call_depth &&
> +	  (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE ||
> +	   opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);

Would it be worth it to DRY up this and merge-recursive.c's
implementation, perhaps:

  int needs_dir_renames(struct merge_options *opt)
  {
    return !opt->priv->call_depth &&
      (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE ||
       opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT);
  }

and then calling that in both places?

> +	if (need_dir_renames) {
> +		for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++)
> +			get_provisional_directory_renames(opt, s, &clean);

Not necessarily related to this patch, but just something that I noticed
while reading the series: I don't find this for-loop to be any clearer
than:

  if (need_dir_renames) {
    get_provisional_directory_renames(opt, MERGE_SIDE1, &clean);
    get_provisional_directory_renames(opt, MERGE_SIDE2, &clean);
    /* ... */
  }

In fact, I think that I find the above clearer than the for-loop.
There's no opportunity to write (...; s < MERGE_SIDE2) when you meant
(...; s <= MERGE_SIDE2), and seeing two lines explicitly makes it
clearer that you're really doing the same thing to each side of the
merge.

Anyway, I may feel totally different than others, and of course I
haven't read many of the previous series as closely as this (and so this
may already be an established pattern and I'm just cutting against the
grain here), but those are my $.02.

Thanks,
Taylor