Re: [PATCH v2 13/13] merge-ort, diffcore-rename: employ cached renames when possible
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 17, 2021, 14:23 UTC
- Message-ID
- <f0c50259-5627-482b-1daf-b73819cde108@gmail.com>
- In-Reply-To
- <91b6768adf2d1777219fb2d83cc2363f1497dbbd.1620094339.git.gitgitgadget@gmail.com>
On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
...
Show 26 quoted lines
> diff --git a/merge-ort.c b/merge-ort.c
> index 2fc98b803d1c..17dc3deb3c73 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -753,15 +753,48 @@ static void add_pair(struct merge_options *opt,
> struct rename_info *renames = &opt->priv->renames;
> int names_idx = is_add ? side : 0;
>
> - if (!is_add) {
> + if (is_add) {
> + if (strset_contains(&renames->cached_target_names[side],
> + pathname))
> + return;
> + } else {
> unsigned content_relevant = (match_mask == 0);
> unsigned location_relevant = (dir_rename_mask == 0x07);
>
> + /*
> + * If pathname is found in cached_irrelevant[side] due to
> + * previous pick but for this commit content is relevant,
> + * then we need to remove it from cached_irrelevant.
> + */
> + if (content_relevant)
> + /* strset_remove is no-op if strset doesn't have key */
> + strset_remove(&renames->cached_irrelevant[side],
> + pathname);I see, content can become relevant again.
...
Show 25 quoted lines
> diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh > index f47d8924ee73..035edc40b1eb 100755 > --- a/t/t6429-merge-sequence-rename-caching.sh > +++ b/t/t6429-merge-sequence-rename-caching.sh > @@ -101,10 +101,10 @@ test_expect_success 'caching renames does not preclude finding new ones' ' > # dramatic change in size of the file, but remembering the rename and > # reusing it is reasonable too. > # > -# Rename detection (diffcore_rename_extended()) will run twice here; it is > -# not needed on the topic side of history for either of the two commits > -# being merged, but it is needed on the upstream side of history for each > -# commit being picked. > +# We do test here that we expect rename detection to only be run once total > +# (the topic side of history doesn't need renames, and with caching we > +# should be able to only run rename detection on the upstream side one > +# time.) > test_expect_success 'cherry-pick both a commit and its immediate revert' ' > test_create_repo pick-commit-and-its-immediate-revert && > ( > @@ -140,11 +140,11 @@ test_expect_success 'cherry-pick both a commit and its immediate revert' ' > GIT_TRACE2_PERF="$(pwd)/trace.output" && > export GIT_TRACE2_PERF && > > - test_might_fail test-tool fast-rebase --onto HEAD upstream~1 topic && > + test-tool fast-rebase --onto HEAD upstream~1 topic &&
Here is a change of behavior, but it appears to be a good one!
Show 6 quoted lines
> #git cherry-pick upstream~1..topic && > > grep region_enter.*diffcore_rename trace.output >calls && > - test_line_count = 2 calls > + test_line_count = 1 calls > )
...
Show 15 quoted lines
> @@ -450,7 +459,7 @@ test_expect_success 'dir rename unneeded, then add new file to old dir' ' > #git cherry-pick upstream..topic && > > grep region_enter.*diffcore_rename trace.output >calls && > - test_line_count = 3 calls && > + test_line_count = 2 calls && > > git ls-files >tracked && > test_line_count = 5 tracked && > @@ -516,7 +525,7 @@ test_expect_success 'dir rename unneeded, then rename existing file into old dir > #git cherry-pick upstream..topic && > > grep region_enter.*diffcore_rename trace.output >calls && > - test_line_count = 4 calls && > + test_line_count = 3 calls &&
I appreciate that this use of tracing demonstrates a change of internal behavior.
Show 20 quoted lines
> test_path_is_missing somefile && > test_path_is_missing olddir/newfile && > @@ -648,9 +657,8 @@ test_expect_success 'caching renames only on upstream side, part 1' ' > # for the wrong side of history. > # > # > -# This testcase should only need three calls to diffcore_rename_extended(), > -# because there are no renames on the topic side of history for picking > -# Topic_2. > +# This testcase should only need two calls to diffcore_rename_extended(), > +# both for the first merge, one for each side of history. > # > test_expect_success 'caching renames only on upstream side, part 2' ' > test_setup_topic_rename cache-renames-only-upstream-rename-file && > @@ -677,7 +685,7 @@ test_expect_success 'caching renames only on upstream side, part 2' ' > #git cherry-pick upstream..topic && > > grep region_enter.*diffcore_rename trace.output >calls && > - test_line_count = 3 calls && > + test_line_count = 2 calls &&
Same here.
As I was reading, I was also thinking that it would be good to have some kind of tracing, if only a summary of how often we relied upon the cached renames. That would present a mechanism for the test cases to verify that the rename cache is behaving as expected, but also provides a way to diagnose any issues that might arise in the future by asking a user to reproduce a problematic rebase/merge with a GIT_TRACE2* target. Such a change would fit as a follow-up, and does not need to insert into an already heavy patch.
I have now read all of the patches in this series to the level I can. It's all very deep stuff, so the more we can rely on the tests to show correctness, the better.
I appreciate the extra tests that you added, which increases my confidence in the series.
Thanks, -Stolee