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(-) Published-As: https://github.com/dscho/git/releases/tag/am-3-merge-recursive-direct-v1 -- 2.9.0.268.gcabc8b0 base-commit: cf4c2cfe52be5bd973a4838f73a35d3959ce2f43 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html