Skip to content

am-3-merge-recursive-direct-v1

This is the long-awaited re-roll of the attempt to avoid spawning
merge-recursive from the builtin am and use merge_recursive() directly
instead.

As indicated in the message of the final commit, the performance
improvement is modest, if noticable.

The *real* reason for the reroll is that I need a libified recursive
merge to accelerate the interactive rebase by teaching the sequencer to
do rebase -i's grunt work.

In other words, this is one of those 13 (now 14) patch series leading up
to a faster interactive rebase.

It did take quite a while to go through the code "with a fine-toothed
comb", and it did turn up a few surprises.

For example, the recursive merge calls remove_file() a couple of times
but actually does not always care about the return value, and rightfully
so. When updating a working tree, for example, where a file was
replaced by a directory, the code may create the directory first
(implicitly deleting the file) and only then attempt to remove the file,
failing because it is a directory already.

One might argue that this logic is flawed, then, and I would agree.
Right now my focus is on rebase -i and I want to avoid getting
side-tracked by the recursive merge logic. So the clean-up will need to
wait for another day (or week).

We also need to be extra careful to retain backwards-compatibility. The
test script t6022-merge-rename.sh, for example, verifies that `git pull`
exits with status 128 in case of a fatal error. To that end, we need to
make sure that fatal errors are handled by existing (builtin) users via
exit(128) (or die(), which calls exit(128) at the end). New users (such
as a builtin helper doing rebase -i's grunt work) may want to print some
helpful advice what happened and how to get out of this mess before
erroring out.

In contrast to the original attempt at libifying merge_recursive() (as
part of fixing a regression in the builtin am which wanted to print some
advice, but could not, because the merge machinery die()d before it
could), I no longer use the "gently" flag. Better to get it right to
begin with: fatal errors are indicated by a negative return value. No
dying without proper cause.

In an earlier iteration of this patch series which was not sent to the
mailing list, I used the special return vale -128 to indicate "real
fatal errors". This turned out to be unnecessary: returning -1 always,
to indicate that the operation could not complete successfully, is the
appropriate way to handle all errors.

As this patch series touches rather important code, I would really
appreciate thorough reviews, with a particular focus on what regressions
this patch series might introduce.

Johannes Schindelin (8):
  Report bugs consistently
  merge-recursive: clarify code in was_tracked()
  Prepare the builtins for a libified merge_recursive()
  merge_recursive: abort properly upon errors
  merge-recursive: avoid returning a wholesale struct
  merge-recursive: allow write_tree_from_memory() to error out
  merge-recursive: handle return values indicating errors
  merge-recursive: switch to returning errors instead of dying

Junio C Hamano (1):
  am: make a direct call to merge_recursive

 builtin/am.c           |  27 ++--
 builtin/checkout.c     |   4 +-
 builtin/ls-files.c     |   3 +-
 builtin/merge.c        |   4 +
 builtin/update-index.c |   2 +-
 grep.c                 |   8 +-
 imap-send.c            |   2 +-
 merge-recursive.c      | 397 ++++++++++++++++++++++++++++++-------------------
 sequencer.c            |   4 +
 sha1_file.c            |   4 +-
 trailer.c              |   2 +-
 transport.c            |   2 +-
 wt-status.c            |   4 +-
 13 files changed, 279 insertions(+), 184 deletions(-)

Submitted-As: http://mid.gmane.org/cover.1467199553.git.johannes.schindelin@gmx.de
Assets 2