Re: [PATCH 04/11] merge-ort: implement compare_pairs() and collect_renames()

From: Elijah Newren <newren@gmail.com>
Date: Dec 11, 2020, 18:43 UTC
Message-ID: <CABPp-BErHFVQ0Omrnn7c=x+apwXmd=T8=xdPTkypb5DOJPWNuw@mail.gmail.com>
In-Reply-To: <8d744966-2344-1dbc-f2a2-7411be3bfc8b@gmail.com>

On Thu, Dec 10, 2020 at 7:00 PM Derrick Stolee <stolee@gmail.com> wrote:

Show 5 quoted lines

>
> On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
>
> Perhaps worth pointing out comparison to score_compare()

That comparison might be slightly misleading due to this line from
collect_renames():
+               p->score = side_index;
Since diffcore-rename has already used the percentage similarity to
determine if two files are a rename and has recorded that in a
separate field, I don't need the percentage similarity anymore.  So
the score is no longer needful at this point.  However, I needed a way
to somehow record which side of the merge each diff_filepair came from
and I can't just add a field to the diff_filepair struct (especially
since its only use is in merge-ort).  I know, I know, I'm evil.
Creating a new struct just so I could have something that contained a
diff_filepair and another auxiliary field just felt so ugly, so I just
reused this one field instead.  And I did use that field to "rank" or
"sort" the pairs, so doesn't that make it a valid "score"?  :-)

I should probably add a big comment about this just above that line. I've meant to do that a multiple different times, but oddly enough this thought has only occurred to me while I'm out running or otherwise away from the computer. Until now, of course.

Show 25 quoted lines

> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  merge-ort.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 1ff637e57af..3cdf8124b85 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -625,7 +625,13 @@ static int process_renames(struct merge_options *opt,
> >
> >  static int compare_pairs(const void *a_, const void *b_)
> >  {
> > -     die("Not yet implemented.");
> > +     const struct diff_filepair *a = *((const struct diff_filepair **)a_);
> > +     const struct diff_filepair *b = *((const struct diff_filepair **)b_);
> > +
> > +     int cmp = strcmp(a->one->path, b->one->path);
> > +     if (cmp)
> > +             return cmp;
> > +     return a->score - b->score;
>
> Hm. I wasn't sure what would happen when subtracting these
> "unsigned short" scores, but I see that score_compare() does
> the same. Any potential for an existing, hidden bug here?

In the case of compare_pairs(), a->score and b->score have a minimum value of 1 and a maximum value of 2 (note above where I set score to the side index). I believe most platforms will have an int big enough to store the result of that subtraction.

I'm not sure why I bother with the secondary sort, though.  It shouldn't matter.

Which is probably a good thing, because strcmp(a, b) gives us ascending order, and a - b gives us descending order. That's messed up. Honestly, it doesn't matter because all I really needed from the sort was for diff_filepairs with the same source name to be adjacent (so that I can check for rename/rename(1to2) conflicts be comparing adjacent pairs), but still it's annoying that the function contradicts itself on the desired order. And it'll trigger whenever the same path is renamed by both sides of history, which we have a number of testcases for in the testsuite. So that confirms that the secondary sort just doesn't matter. I'll get rid of it and just use strcmp.