Re: [PATCH v2 07/17] merge-ort: implement compute_rename_counts()
On Mon, Jan 18, 2021 at 12:36 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 66 quoted lines
>
> 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?Show 25 quoted lines
> > + /* 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".Show 10 quoted lines
>
> > + /* 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
Elijah Newren <newren@gmail.com>