| Message ID | 207bb9a837cb855721daa88caaad80e37cb40ffe.1607542887.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | merge-ort: add basic rename detection | expand |
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() > 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? Thanks, -Stolee
On Thu, Dec 10, 2020 at 7:00 PM Derrick Stolee <stolee@gmail.com> wrote: > > 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. > > 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.
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; } /* Call diffcore_rename() to compute which files have changed on given side */ @@ -671,7 +677,24 @@ static int collect_renames(struct merge_options *opt, struct diff_queue_struct *result, unsigned side_index) { - die("Not yet implemented."); + int i, clean = 1; + struct diff_queue_struct *side_pairs; + struct rename_info *renames = opt->priv->renames; + + side_pairs = &renames->pairs[side_index]; + + for (i = 0; i < side_pairs->nr; ++i) { + struct diff_filepair *p = side_pairs->queue[i]; + + if (p->status != 'R') { + diff_free_filepair(p); + continue; + } + p->score = side_index; + result->queue[result->nr++] = p; + } + + return clean; } static int detect_and_process_renames(struct merge_options *opt,