diff mbox series

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

Message ID ccb30dfc3c4c9ad2fc7cd33dc72ecf768827ed9f.1610055365.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add directory rename detection to merge-ort | expand

Commit Message

Elijah Newren Jan. 7, 2021, 9:35 p.m. UTC 
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.
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().

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Comments

Taylor Blau Jan. 18, 2021, 7:54 p.m. UTC | #1 
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 (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
Elijah Newren Jan. 18, 2021, 8:15 p.m. UTC | #2 
On Mon, Jan 18, 2021 at 11:54 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.

>
> > +     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!
Taylor Blau Jan. 18, 2021, 8:28 p.m. UTC | #3 
On Mon, Jan 18, 2021 at 12:15:11PM -0800, Elijah Newren wrote:
> 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.

That all makes sense, and is very helpful information to have. I'll keep
it in mind as I continue to review this series.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index 999a7c91c52..378ac495d09 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -721,6 +721,18 @@  static int handle_content_merge(struct merge_options *opt,
 
 /*** Function Grouping: functions related to directory rename detection ***/
 
+static void get_provisional_directory_renames(struct merge_options *opt,
+					      unsigned side,
+					      int *clean)
+{
+	die("Not yet implemented!");
+}
+
+static void handle_directory_level_conflicts(struct merge_options *opt)
+{
+	die("Not yet implemented!");
+}
+
 /*** Function Grouping: functions related to regular rename detection ***/
 
 static int process_renames(struct merge_options *opt,
@@ -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);
+
+	if (need_dir_renames) {
+		for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++)
+			get_provisional_directory_renames(opt, s, &clean);
+		handle_directory_level_conflicts(opt);
+	}
+
 	ALLOC_GROW(combined.queue,
 		   renames->pairs[1].nr + renames->pairs[2].nr,
 		   combined.alloc);