| Message ID | 1e48cde01b9e2fe24eeda063e0298db8421b13a7.1610055365.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add directory rename detection to merge-ort | expand |
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? > + /* 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". > + /* 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
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
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; + + /* Get the old and new directory names */ + get_renamed_dir_portion(pair->one->path, pair->two->path, + &old_dir, &new_dir); + 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); + + /* Free resources we don't need anymore */ + free(old_dir); + free(new_dir); + } } static void get_provisional_directory_renames(struct merge_options *opt,