On Mon, Jan 18, 2021 at 11:54 AM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> 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 the intent was to keep merge-recursive.c indefinitely, then yes it
would. However, the intent is to (1) avoid touching merge-recursive.c
so I don't destabilize it and so folks can fall back to it, (2) get
merge-ort.c completed, and people to adopt and feel confident in it,
(3) delete merge-recursive.[ch].
This has come up a few other times in a review on the series, because
there are even examples of copied-and-unmodified functions; see
https://lore.kernel.org/git/CABPp-BGtpXRSz+ngFz20j8W4qgpb8juogsLf6HF7b0-Pt=s6=g@xxxxxxxxxxxxxx/
and https://lore.kernel.org/git/CABPp-BEEoqOer11BYrqSVE9E4HcT5MNWcRm7ZHBQ7MVZRUDVXw@xxxxxxxxxxxxxx/.
I know it seems weird to intentionally repeat, but since the goal is
to nuke merge-recursive.c, I'm doing it as a temporary measure.
>
> > + 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);
> /* ... */
> }
>
That also makes it more similar to the calls I make to
detect_regular_renames() above, where I explicitly called it twice
instead of using a loop. I like the suggested change; I'll update it.
> 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
As always, thanks for the review!
