Re: [PATCH 03/11] merge-ort: implement detect_regular_renames()
From: Derrick Stolee <hidden>
Date: 2020-12-11 02:56:06
On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted> Based heavily on merge-recursive's get_diffpairs() function.
(You're not kidding, and I should have looked here before making some comments below.)
quoted hunk ↗ jump to hunk
Signed-off-by: Elijah Newren <redacted> --- merge-ort.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)diff --git a/merge-ort.c b/merge-ort.c index 92b765dd3f0..1ff637e57af 100644 --- a/merge-ort.c +++ b/merge-ort.c@@ -634,7 +634,33 @@ static void detect_regular_renames(struct merge_options *opt, struct tree *side, unsigned side_index) { - die("Not yet implemented."); + struct diff_options diff_opts; + struct rename_info *renames = opt->priv->renames; + + repo_diff_setup(opt->repo, &diff_opts); + diff_opts.flags.recursive = 1; + diff_opts.flags.rename_empty = 0; + diff_opts.detect_rename = DIFF_DETECT_RENAME; + diff_opts.rename_limit = opt->rename_limit;
I assume that opt->rename_limit has been initialized properly against merge.renameLimit/diff.renameLimit in another location...
+ if (opt->rename_limit <= 0) + diff_opts.rename_limit = 1000;
(I made the following comments before thinking to look at get_diffpairs() which behaves in an equivalent way with this "1000" constant limit. I'm not sure if there is a reason why this limit is different from the _other_ limits I discovered, but it might still be good to reduce magic literal ints by grouping this "1000" into a const or macro.) ...and this just assigns the default again. Why is this done here instead of inside the diff machinery? Also, wouldn't a diff.renameLimit = 0 imply no renames, not "use default"? I notice that the docs don't make this explicit: diff.renameLimit:: The number of files to consider when performing the copy/rename detection; equivalent to the 'git diff' option `-l`. This setting has no effect if rename detection is turned off. but also too_many_rename_candidates() has this strange default check: /* * This basically does a test for the rename matrix not * growing larger than a "rename_limit" square matrix, ie: * * num_create * num_src > rename_limit * rename_limit */ if (rename_limit <= 0) rename_limit = 32767; this is... a much larger limit than I would think is reasonable. Of course, diff_rename_limit_default is set to 400 inside diff.c. Should that be extracted as a constant so we can repeat it here?
quoted hunk ↗ jump to hunk
+ diff_opts.rename_score = opt->rename_score; + diff_opts.show_rename_progress = opt->show_rename_progress; + diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_setup_done(&diff_opts); + diff_tree_oid(&merge_base->object.oid, &side->object.oid, "", + &diff_opts); + diffcore_std(&diff_opts); + + if (diff_opts.needed_rename_limit > opt->priv->renames->needed_limit) + opt->priv->renames->needed_limit = diff_opts.needed_rename_limit; + + renames->pairs[side_index] = diff_queued_diff; + + diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_queued_diff.nr = 0; + diff_queued_diff.queue = NULL; + diff_flush(&diff_opts); } /*@@ -1379,6 +1405,10 @@ void merge_switch_to_result(struct merge_options *opt, printf("%s", sb->buf); } string_list_clear(&olist, 0); + + /* Also include needed rename limit adjustment now */ + diff_warn_rename_limit("merge.renamelimit", + opti->renames->needed_limit, 0);
I suppose this new call is appropriate in this patch, since you assign the value inside detect_regular_renames(), but it might be good to describe its presence in the commit message. Thanks, -Stolee