Small new merge-ort features, prepping for deletion of merge-recursive.[ch] #1875
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@f266577. |
|
This patch series was integrated into seen via git@52fe680. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@a054011. |
|
This patch series was integrated into seen via git@fe442ee. |
|
This patch series was integrated into seen via git@9eb6de7. |
|
This patch series was integrated into seen via git@18cf156. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Fri, Mar 07, 2025 at 03:48:39PM +0000, Elijah Newren via GitGitGadget wrote:
> I've got 19 patches covering the work needed to prep for and allow us to
> delete merge-recursive.[ch], and remap 'recursive' to 'ort', including some
> clean-up along the way. I've tried to divide it up into five smaller patch
> series.
>
> These 3 patches are the first of those series, and each of these 3 patches
> provide a small new feature that together will be used to allow us to
> convert some callers over from recursive to ort. If the third patch,
> introducing merge_ort_generic(), doesn't make sense to submit without one of
> its new callers, I can extend this series to 6 patches and include the
> conversion of git-am.sh.
I think extending it to 6 patches would make sense as it's somewhat
unfortunate that this version introduces the function, but has no
callers at all.
Patrick |
merge-recursive.[ch] have three entry points: * merge_trees() * merge_recursive() * merge_recursive_generic() merge-ort*.[ch] only has equivalents for the first two. Add an equivalent for the final entry point, so we can switch callers to use it and remove merge-recursive.[ch]. While porting it over, finally fix the issue with the label for the ancestor (used when merge.conflictStyle=diff3 as a conflict label). merge-recursive.c has traditionally not allowed callers to set that label, but I have found that problematic for years. (Side note: This function was initially part of the merge-ort rewrite, but reviewers questioned the ancestor label funnyness which I was never really happy with anyway. It resulted in me jettisoning it and hoping at the time that I would eventually be able to force the existing callers to use some other API. That worked with `git stash`, as per 874cf2a (stash: apply stash using 'merge_ort_nonrecursive()', 2022-05-10), but this API is the most reasonable one for `git am` and `git merge-recursive`, if we can just allow them some freedom over the ancestor label.) The merge_recursive_generic() function did not know whether it was being invoked by `git stash`, `git merge-recursive`, or `git am`, and the choice of meaningful ancestor label, when there is a unique ancestor, varies for these different callers: * git am: ancestor is a constructed "fake ancestor" that user knows nothing about and has no access to. (And is different than the normal thing we mean by a "virtual merge base" which is the merging of merge bases.) * git merge-recursive: ancestor might be a tree, but at least it was one specified by the user (if they invoked merge-recursive directly) * git stash: ancestor was the commit serving as the stash base Thus, using a label like "constructed merge base" (as merge_recursive_generic() does) presupposes that `git am` is the only caller; it is incorrect for other callers. This label has thrown me off more than once. Allow the caller to override when there is a unique merge base. Signed-off-by: Elijah Newren <[email protected]>
|
User |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Wed, Mar 12, 2025 at 09:06:31AM +0100, Patrick Steinhardt wrote:
> On Fri, Mar 07, 2025 at 03:48:39PM +0000, Elijah Newren via GitGitGadget wrote:
> > I've got 19 patches covering the work needed to prep for and allow us to
> > delete merge-recursive.[ch], and remap 'recursive' to 'ort', including some
> > clean-up along the way. I've tried to divide it up into five smaller patch
> > series.
> >
> > These 3 patches are the first of those series, and each of these 3 patches
> > provide a small new feature that together will be used to allow us to
> > convert some callers over from recursive to ort. If the third patch,
> > introducing merge_ort_generic(), doesn't make sense to submit without one of
> > its new callers, I can extend this series to 6 patches and include the
> > conversion of git-am.sh.
>
> I think extending it to 6 patches would make sense as it's somewhat
> unfortunate that this version introduces the function, but has no
> callers at all.
Eh. I have gone back and forth about that over the years. I think in
cases where the either caller(s) or implementation is sufficiently
complicated, it's OK to introduce a (non-static) function without any
callers.
I don't feel strongly about it, so I think the multi-series structure is
fine as it is (especially since we know that more are coming in the
future). But I'm also not opposed to seeing this series extended out to
include the three additional patches.
Thanks,
Taylor |
When merge-ort was written, I did not at first allow rename detection to be disabled, because I suspected that most folks disabling rename detection were doing so solely for performance reasons. Since I put a lot of working into providing dramatic speedups for rename detection performance as used by the merge machinery, I wanted to know if there were still real world repositories where rename detection was problematic from a performance perspective. We have had years now to collect such information, and while we never received one, waiting longer with the option disabled seems unlikely to help surface such issues at this point. Also, there has been at least one request to allow rename detection to be disabled for behavioral rather than performance reasons (see the thread including https://lore.kernel.org/git/CABPp-BG-Nx6SCxxkGXn_Fwd2wseifMFND8eddvWxiZVZk0zRaA@mail.gmail.com/ ), so let's start heeding the config and command line settings. Signed-off-by: Elijah Newren <[email protected]>
Various callers such as am & checkout set the merge verbosity to 0 to avoid having conflict messages printed. While this could be achieved by avoiding the wrappers from merge-ort-wrappers and instead passing 0 for display_update_msgs to merge_switch_to_result(), for simplicity of converting callers simply allow them to also achieve this with the merge-ort-wrappers by setting verbosity to 0. Signed-off-by: Elijah Newren <[email protected]>
There is a bug in the way renames are cached that rears its head when
`merge.directoryRenames` is set to false; it results in the following
message:
merge-ort.c:3002: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.
Aborted
It is quite a curious bug: the same test case will succeed, without any
assertion, if instead run with `merge.directoryRenames=true`.
Further, the assertion does not manifest while replaying the first
commit, it manifests while replaying the _second_ commit of the commit
range. But it does _not_ manifest when the second commit is replayed
individually.
This would indicate that there is an incomplete rename cache left-over
from the first replayed commit which is being reused for the second
commit, and if directory rename detection is enabled, the missing paths
are somehow regenerated.
Incidentally, the same bug can by triggered by modifying t6429 to switch
from merge.directoryRenames=true to merge.directoryRenames=false.
Signed-off-by: Johannes Schindelin <[email protected]>
[en: tweaked the commit message slightly, including adjusting the
line number of the assertion to the latest version, and the much
later discovery that a simple t6429 tweak would also display the
issue.]
Signed-off-by: Elijah Newren <[email protected]>
There are two issues here. First, when merge.directoryRenames is set to false, there are a few code paths that should be turned off. I missed one; collect_renames() was still doing some directory rename detection logic unconditionally. It ended up not having much effect because get_provisional_directory_renames() was skipped earlier and not setting up renames->dir_renames, but the code should still be skipped. Second, the larger issue is that sometimes we get a cached_pair rename from a previous commit being replayed mapping A->B, but in a subsequent commit but collect_merge_info() doesn't even recurse into the directory containing B because there are no source pairings for that rename that are relevant; we can merge that commit fine without knowing the rename. But since the cached renames are added to the normal renames, when we go to process it and find that B is not part of opt->priv->paths, we hit the assertion error process_renames: Assertion `newinfo && ~newinfo->merged.clean` failed. I think we could fix this at the beginning of detect_regular_renames() by pruning from cached_pairs any entry whose destination isn't in opt->priv->paths, but it's suboptimal in that we'd kind of like the cached_pair to be restored afterwards so that it can help the subsequent commit, but more importantly since it sits at the intersection of the caching renames optimization and the relevant renames optimization, and the trivial directory resolution optimization, and I don't currently have Documentation/technical/remembering-renames.txt fully paged in, I'm not sure if that's a full solution or a bandaid for the current testcase. However, since the remembering renames optimization was the weakest of the set, and the optimization is far less important when directory rename detection is off (as that implies far fewer potential renames), let's just use a bigger hammer to ensure this special case is fixed: turn off the rename caching. We do the same thing already when we encounter rename/rename(1to1) cases (as per `git grep -3 disabling.the.optimization`, though it uses a slightly different triggering mechanism since it's trying to affect the next time that merge_check_renames_reusable() is called), and I think it makes sense to do the same here. Signed-off-by: Elijah Newren <[email protected]>
|
User |
Switch from merge-recursive to merge-ort. Adjust the following
testcases due to the switch:
* t4151: This test left an untracked file in the way of the merge.
merge-recursive could only sometimes tell when untracked files were
in the way, and by the time it discovers others, it has already made
too many changes to back out of the merge. So, instead of writing the
results to e.g. 'file1' it would instead write them to
'file1~branch1'. This is confusing for users, because they might not
notice 'file1~branch1' and accidentally add and commit 'file1'.
In contrast, merge-ort correctly notices the file in the way before
making any changes and aborts. Since this test didn't care about the
file in the way, just remove it before calling git-am.
* t4255: Usage of merge-ort allows us to change two known failures into
successes.
* t6427: As noted a few commits ago, the choice of conflict label for
diff3 markers for the ancestor commit was previously handled by
merge-recursive.c rather than by callers. Since that has now changed,
`git am` needs to specify that label. Although the previous conflict
label ("constructed merge base") was already fairly somewhat slanted
towards `git am`, let's use wording more along the lines of the
related command-line flag from `git apply` and function involved to
tie it more closely to `git am`.
Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
endit-new-features
branch
from
c2a2be3 to
3f4b74e
Compare
|
This patch series was integrated into seen via git@ed623a0. |
|
There was a status update in the "New Topics" section about the branch First step of deprecating and removing merge-recursive. Will merge to 'next'. source: <[email protected]> |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
User |
|
This patch series was integrated into seen via git@88cf449. |
|
This patch series was integrated into seen via git@e8a6368. |
|
There was a status update in the "Cooking" section about the branch First step of deprecating and removing merge-recursive. Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@b3bb87e. |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Thu, Mar 13, 2025 at 02:46:35AM +0000, Elijah Newren via GitGitGadget wrote:
> Range-diff vs v1:
This is already marked as ready to merge to 'next' in the last WC, but I
took a look at the range-diff and everything LGTM:
Reviewed-by: Taylor Blau <[email protected]>
Thanks,
Taylor |
|
This patch series was integrated into seen via git@7ee7ddd. |
|
This patch series was integrated into seen via git@c17492e. |
|
This patch series was integrated into next via git@a911944. |
|
This patch series was integrated into seen via git@8e8d58d. |
|
There was a status update in the "Cooking" section about the branch First step of deprecating and removing merge-recursive. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@21fee2d. |
|
There was a status update in the "Cooking" section about the branch First step of deprecating and removing merge-recursive. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@1c8d33e. |
|
This patch series was integrated into seen via git@ea35b7c. |
|
There was a status update in the "Cooking" section about the branch First step of deprecating and removing merge-recursive. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@65dc24c. |
|
This patch series was integrated into seen via git@eb7923b. |
|
This patch series was integrated into master via git@eb7923b. |
|
This patch series was integrated into next via git@eb7923b. |
|
Closed via eb7923b. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've got 19 patches covering the work needed to prep for and allow us to delete merge-recursive.[ch], and remap 'recursive' to 'ort', including some clean-up along the way. I've tried to divide it up into some smaller patch series.
These 6 patches are the first of those series. Breakdown:
git amalways turned off directory rename detection, this is a prerequisite for convertinggit am)git amfrom using merge_recursive_generic() to use the new merge_ort_generic().Changes since v1:
cc: Patrick Steinhardt [email protected]
cc: Taylor Blau [email protected]
cc: Elijah Newren [email protected]
cc: Jeff King [email protected]