Re: [PATCH v2 2/4] diffcore-rename: complete find_basename_matches()

[Elijah Newren <newren@gmail.com>]

On Tue, Feb 9, 2021 at 5:25 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/9/2021 6:32 AM, Elijah Newren via GitGitGadget wrote:
> > +     /*
> > +      * When I checked, over 76% of file renames in linux just moved
>
> Perhaps "In late 2020," instead of "When I checked".

In early 2020 (in fact, it might have been 2019, but I have no records
to verify the actual year), but sure I can change that.

> > +      * files to a different directory but kept the same basename.  gcc
> > +      * did that with over 64% of renames, gecko did it with over 79%,
> > +      * and WebKit did it with over 89%.
> > +      *
> > +      * Therefore we can bypass the normal exhaustive NxM matrix
> > +      * comparison of similarities between all potential rename sources
> > +      * and destinations by instead using file basename as a hint, checking
> > +      * for similarity between files with the same basename, and if we
> > +      * find a pair that are sufficiently similar, record the rename
> > +      * pair and exclude those two from the NxM matrix.
> > +      *
> > +      * This *might* cause us to find a less than optimal pairing (if
> > +      * there is another file that we are even more similar to but has a
> > +      * different basename).  Given the huge performance advantage
> > +      * basename matching provides, and given the frequency with which
> > +      * people use the same basename in real world projects, that's a
> > +      * trade-off we are willing to accept when doing just rename
> > +      * detection.  However, if someone wants copy detection that
> > +      * implies they are willing to spend more cycles to find
> > +      * similarities between files, so it may be less likely that this
> > +      * heuristic is wanted.
> > +      */
> > +
> > +     int i, renames = 0;
> >       struct strintmap sources;
> >       struct strintmap dests;
>
> ...
>
> > +      * copy detection.  find_basename_matches() is only used when detecting
> > +      * renames, not when detecting copies, so it'll only be used when a file
> > +      * only existed in the source.  Since we already know that the file
>
> There are two "only"s in this sentence. Just awkward, not wrong.
>
> > +      * won't be unmodified, there's no point checking for it; that's just a
> > +      * waste of resources.  So set skip_unmodified to 0 so that
> > +      * estimate_similarity() and prefetch() won't waste resources checking
> > +      * for something we already know is false.
> > +      */
> > +     int skip_unmodified = 0;
> > +
>
>
>
> > -     /* TODO: Make use of basenames source and destination basenames */
> > +     /* Now look for basename matchups and do similarity estimation */
> > +     for (i = 0; i < num_src; ++i) {
> > +             char *filename = rename_src[i].p->one->path;
> > +             char *base = NULL;
> > +             intptr_t src_index;
> > +             intptr_t dst_index;
> > +
> > +             /* Get the basename */
> > +             base = strrchr(filename, '/');
> > +             base = (base ? base+1 : filename);
>
> Here is the third instance of this in the same function. At minimum we should
> extract a helper for you to consume.

Where by "this" you mean these last two lines, right?

And perhaps explain why I'm not using either basename(3) or
gitbasename() from git-compat-util.h?  (The latter of which I just
learned about while responding to the review of this patch.)

or maybe gitbasename can do the job, but the skip_dos_drive_prefix()
and the munging of the string passed in both worry me.  And the
is_dir_sep() looks inefficient since I know I'm dealing with filenames
as stored in git internally, and thus can only use '/' characters.
Hmm...

Yeah, I think I'll add my own helper in this file, since you want one,
and just use it.

> > +             /* Find out if this basename is unique among sources */
> > +             src_index = strintmap_get(&sources, base);
> > +             if (src_index == -1)
> > +                     continue; /* not a unique basename; skip it */
> > +             assert(src_index == i);
> > +
> > +             if (strintmap_contains(&dests, base)) {
> > +                     struct diff_filespec *one, *two;
> > +                     int score;
> > +
> > +                     /* Find out if this basename is unique among dests */
> > +                     dst_index = strintmap_get(&dests, base);
> > +                     if (dst_index == -1)
> > +                             continue; /* not a unique basename; skip it */
> > +
> > +                     /* Ignore this dest if already used in a rename */
> > +                     if (rename_dst[dst_index].is_rename)
> > +                             continue; /* already used previously */
> > +
> > +                     /* Estimate the similarity */
> > +                     one = rename_src[src_index].p->one;
> > +                     two = rename_dst[dst_index].p->two;
> > +                     score = estimate_similarity(options->repo, one, two,
> > +                                                 minimum_score, skip_unmodified);
> > +
> > +                     /* If sufficiently similar, record as rename pair */
> > +                     if (score < minimum_score)
> > +                             continue;
> > +                     record_rename_pair(dst_index, src_index, score);
> > +                     renames++;
> > +
> > +                     /*
> > +                      * Found a rename so don't need text anymore; if we
> > +                      * didn't find a rename, the filespec_blob would get
> > +                      * re-used when doing the matrix of comparisons.
> > +                      */
> > +                     diff_free_filespec_blob(one);
> > +                     diff_free_filespec_blob(two);
> > +             }
> > +     }
>
> Makes sense to me.
>
> Thanks,
> -Stolee