Subject: [PATCH 00/19] Cleanup merge API

From: Elijah Newren <[email-masked]>
Date: Thu, 25 Jul 2019 10:45:52 -0700

Before writing a replacement merge strategy for recursive, I decided
to first cleanup the merge API -- streamlining merge-recursive.h and
making it more readable.  It includes some fixes I noticed along the
way, and the last two patches were some forgotten changes of mine I
rediscovered that had minor textual conflicts before I rebased them on
this series.

    While there are minor textual and semantic dependencies between
    these patches (preventing me from splitting up this series), they
    are logically separate and can be reviewed independently.

Stuff I'd most welcome review on:
  * Is cache-tree.c the right place for write_tree_from_memory()?
    [see patch 7]  Should there be docs on how it differs from
    write_index_as_tree(), already found in cache-tree?  What does
    the latter even do?

Some notes:
  * Applies on master, merges cleanly to next & pu
  * Only patches 3, 5-7 touch anything outside of merge-recursive
  * Patches 1 & 3 are bugfixes, 2 & 4 arguably are too, but all are
    quite minor and do not need to be part of the impending release
    candidates or 2.23.0.
  * I'm going to be out next week (July 29-Aug 3), so I can only
    respond to feedback for the next couple days or it'll have to
    wait until the 5th.

Stuff I didn't address:
  * merge_recursive() empties the commit_list of merge_bases it is
    passed, so the caller is expected to NOT use it afterwards.  Seems
    suboptimal to place such an expectation on the caller.
  * All current callers (3 of them?) of merge_recursive() always pass
    it a specially created reversed-list for the merge_bases.  Some
    history spelunking provides no details on any of these about why;
    it appears that the 2nd and 3rd callers reversed the list because
    the first did, and I'm guessing the first did in an attempt to
    exactly match the git-merge-recursive.py scripts' behavior.  But
    if the API needs them in a reverse order from what people would
    normally expect to pass them in, shouldn't it reverse them itself
    instead of making all callers do it?  Also, the order shouldn't
    matter when there are no conflicts, and when there are conflicts
    it'd only change which side of the conflict markers the made-up
    virtual merge base would list things in.  However, we do have
    tests with recursive virtual merge bases and which test the
    output, and I didn't want to try to clean those all up.  Besides,
    the current order shows nicely when commits are named things like
    "L1", "L2", "R1", "R2" -- it's nice having a well defined left and
    right side.  Wasn't sure what to do yet, so I just punted for now;
    this series is already long enough...

Elijah Newren (19):
  merge-recursive: fix minor memory leak in error condition
  merge-recursive: remove another implicit dependency on the_repository
  Ensure index matches head before invoking merge machinery, round N
  merge-recursive: exit early if index != head
  merge-recursive: don't force external callers to do our logging
  Change call signature of write_tree_from_memory()
  Move write_tree_from_memory() from merge-recursive to cache-tree
  merge-recursive: fix some overly long lines
  merge-recursive: use common name for ancestors/common/base_list
  merge-recursive: rename 'mrtree' to 'result_tree', for clarity
  merge-recursive: rename merge_options argument to opt in header
  merge-recursive: move some definitions around to clean up the header
  merge-recursive: consolidate unnecessary fields in merge_options
  merge-recursive: comment and reorder the merge_options fields
  merge-recursive: split internal fields into a separate struct
  merge-recursive.c: alphabetize include list
  merge-recursive: rename MERGE_RECURSIVE_* to MERGE_VARIANT_*
  merge-recursive: be consistent with assert
  merge-recursive: provide a better label for diff3 common ancestor

 builtin/checkout.c                |   2 +-
 builtin/merge-recursive.c         |   4 +
 builtin/stash.c                   |   2 +
 cache-tree.c                      |  30 ++
 cache-tree.h                      |   2 +
 merge-recursive.c                 | 517 +++++++++++++++++-------------
 merge-recursive.h                 | 119 +++----
 sequencer.c                       |   1 -
 t/t3030-merge-recursive.sh        |   9 +-
 t/t6036-recursive-corner-cases.sh |   8 +-
 t/t6047-diff3-conflict-markers.sh | 191 +++++++++++
 11 files changed, 589 insertions(+), 296 deletions(-)
 create mode 100755 t/t6047-diff3-conflict-markers.sh

-- 
2.22.0.564.gb52f8ae349