Re: [PATCH v2 01/10] Move computation of dir_rename_count from merge-ort to diffcore-rename

[Elijah Newren <newren@gmail.com>]

On Wed, Feb 24, 2021 at 7:25 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/23/2021 6:43 PM, Elijah Newren via GitGitGadget wrote:
> > ... While the
> > diffstat looks large, viewing this commit with --color-moved makes it
> > clear that only about 20 lines changed.  With this patch, the
> > computation of dir_rename_count is still only done after inexact rename
> > detection, but subsequent commits will add a preliminary computation of
> > dir_rename_count after exact rename detection, followed by some updates
> > after inexact rename detection.
>
> The --color-moved recommendation is a good one. Everything seems to be
> pretty standard, except this function:
>
> > +static void compute_dir_rename_counts(struct strmap *dir_rename_count,
> > +                                   struct strset *dirs_removed)
> > +{
> > +     int i;
> > +
> > +     /* Set up dir_rename_count */
> > +     for (i = 0; i < rename_dst_nr; ++i) {
> > +             /*
> > +              * 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.
> > +              */
> > +             update_dir_rename_counts(dir_rename_count, dirs_removed,
> > +                                      rename_dst[i].p->one->path,
> > +                                      rename_dst[i].p->two->path);
> > +     }
> > +}
>
> is very similar to this:
>
> > -static void compute_rename_counts(struct diff_queue_struct *pairs,
> > -                               struct strmap *dir_rename_count,
> > -                               struct strset *dirs_removed)
> > -{
> > -     int i;
> > -
> > -     for (i = 0; i < pairs->nr; ++i) {
> > -             struct diff_filepair *pair = pairs->queue[i];
> > -
> > -             /* File not part of directory rename if it wasn't renamed */
> > -             if (pair->status != 'R')
> > -                     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.
> > -              */
> > -             update_dir_rename_counts(dir_rename_count, dirs_removed,
> > -                                      pair->one->path,
> > -                                      pair->two->path);
> > -     }
> > -}
> > -
>
> but we dropped that "File not part of directory rename" check.
>
> It seems that is no longer possible to use with the new data structure,
> but I wonder if this will cause a slowdown in the directory renames when
> merging? Or, has the data already been filtered before calling
> compute_dir_rename_counts()?

Oh, indeed, good catch.  The
    if (pair->status != 'R')
        continue;
check should have been translated to
    if (!rename_dst[i].is_rename)
        continue;
Such a check _was_ added later in the series except with more code
than just the "continue" that didn't make sense at this stage; the
extra code made me overlook it when I was splitting.  Oops.  I'll add
this check back in; thanks for catching.