| Message ID | pull.1898.v2.git.1743891374.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Debug merge-recursive.[ch] | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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 Mon, Apr 7, 2025 at 1:10 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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.
Changes since v1: * fixed a couple of typos in commit messages Original 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]. Series overview: * Patches 1-5: Preparation; switch remaining callers of merge-recursive.[ch] functions to merge-ort equivalents, and add sole remaining missing feature (diff-algorithm selection) * Patch 6: Nuke merge-recursive.[ch] * Patch 7-8: Cleanup testsuite; we don't need GIT_TEST_MERGE_ALGORITHM anymore While the diffstat might look large, the non-test code changes are actually pretty small. The drivers of the big diffstat are: * We move a significant chunk of shared code from merge-recursive.[ch] to merge-ort.[ch], without modifying it * We delete (the remainder of) merge-recursive.[ch] * We rip out all the temporary GIT_TEST_MERGE_ALGORITHM stuff designed to let us reuse tests between recursive and ort Elijah Newren (8): checkout: replace merge_trees() with merge_ort_nonrecursive() builtin/merge-recursive: switch to using merge_ort_generic() merge-ort: enable diff-algorithms other than histogram sequencer: switch non-recursive merges over to ort merge, sequencer: switch recursive merges over to ort merge-recursive.[ch]: thoroughly debug these tests: remove GIT_TEST_MERGE_ALGORITHM and test_expect_merge_algorithm builtin/{merge,rebase,revert}: remove GIT_TEST_MERGE_ALGORITHM Documentation/merge-strategies.adoc | 51 +- Documentation/technical/sparse-checkout.adoc | 2 - Makefile | 1 - builtin/checkout.c | 10 +- builtin/merge-recursive.c | 4 +- builtin/merge.c | 23 +- builtin/rebase.c | 5 - builtin/revert.c | 2 - ci/run-build-and-tests.sh | 1 - merge-ort-wrappers.h | 2 +- merge-ort.c | 162 +- merge-ort.h | 60 +- merge-recursive.c | 4079 ----------------- merge-recursive.h | 132 - meson.build | 1 - sequencer.c | 58 +- t/lib-merge.sh | 13 - t/t1092-sparse-checkout-compatibility.sh | 2 - t/t2501-cwd-empty.sh | 2 - t/t3512-cherry-pick-submodule.sh | 5 - t/t3513-revert-submodule.sh | 4 - t/t4069-remerge-diff.sh | 7 - t/t4301-merge-tree-write-tree.sh | 7 - t/t5572-pull-submodule.sh | 5 - t/t6400-merge-df.sh | 14 +- t/t6402-merge-rename.sh | 125 +- t/t6404-recursive-merge.sh | 21 +- t/t6406-merge-attr.sh | 7 +- t/t6416-recursive-corner-cases.sh | 194 +- t/t6421-merge-partial-clone.sh | 7 +- t/t6422-merge-rename-corner-cases.sh | 31 +- t/t6423-merge-rename-directories.sh | 517 +-- t/t6424-merge-unrelated-index-changes.sh | 8 +- t/t6426-merge-skip-unneeded-updates.sh | 4 +- t/t6428-merge-conflicts-sparse.sh | 2 - t/t6430-merge-recursive.sh | 46 +- t/t6434-merge-recursive-rename-options.sh | 16 +- t/t6436-merge-overwrite.sh | 17 +- t/t6437-submodule-merge.sh | 65 +- t/t6438-submodule-directory-file-conflicts.sh | 5 - t/t6439-merge-co-error-msgs.sh | 2 +- t/t7402-submodule-rebase.sh | 7 +- t/t7602-merge-octopus-many.sh | 9 +- t/t7610-mergetool.sh | 40 +- t/t7615-diff-algo-with-mergy-operations.sh | 2 - t/test-lib.sh | 2 - 46 files changed, 538 insertions(+), 5241 deletions(-) delete mode 100644 merge-recursive.c delete mode 100644 merge-recursive.h delete mode 100644 t/lib-merge.sh base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1898%2Fnewren%2Fendit-quote-debugging-unquote-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1898/newren/endit-quote-debugging-unquote-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1898 Range-diff vs v1: 1: 0e150ee9065 = 1: 0e150ee9065 checkout: replace merge_trees() with merge_ort_nonrecursive() 2: 44be41da6c5 ! 2: b093d74968b builtin/merge-recursive: switch to using merge_ort_generic() @@ Commit message adaptation from merge_recursive_internal(); see 8119214f4e70 (merge-ort: implement merge_incore_recursive(), 2020-12-16). - * t6436: This test is built entirely around rename/delete conflicts, + * 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 1f3c9ba707 ("t6425: be more flexible with rename/delete conflict messages", 2020-08-10) and 727c75b23f ("t6404, 3: 3945c471b0c = 3: cf774437123 merge-ort: enable diff-algorithms other than histogram 4: 39ff4860fcd = 4: 6203589ac17 sequencer: switch non-recursive merges over to ort 5: 91398ffd1e1 = 5: 8821f22d5ea merge, sequencer: switch recursive merges over to ort 6: 6ef536cdbcd = 6: 770f611c2b6 merge-recursive.[ch]: thoroughly debug these 7: 0b6bcd225dc ! 7: a6501ee85fa tests: remove GIT_TEST_MERGE_ALGORITHM and test_expect_merge_algorithm @@ Metadata ## Commit message ## tests: remove GIT_TEST_MERGE_ALGORITHM and test_expect_merge_algorithm - Both of these existed to allow use to reuse all the merge-related tests + 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. 8: 3abcfe6faf0 = 8: d1dea986646 builtin/{merge,rebase,revert}: remove GIT_TEST_MERGE_ALGORITHM