Declare merge-ort ready for general usage #905
Closed
Conversation
This file contains 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
newren
force-pushed
the
temporary/ort-perf-batch-10
branch
from
495c109
to
ac0ba91
Compare
|
/submit |
|
Submitted as pull.905.git.1615867503.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
|
User |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
User |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
rename/rename conflict handling depends on the fact that if both sides renamed the same path, that the one on the MERGE_SIDE1 will appear first in the combined diff_queue_struct passed to process_renames(). Since we add all pairs from MERGE_SIDE1 to combined first, and then all pairs from MERGE_SIDE2, and then sort based on filename, this will only be true if the sort used is stable. This was found due to the fact that Mac, unlike Linux, apparently has a system-defined qsort that is not stable. While we are at it, review the other callers of QSORT and add comments about why they can remain as calls to QSORT instead of being modified to call STABLE_QSORT. Signed-off-by: Elijah Newren <newren@gmail.com>
renormalize_buffer() requires an index_state, which is something that merge-ort does not operate with. However, all the renormalization code needs is an index with a .gitattributes file...plus a little bit of setup. Create such an index, along with the deallocation and attr_direction handling. A subsequent commit will add a function to finish the initialization of this index. Signed-off-by: Elijah Newren <newren@gmail.com>
ll_merge() needs an index when renormalization is requested. Create one specifically for just this purpose with just the one needed entry. This fixes t6418.4 and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort. NOTE 1: Even if the user has a working copy or a real index (which is not a given as merge-ort can be used in bare repositories), we explicitly ignore any .gitattributes file from either of these locations. merge-ort can be used to merge two branches that are unrelated to HEAD, so .gitattributes from the working copy and current index should not be considered relevant. NOTE 2: Since we are in the middle of merging, there is a risk that .gitattributes itself is conflicted...leaving us with an ill-defined situation about how to perform the rest of the merge. It could be that the .gitattributes file does not even exist on one of the sides of the merge, or that it has been modified on both sides. If it's been modified on both sides, it's possible that it could itself be merged cleanly, though it's also possible that it only merges cleanly if you use the right version of the .gitattributes file to drive the merge. It gets kind of complicated. The only test we ever had that attempted to test behavior in this area was seemingly unaware of the undefined behavior, but knew the test wouldn't work for lack of attribute handling support, marked it as test_expect_failure from the beginning, but managed to fail for several reasons unrelated to attribute handling. See commit 6f6e7cf ("t6038: remove problematic test", 2020-08-03) for details. So there are probably various ways to improve what initialize_attr_index() picks in the case of a conflicted .gitattributes but for now I just implemented something simple -- look for whatever .gitattributes file we can find in any of the higher order stages and use it. Signed-off-by: Elijah Newren <newren@gmail.com>
When we have a modify/delete conflict, but the only change to the modification is e.g. change of line endings, then if renormalization is requested then we should be able to recognize such a case as a not-modified/delete and resolve the conflict automatically. This fixes t6418.10 under GIT_TEST_MERGE_ALGORITHM=ort. Signed-off-by: Elijah Newren <newren@gmail.com>
merge-recursive has some simple code to support subtree shifting; copy it over to merge-ort. This fixes t6409.12 under GIT_TEST_MERGE_ALGORITHM=ort. Signed-off-by: Elijah Newren <newren@gmail.com>
If there is a conflict during a merge for a SKIP_WORKTREE entry, we expect that file to be written to the working copy and have the SKIP_WORKTREE bit cleared in the index. If the user had manually created a file in the working tree despite SKIP_WORKTREE being set, we do not want to clobber their changes to that file, but want to move it out of the way. Add tests that check for these behaviors. Signed-off-by: Elijah Newren <newren@gmail.com>
When merge conflicts occur in paths removed by a sparse-checkout, we need to unsparsify those paths (clear the SKIP_WORKTREE bit), and write out the conflicted file to the working copy. In the very unlikely case that someone manually put a file into the working copy at the location of the SKIP_WORKTREE file, we need to avoid overwriting whatever edits they have made and move that file to a different location first. Signed-off-by: Elijah Newren <newren@gmail.com>
merge-ort handles submodules (and directory/file conflicts in general) differently than merge-recursive does; it basically puts all the special handling for different filetypes into one place in the codebase instead of needing special handling for different filetypes in many different code paths. This one code path in merge-ort could perhaps use some work still (there are still test_expect_failure cases in the testsuite), but it passes all the tests that merge-recursive does as well as 12 additional ones that merge-recursive fails. Mark those 12 tests as test_expect_success under merge-ort. Signed-off-by: Elijah Newren <newren@gmail.com>
There are a variety of questions users might ask while resolving conflicts: * What changes have been made since the previous (first) parent? * What changes are staged? * What is still unstaged? (or what is still conflicted?) * What changes did I make to resolve conflicts so far? The first three of these have simple answers: * git diff HEAD * git diff --cached * git diff There was no way to answer the final question previously. Adding one is trivial in merge-ort, since it works by creating a tree representing what should be written to the working copy complete with conflict markers. Simply write that tree to .git/AUTO_MERGE, allowing users to answer the fourth question with * git diff AUTO_MERGE I avoided using a name like "MERGE_AUTO", because that would be merge-specific (much like MERGE_HEAD, REBASE_HEAD, REVERT_HEAD, CHERRY_PICK_HEAD) and I wanted a name that didn't change depending on which type of operation the merge was part of. Ensure that paths which clean out other temporary operation-specific files (e.g. CHERRY_PICK_HEAD, MERGE_MSG, rebase-merge/ state directory) also clean out this AUTO_MERGE file. Signed-off-by: Elijah Newren <newren@gmail.com>
The plan is to just delete merge-recursive, but not until everyone is comfortable with merge-ort as a replacement. Given that I haven't switched all callers of merge-recursive over yet (e.g. git-am still uses merge-recursive), maybe there's some value documenting known bugs in the algorithm in case we end up keeping it or someone wants to dig it up in the future. Signed-off-by: Elijah Newren <newren@gmail.com>
This reverts commit 5ced7c3, which was put in place as a temporary measure to avoid optimizations unstably erroring out on no destination having a majority of the necessary renames for directories that had no new files and thus no need for directory rename detection anyway. Now that optimizations are in place to prevent us from trying to compute directory rename count computations for directories that do not need it, we can undo this temporary measure. Signed-off-by: Elijah Newren <newren@gmail.com>
When we started on merge-ort, thousands of tests failed when run with
the GIT_TEST_MERGE_ALGORITHM=ort flag; with so many, it didn't make
sense to flip all their test expectations. The ones in t6409, t6418,
and the submodule tests are being handled by an independent in-flight
topic ("Complete merge-ort implemenation...almost"). The ones in
t6423 were left out of the other series because other ongoing series
that this commit depends upon were addressing those. Now that we only
have one remaining test failure in t6423, let's mark it as such.
This remaining test will be fixed by a future optimization series, but
since merge-recursive doesn't pass this test either, passing it is not
necessary for declaring merge-ort ready for general use.
Signed-off-by: Elijah Newren <newren@gmail.com>
|
/submit |
|
Submitted as pull.905.v2.git.1616016485.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
In preparation for switching from merge-recursive to merge-ort as the default strategy, have the testsuite default to running with merge-ort. Keep coverage of the recursive backend by having the linux-gcc job run with it. Signed-off-by: Elijah Newren <newren@gmail.com>
|
This branch is now known as |
|
This patch series was integrated into seen via git@bab461d. |
|
This patch series was integrated into seen via git@dacf9fb. |
|
This patch series was integrated into seen via git@2c6478c. |
|
This patch series was integrated into seen via git@40cd16d. |
|
There was a status update about the branch Plug the ort merge backend throughout the rest of the system, and start testing it as a replacement for the recursive backend. |
|
This patch series was integrated into seen via git@a7bd703. |
|
This patch series was integrated into seen via git@4cc959d. |
|
This patch series was integrated into seen via git@eb270b5. |
|
This patch series was integrated into seen via git@8246dda. |
|
This patch series was integrated into seen via git@6086de4. |
|
This patch series was integrated into seen via git@c0ed33e. |
|
This patch series was integrated into seen via git@3e5feb4. |
|
There was a status update about the branch Plug the ort merge backend throughout the rest of the system, and start testing it as a replacement for the recursive backend. |
|
This patch series was integrated into seen via git@517283a. |
|
This patch series was integrated into seen via git@f917097. |
|
This patch series was integrated into seen via git@c25b67b. |
|
This patch series was integrated into seen via git@1f9af43. |
|
There was a status update about the branch Plug the ort merge backend throughout the rest of the system, and start testing it as a replacement for the recursive backend. Will merge to 'next'? |
|
This patch series was integrated into seen via git@ed9ef8f. |
|
This patch series was integrated into next via git@20283a3. |
|
There was a status update in the "Cooking" section about the branch Plug the ort merge backend throughout the rest of the system, and start testing it as a replacement for the recursive backend. Will merge to 'next'. |
|
This patch series was integrated into seen via git@e443742. |
|
This patch series was integrated into seen via git@13e74a9. |
|
There was a status update in the "Cooking" section about the branch Plug the ort merge backend throughout the rest of the system, and start testing it as a replacement for the recursive backend. Will merge to 'master'. |
|
This patch series was integrated into seen via git@7bec8e7. |
|
This patch series was integrated into next via git@7bec8e7. |
|
This patch series was integrated into master via git@7bec8e7. |
|
Closed via 7bec8e7. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This series depends on ort-perf-batch-10[1], and obsoletes the ort-remainder topic[2] (that hadn't been picked up yet, so hopefully this doesn't cause any confusion)
With this series, merge-ort is ready for general usage -- it passes all tests, passes dozens of tests that don't under merge-recursive, and merge-ort is is already significantly faster than merge-recursive when rename detection is involved. Users can select merge-ort by (a) passing
-sortto eithergit mergeorgit rebase, or (b) by setting pull.twohead=ort [3], or (c) by setting GIT_TEST_MERGE_ALGORITHM=ort.Changes since v2:
[1] https://lore.kernel.org/git/pull.853.git.1615674128.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com/
[3] See commit 14c4586 ("merge,rebase,revert: select ort or recursive by config or environment", 2020-11-02)
cc: Derrick Stolee dstolee@microsoft.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Taylor Blau me@ttaylorr.com
cc: Jonathan Tan jonathantanmy@google.com
cc: Jeff King peff@peff.net
cc: Jonathan Nieder jrnieder@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Junio C Hamano gitster@pobox.com
cc: Derrick Stolee stolee@gmail.com
cc: Elijah Newren newren@gmail.com