Small new merge-ort features, prepping for deletion of merge-recursive.[ch]
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest6 patches 1 replyIn review
- v13 patches 13 repliesSuperseded
Changes in v2 from the cover letter
* Patch 1: Added an explanation in the commit message about the one difference between merge_recursive_generic() and merge_ort_generic() -- the label for the ancestor commit
* Patch 2: Linked the relevant discussion in the commit message, fixed a style issue, and added a testcase
* Patch 3: Since the opt->verbosity stuff was an unwanted carryover (due to being partially public API), move the tweak to the merge-ort-wrappers to avoid promoting it
* Added 3 more patches, so folks can see one of the callers of merge_ort_generic().
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/6 | merge-ort: add new merge_ort_generic() functionmerge-ort-wrappers.c merge-ort-wrappers.h merge-ort.c +1 more | +93 | 0 replies | |
| 2/6 | merge-ort: allow rename detection to be disabledDocumentation/ merge-ort.c t/ | +17 | 0 replies | |
| 3/6 | merge-ort: support having merge verbosity be set to 0merge-ort-wrappers.c | +6 | 0 replies | |
| 4/6 | t3650: document bug when directory renames are turned offt/ | +22 | 0 replies | |
| 5/6 | merge-ort: fix merge.directoryRenames=falsemerge-ort.c t/ | +30 | 0 replies | |
| 6/6 | am: switch from merge_recursive_generic() to merge_ort_generic()builtin/ t/ t/ +1 more | +5 | 0 replies |
Reviews trailers per patch
None of the 6 patches has a Reviewed-by or Acked-by of their own; the series has one for all of them.
| Patch | Taylor Blau |
|---|---|
| Series | R-b |
| 1/6 | |
| 2/6 | |
| 3/6 | |
| 4/6 | |
| 5/6 | |
| 6/6 |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH v2 0/6] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]
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:
* The first 3 patches provide small new features to allow us to convert later callers
* Patches 4-5 document and fix a bug with directory rename detection being turned off (since git am always turned off directory rename detection, this is a prerequisite for converting git am)
* Patch 6 converts git am from using merge_recursive_generic() to use the new merge_ort_generic().
Changes since v1:
* Patch 1: Added an explanation in the commit message about the one difference between merge_recursive_generic() and merge_ort_generic() -- the label for the ancestor commit
* Patch 2: Linked the relevant discussion in the commit message, fixed a style issue, and added a testcase
* Patch 3: Since the opt->verbosity stuff was an unwanted carryover (due to being partially public API), move the tweak to the merge-ort-wrappers to avoid promoting it
* Added 3 more patches, so folks can see one of the callers of merge_ort_generic().
Elijah Newren (5):
merge-ort: add new merge_ort_generic() function
merge-ort: allow rename detection to be disabled
merge-ort: support having merge verbosity be set to 0
merge-ort: fix merge.directoryRenames=false
am: switch from merge_recursive_generic() to merge_ort_generic()
Johannes Schindelin (1):
t3650: document bug when directory renames are turned off
Documentation/merge-strategies.adoc | 12 ++--- builtin/am.c | 5 +- merge-ort-wrappers.c | 72 ++++++++++++++++++++++++++++- merge-ort-wrappers.h | 12 +++++ merge-ort.c | 53 ++++++++++++++++++--- merge-ort.h | 5 ++ t/t3650-replay-basics.sh | 22 +++++++++ t/t4151-am-abort.sh | 2 +- t/t4255-am-submodule.sh | 1 - t/t4301-merge-tree-write-tree.sh | 6 +++ t/t6427-diff3-conflict-markers.sh | 2 +- 11 files changed, 172 insertions(+), 20 deletions(-)
base-commit: a36e024e989f4d35f35987a60e3af8022cac3420
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1875%2Fnewren%2Fendit-new-features-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1875/newren/endit-new-features-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1875
Range-diff vs v1:
1: 9f73e54224d ! 1: c2f2e3e0cd6 merge-ort: add new merge_ort_generic() function
@@ Commit message
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
+ 874cf2a60444 (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.
+
## merge-ort-wrappers.c ##
2: 4292b22723f ! 2: f401a8e0967 merge-ort: allow rename detection to be disabled
@@ Commit message
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, so let's start heeding the config and command line
- settings.
+ 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.
@@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt)
if (!possible_renames(renames))
goto cleanup;
-+ if (opt->detect_renames == 0) {
++ if (!opt->detect_renames) {
+ renames->redo_after_renames = 0;
+ renames->cached_pairs_valid_side = 0;
+ goto cleanup;
@@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt)
trace2_region_enter("merge", "regular renames", opt->repo); detection_run |= detect_regular_renames(opt, MERGE_SIDE1);
+
+ ## t/t4301-merge-tree-write-tree.sh ##
+@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Clean merge' '
+ test_cmp expect actual
+ '
+
++# Repeat the previous test, but turn off rename detection ++test_expect_success 'Failed merge without rename detection' '
++ test_must_fail git -c diff.renames=false merge-tree --write-tree side1 side3 >out &&
++ grep "CONFLICT (modify/delete): numbers deleted" out
++'
++
+ test_expect_success 'Content merge and a few conflicts' '
+ git checkout side1^0 &&
+ test_must_fail git merge side2 &&
3: c2a2be336e0 < -: ----------- merge-ort: support having merge verbosity be set to 0 -: ----------- > 3: a508b0a0fe2 merge-ort: support having merge verbosity be set to 0 -: ----------- > 4: fefda4add11 t3650: document bug when directory renames are turned off -: ----------- > 5: b25225c3cab merge-ort: fix merge.directoryRenames=false -: ----------- > 6: 3f4b74eb3b9 am: switch from merge_recursive_generic() to merge_ort_generic()
Signature · 1 line
gitgitgadget