Re: [PATCH v2 04/17] merge-ort: add outline for computing directory renames
- From
Elijah Newren <newren@gmail.com>
- Date
- Jan 18, 2021, 20:15 UTC
- Message-ID
- <CABPp-BE+NmuFE-Vj8dAzs-Jdsz_ruDE6P_GhAKfrFAe-sJcNEg@mail.gmail.com>
- In-Reply-To
- <YAXnfwGpvhtxbQhF@nand.local>
On Mon, Jan 18, 2021 at 11:54 AM Taylor Blau <me@ttaylorr.com> wrote:
Show 44 quoted lines
>
> On Thu, Jan 07, 2021 at 09:35:52PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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@mail.gmail.com/ and https://lore.kernel.org/git/CABPp-BEEoqOer11BYrqSVE9E4HcT5MNWcRm7ZHBQ7MVZRUDVXw@mail.gmail.com/. 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.
Show 15 quoted lines
>
> > + 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.
Show 13 quoted lines
> 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!