On Thu, Aug 29, 2024 at 5:31 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Fri, Aug 30, 2024 at 12:12:07AM +0100, Pavel Rappo wrote:
>
> > You seem to have confirmed my understanding that I described in my
> > initial email (you replied to my second email in this thread).
>
> Heh, I did not even see the first message in the thread. But since we
> independently arrived at the same conclusions, I guess we can consider
> everything there accurate. :)
>
> I do think it's a bug that ort doesn't respect -Xno-renames.
I'm kind of splitting hairs, but doesn't "bug" imply it was
overlooked? Documentation/merge-strategies.txt makes it clear it
wasn't. ;-)
However, I agree that it makes sense to start supporting. I didn't at
first because (a) I could find no evidence at the time that anyone
actually ever used the option in conjunction with merges for
behavioral reasons (this thread from Pavel is the first counterexample
I've seen), and (b) there was lots of evidence that the related config
option was in widespread use as a workaround to the unnecessary
performance problems of rename handling with the recursive merge
algorithm. In particular, I wanted to hear about any performance
issues with renames in merges, and it was far easier to make the new
(then-experimental) algorithm just ignore these options than attempt
to do widespread convincing of folks to switch the relevant config
back on. I think that a few years has given us more than ample time
to hear about potential remaining performance issues with renames in
merges, and this thread is a good example of why to support this
option.
> The fix is probably something like this:
The fix probably starts with something like this...
> diff --git a/merge-ort.c b/merge-ort.c
> index 3752c7e595..94b3ce734c 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3338,7 +3338,7 @@ static int detect_regular_renames(struct merge_options *opt,
> 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.detect_rename = opts->detect_rename;
> diff_opts.rename_limit = opt->rename_limit;
> if (opt->rename_limit <= 0)
> diff_opts.rename_limit = 7000;
>
> though I'm not sure how DIFF_DETECT_COPIES should be handled
> ("recursive" silently downgrades it to DIFF_DETECT_RENAME).
Not downgrading DIFF_DETECT_COPIES for merging would break both
"recursive" and "ort" in all kinds of interesting ways. I once spent
a little time thinking how copy detection might affect things (long
before working on ort), and noted that under such a scenario,
"recursive" would have multiple logic bugs (mostly in the form of
missing additional needed logic rather than existing logic being
incorrect, but in places that aren't necessarily obvious at first).
Someone would have to very carefully audit the whole file containing
either recursive or ort's algorithms if they wanted to make either
somehow support copy detection. "ort" would be even more problematic
than "recursive" for such a case -- I took full advantage of the
differences between rename detection and copy detection while
optimizing ort and I think it was intrinsic to every one of the major
optimizations I did. So, if you really wanted to support copy
detection in ort, the very first step would be adding code to turn off
every single one of its major optimizations (including tree-level
merging, which didn't sound like a rename or copy detection like
thing, but hinged on how rename detection works to make sure it didn't
miss important halfs of renames).
But, the bigger issue with copies is how exactly could a merge
algorithm use them in any way that would make any logical sense? What
are you going to do, take the modifications that one side of history
made to some source file, and apply those modifications to all the
copies of the file that the other side of history made? That sounds
crazy and counter-intuitive to me. (...and incidentally, like a
factory for creating all kinds of crazy corner case issues; we could
probably make things even messier than the mod6 testcases in the
testsuite.)
> I've added ort's author to the thread, so hopefully he should have a
> more clueful response.
Yeah, not so straightforward. Even with the downgrading of copy
detection to rename detection in this area (like "recursive" does),
this isn't sufficient. IIRC (I looked at this about 2 years ago or so
when some others asked off-list), at least one of the optimizations
managed to bake in an assumption about having gone through the rename
detection codepaths. As best I remember, when you attempt to turn
rename detection off, it triggers an assertion failure in a
non-obvious place far removed from the original issue, and I only ever
got a slightly hacky workaround or two for the issue. I didn't have
good motivation or rationale to pursue very far at the time, though,
so after some hours of looking at it, I just decided to move on to
something else.
Anyway, I'll add support for no-renames to my list of things needed
before we can delete merge-recursive.[ch] and make requests for
"recursive" just map to "ort".
