On 12/9/2020 2:41 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> Based heavily on merge-recursive's get_diffpairs() function.
(You're not kidding, and I should have looked here before making
some comments below.)
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> 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?
> + 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
