Re: [PATCH v2 0/8] Optimization batch 9: avoid detecting irrelevant renames

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 10, 2021, 15:08 UTC
Message-ID: <c4ce57f2-fc02-a70c-5ecb-0911a551192c@gmail.com>
In-Reply-To: <pull.845.v2.git.1615248599.gitgitgadget@gmail.com>

On 3/8/2021 7:09 PM, Elijah Newren via GitGitGadget wrote:

Show 20 quoted lines

> This series depends textually on ort-perf-batch-8, but semantically it's
> almost completely unrelated and can be reviewed without any familiarity with
> any of the previous patch series.
> 
> There are no changes since v1; it's just a resend just over a week later to
> bump it so it isn't lost.
> 
> === Basic Optimization idea ===
> 
> This series determines paths which meet special cases where detection of
> renames is irrelevant, where the irrelevance is due to the fact that the
> merge machinery will arrive at the same result regardless of whether a
> rename is detected for any of those paths. This series represents
> "Optimization #2" from my Git Merge 2020 talk[1], though this series has
> some improvements on the optimization relative to what I had at that time.
> 
> The basic idea here is:
> 
> We only need expensive rename detection on the subset of files changed on
> both sides of history (for the most part).

I've taken time this morning to re-read some of the patches. I have a grasp on the idea of the optimization and the code looks well presented and correct.

The only issue I have is that there are no additional tests to ensure that these scenarios are being tested and are checked to return the correct results. Naturally, it seems we are not testing the ORT strategy by default, and if I do enable it, that causes test failures still.

I wonder how much we should be merging these optimizations before the full test suite can pass with ORT as the default. Then, we can check to see if small mutations can be caught by the test suite. In particular, everything in this optimization seems to revolve around this condition in add_pair():

		if (content_relevant || location_relevant)
			strset_add(&renames->relevant_sources[side], pathname);

I'd prefer to have test cases that cover all four options for the two boolean values on this operator. Mostly, I'd like to know that if I delete either side of the || operator (or change it to &&) then we would have a test failure.

Thanks, -Stolee