Re: [PATCH v2 07/17] merge-ort: implement compute_rename_counts()

[Elijah Newren <newren@gmail.com>]

On Mon, Jan 18, 2021 at 12:36 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Jan 07, 2021 at 09:35:55PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > This function is based on the first half of get_directory_renames() from
> > merge-recursive.c
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 7e0cc597055..a8fcc026031 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,
> >
> >  /*** Function Grouping: functions related to directory rename detection ***/
> >
> > -MAYBE_UNUSED
> >  static void get_renamed_dir_portion(const char *old_path, const char *new_path,
> >                                   char **old_dir, char **new_dir)
> >  {
> > @@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
> >       *new_dir = xstrndup(new_path, end_of_new - new_path);
> >  }
> >
> > +static void increment_count(struct strmap *dir_rename_count,
> > +                         char *old_dir,
> > +                         char *new_dir)
> > +{
> > +     struct strintmap *counts;
> > +     struct strmap_entry *e;
> > +
> > +     /* Get the {new_dirs -> counts} mapping using old_dir */
> > +     e = strmap_get_entry(dir_rename_count, old_dir);
> > +     if (e) {
> > +             counts = e->value;
> > +     } else {
> > +             counts = xmalloc(sizeof(*counts));
> > +             strintmap_init_with_options(counts, 0, NULL, 1);
> > +             strmap_put(dir_rename_count, old_dir, counts);
> > +     }
> > +
> > +     /* Increment the count for new_dir */
> > +     strintmap_incr(counts, new_dir, 1);
> > +}
> > +
> >  static void compute_rename_counts(struct diff_queue_struct *pairs,
> >                                 struct strmap *dir_rename_count,
> >                                 struct strset *dirs_removed)
> >  {
> > -     die("Not yet implemented!");
> > +     int i;
> > +
> > +     for (i = 0; i < pairs->nr; ++i) {
> > +             char *old_dir, *new_dir;
> > +             struct diff_filepair *pair = pairs->queue[i];
> > +
> > +             if (pair->status != 'R')
> > +                     continue;
>
> This had a useful comment in merge-recursive.c that I think would help
> future readers here. Would you consider brining over the comment that
> starts with "File not part of directory rename ..." here?

Sure, will do.

> > +             /* Get the old and new directory names */
> > +             get_renamed_dir_portion(pair->one->path, pair->two->path,
> > +                                     &old_dir,        &new_dir);
>
> This spacing is a little odd to me, but it does come directly from
> merge-recursive.c. I don't feel strongly about it.
>
> > +             if (!old_dir)
> > +                     /* Directory didn't change at all; ignore this one. */
> > +                     continue;
> > +
> > +             /*
> > +              * Make dir_rename_count contain a map of a map:
> > +              *   old_directory -> {new_directory -> count}
> > +              * In other words, for every pair look at the directories for
> > +              * the old filename and the new filename and count how many
> > +              * times that pairing occurs.
> > +              */
> > +             if (strset_contains(dirs_removed, old_dir))
> > +                     increment_count(dir_rename_count, old_dir, new_dir);
>
> Much clearer. If you're rerolling anyway, it may be worth it to say
> something about this movement in the patch message: "along the way,
> factor out the routine to update the bookkeeping to track the number of
> items renamed into new directories".

Sure, makes sense.

>
> > +             /* Free resources we don't need anymore */
> > +             free(old_dir);
> > +             free(new_dir);
> > +     }
>
> And obviously this is new, but it makes sense here. Thanks.
>
> Thanks,
> Taylor