Debug merge-recursive.[ch] #1898
Open
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
Replace the use of merge_trees() from merge-recursive.[ch] with the merge-ort equivalent. Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
endit-quote-debugging-unquote
branch
from
808991d to
3abcfe6
Compare
|
/submit |
|
Submitted as [email protected] 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}
|
User |
|
User |
|
This patch series was integrated into seen via git@937fe0a. |
Switch from merge-recursive to merge-ort. Adjust the following testcases due to the switch: * t6430: most of the test differences here were due to improved D/F conflict handling explained in more detail in ef52778 (merge tests: expect improved directory/file conflict handling in ort, 2020-10-26). These changes weren't made to this test back in that commit simply because I had been looking at `git merge` rather than `git merge-recursive`. The final test in this testsuite, though, was expunged because it was looking for specific output, and the calls to output_commit_title() were discarded from merge_ort_internal() in its adaptation from merge_recursive_internal(); see 8119214 (merge-ort: implement merge_incore_recursive(), 2020-12-16). * t6434: This test is built entirely around rename/delete conflicts, which had a suboptimal handling under merge-recursive. As explained in more detail in commits 1f3c9ba ("t6425: be more flexible with rename/delete conflict messages", 2020-08-10) and 727c75b ("t6404, t6423: expect improved rename/delete handling in ort backend", 2020-10-26), rename/delete conflicts should each have two entries in the index rather than just one. Adjust the expectations for all the tests in this testcase to see the two entries per rename/delete conflict. * t6424: merge-recursive had a special check-if-toplevel-trees-match check that it ran at the beginning on both the merge-base and the other side being merged in. In such a case, it exited early and printed an "Already up to date." message. merge-ort got rid of this, and instead checks the merge base tree matching the other side throughout the tree instead of just at the toplevel, allowing it to avoid recursing into various subtrees. As part of that, it got rid of the specialty toplevel message. That message hasn't been missed for years from `git merge`, so I don't think it is necessary to keep it just for `git merge-recursive`, especially since the latter is rarely used. (git itself only references it in the testsuite, whereas it used to power one of the three rebase backends that existed once upon a time.) Signed-off-by: Elijah Newren <[email protected]>
The ort merge strategy has always used the histogram diff algorithm. The recursive merge strategy, in contrast, defaults to the myers diff algorithm, while allowing it to be changed. Change the ort merge strategy to allow different diff algorithms, by removing the hard coded value in merge_start() and instead just making it a default in init_merge_options(). Technically, this also changes the default diff algorithm for the recursive backend too, but we're going to remove the final callers of the recursive backend in the next two commits. Signed-off-by: Elijah Newren <[email protected]>
The do_recursive_merge() function, which is somewhat misleadingly named since its purpose in life is to do a *non*-recursive merge, had code to allow either using the recursive or ort backends. The default has been ort for a very long time, let's just remove the code path for allowing the recursive backend to be selected. Signed-off-by: Elijah Newren <[email protected]>
More precisely, replace calls to merge_recursive() with merge_ort_recursive(). Also change t7615 to quit calling out recursive; it is not needed anymore, and we are in fact using ort now. Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
endit-quote-debugging-unquote
branch
from
3abcfe6 to
d1dea98
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <[email protected]> writes:
> This series does some preparation, then moves the code shared between
> merge-recursive and merge-ort from the former to the latter, and then debugs
> the remainder of merge-recursive.[ch].
Help unconfusing me. When we have bugs in our code, the action we
take consists of two parts, i.e. first we find them, and then we fix
them. To me, the verb "debug" refers only to the earlier half, and
never the latter.
But the code in the later part of this series is not only to find or
expose existing bugs, but also fixing them, right?
I've already named the topic with "debug" in its name while queuing
the original iteration of this series, as I was on vacation and did
not want to spend more than minimum braincycles on naming, but now I
am back, I sense that the use of the word, and the proposed log
message for 6/8, are overly suboptimal. If you are referring to
fixing remaining bugs, "Debug the remainder of merge-recursive.[ch]"
is not how we usually describe our fixes.
I suspect that the overall sentiment behind this series is ...
Such and such bugs existed in the older backend, but now the
newer backend is used when the older one is asked, and the
newer backend does not share these bugs, we can simply
remove the buggy code specific to the older backend.
... but I find it somewhat disturbing to stay totally silent about
"such and such bugs existed" part.
Thanks. |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Mon, Apr 7, 2025 at 1:10 PM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>
> > This series does some preparation, then moves the code shared between
> > merge-recursive and merge-ort from the former to the latter, and then debugs
> > the remainder of merge-recursive.[ch].
>
> Help unconfusing me.
It was an attempt at humor, which importantly relied on the sentence
you stripped out immediately before this part you quoted, namely:
> > As a wise man once told me, "Deleted code is debugged code!"
With this sentence as context, "debug the code" was a funny way of
saying "delete the code". Because if it's deleted, we are no longer
affected by any bugs contained within it (and that'd be true for both
known bugs and latent ones we hadn't triggered yet).
> I've already named the topic with "debug" in its name while queuing
> the original iteration of this series, as I was on vacation and did
> not want to spend more than minimum braincycles on naming, but now I
> am back, I sense that the use of the word, and the proposed log
> message for 6/8, are overly suboptimal. If you are referring to
> fixing remaining bugs, "Debug the remainder of merge-recursive.[ch]"
> is not how we usually describe our fixes.
>
> I suspect that the overall sentiment behind this series is ...
>
> Such and such bugs existed in the older backend, but now the
> newer backend is used when the older one is asked, and the
> newer backend does not share these bugs, we can simply
> remove the buggy code specific to the older backend.
I'd say the sentiment is more:
merge-ort was always meant to be a replacement for merge-recursive
(and has various advantages -- worktree-less and index-less
operation, faster, fixes some bugs); let's convert the rest of the
callers over and then clean up by deleting merge-recursive (as
well as removing the extra test scaffolding added long ago to aid
in the conversion).
In other words the bug fixes, while real, were not the thrust of the
story. I showed a handful of people my existing commit message and
cover letter, all of whom found it a humorous way of stating that
we're finally replacing merge-recursive. But, since this intent
wasn't obvious to everyone, I can re-roll with some clarification. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@6372956. |
As a wise man once told me, "Deleted code is debugged code!" So, move the functions that are shared between merge-recursive and merge-ort from the former to the latter, and then debug the remainder of merge-recursive.[ch]. Joking aside, merge-ort was always intended to replace merge-recursive. It has numerous advantages over merge-recursive (operates much faster, can operate without a worktree or index, and fixes a number of known bugs and suboptimal merges). Since we have now replaced all callers of merge-recursive with equivalent functions from merge-ort, move the shared functions from the former to the latter, and delete the remainder of merge-recursive.[ch]. Signed-off-by: Elijah Newren <[email protected]>
Both of these existed to allow us to reuse all the merge-related tests in the testsuite while easily flipping between the 'recursive' and the 'ort' backends. Now that we have removed merge-recursive and remapped 'recursive' to mean 'ort', we don't need this scaffolding anymore. Signed-off-by: Elijah Newren <[email protected]>
This environment variable existed to allow the testsuite to reuse all the merge-related tests in the testsuite while easily flipping between the 'recursive' and the 'ort' backends. Now that we have removed merge-recursive and remapped 'recursive' to mean 'ort', we don't need this scaffolding anymore. Remove it from these three builtins. Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
endit-quote-debugging-unquote
branch
from
d1dea98 to
bf2d462
Compare
|
This patch series was integrated into seen via git@dc31dd1. |
|
/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@aeb0c09. |
|
This patch series was integrated into seen via git@74fbd6e. |
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.
Changes since v1:
Changes since v2.
Clarified cover letter:
As a wise man once told me, "Deleted code is debugged code!"
This series does some preparation, then moves the code shared between merge-recursive and merge-ort from the former to the latter, and then "debugs" the remainder of merge-recursive.[ch].
Joking aside, merge-ort was always intended to replace merge-recursive. It did replace it as the default years ago in commit 6a5fb96 (Change default merge backend from recursive to ort, 2021-08-04), and was noted to be better on correctness, extensibility, and performance issues. In this series, we convert the remaining callers of merge-recursive over to merge-ort equivalents, move the shared code between merge-recursive and merge-ort from the former to the latter, and then delete the remainder of merge-recursive.[ch] as it has now been fully replaced. We then clean up some test framework scaffolding that was put into place to allow us to reuse the recursive tests as ort tests.
Series overview:
While the diffstat might look large, the non-test code changes are actually pretty small. The drivers of the big diffstat are:
cc: Eric Sunshine [email protected]
cc: Elijah Newren [email protected]