[v2,0/6] Small new merge-ort features, prepping for deletion of merge-recursive.[ch]

Message

Elijah Newren via GitGitGadget March 13, 2025, 2:46 a.m. UTC

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.
     +
          Signed-off-by: Elijah Newren <[e-mail]>
      
       ## 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/[e-mail]/
     +    ), so let's start heeding the config and command line settings.
      
          Signed-off-by: Elijah Newren <[e-mail]>
      
     @@ 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()