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

On Wed, Feb 24, 2021 at 7:25 AM Derrick Stolee [off-list ref] wrote:

On 2/23/2021 6:43 PM, Elijah Newren via GitGitGadget wrote:

quoted

... 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:

quoted

+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:

quoted

-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.