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

On Mon, Jan 18, 2021 at 12:36 PM Taylor Blau [off-list ref] wrote:

On Thu, Jan 07, 2021 at 09:35:55PM +0000, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

This function is based on the first half of get_directory_renames() from
merge-recursive.c

Signed-off-by: Elijah Newren <redacted>
---
 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.

quoted

+             /* 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.

quoted

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

quoted

+             /* 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