{"thread":{"id":"40522","subject":"[PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","startedAt":"2015-10-08T20:35:08Z","lastAt":"2015-10-13T11:48:16Z","messageCount":22,"participants":["Johannes Schindelin","Junio C Hamano","Paul Tan","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"271352","messageId":"cover.1444336120.git.johannes.schindelin@gmx.de","threadId":"40522","inReplyTo":null,"subject":"[PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T20:35:08Z","receivedAt":"2015-10-08T20:35:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Brendan Forster noticed that we no longer see the helpful message after\na failed `git pull --rebase`. It turns out that the builtin `am` calls\nthe recursive merge function directly, not via a separate process. But\nthat function was not really safe to be called that way, as it die()s\npretty liberally.\n\nAs a consequence, the code that wanted to see whether the merge failed\nis not even executed, and the helpful message advising how to fix the\nmess is not displayed.\n\nThis topic branch fixes this.\n\nPlease note that there are a couple of unhandled die() calls in\nmerge-recursive.c, most of which indicate code paths that should\nnever be reached (except in the case of a bug).\n\nBut there are two other functions that can die(): `update_file_flags()`\n(which returns void) and `merge_file_1()`.\n\nThe latter function is already nested quite deeply so that the code\nwould have to be made much uglier to handle the `gentle` flag. It is\nalso not quite clear to me whether those error cases can be hit in a\nregular `git pull --rebase` (which is what I really care about most).\n\nAs `update_file_flags()` is called by functions returning void and\nthat are again called in turn by other functions that also return void,\nfixing this part is more involved, so I would like to avoid it, unless\nit is deemed absolutely necessary to address in this patch series.\n\n\nJohannes Schindelin (2):\n  merge_recursive_options: introduce the \"gentle\" flag\n  pull --rebase: reinstate helpful message on abort\n\n builtin/am.c      |  1 +\n merge-recursive.c | 44 +++++++++++++++++++++++++++++++++++---------\n merge-recursive.h |  1 +\n 3 files changed, 37 insertions(+), 9 deletions(-)\n\n-- \n2.6.1.windows.1\n"},{"id":"271353","messageId":"fed6b32cff01f38196e3dd43025076a47f841a74.1444336120.git.johannes.schindelin@gmx.de","threadId":"40522","inReplyTo":"cover.1444336120.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] merge_recursive_options: introduce the \"gentle\" flag","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T20:35:19Z","receivedAt":"2015-10-08T20:35:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Traditionally, all of Git's operations were intended as single\nexecutables to be run once and exit, not as library functions with\ncareful error code paths. Therefore, it was okay for those operations\nto simply die() with an error.\n\nHowever, this assumption no longer holds true: builtin `am` calls\nmerge_recursive_generic() as a regular library function whose return\nvalue indicates whether there was a problem.\n\nThroughout Git's source code, that paradigm (to return an error instead\nof die()ing) is called \"gentle\".\n\nLet's introduce this flag and heed it in as many places as is easily\ndone.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n merge-recursive.c | 44 +++++++++++++++++++++++++++++++++++---------\n merge-recursive.h |  1 +\n 2 files changed, 36 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 44d85be..37528c9 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -266,8 +266,12 @@ struct tree *write_tree_from_memory(struct merge_options *o)\n \t\tactive_cache_tree = cache_tree();\n \n \tif (!cache_tree_fully_valid(active_cache_tree) &&\n-\t    cache_tree_update(&the_index, 0) < 0)\n-\t\tdie(_(\"error building trees\"));\n+\t    cache_tree_update(&the_index, 0) < 0) {\n+\t\tif (!o->gentle)\n+\t\t\tdie(_(\"error building trees\"));\n+\t\terror(_(\"error building trees\"));\n+\t\treturn NULL;\n+\t}\n \n \tresult = lookup_tree(active_cache_tree->sha1);\n \n@@ -712,6 +716,8 @@ static int make_room_for_path(struct merge_options *o, const char *path)\n \t\t\terror(msg, path, _(\": perhaps a D/F conflict?\"));\n \t\t\treturn -1;\n \t\t}\n+\t\tif (o->gentle)\n+\t\t\treturn error(msg, path, \"\");\n \t\tdie(msg, path, \"\");\n \t}\n \n@@ -1340,8 +1346,11 @@ static int process_renames(struct merge_options *o,\n \t\t\tconst char *ren2_src = ren2->pair->one->path;\n \t\t\tconst char *ren2_dst = ren2->pair->two->path;\n \t\t\tenum rename_type rename_type;\n-\t\t\tif (strcmp(ren1_src, ren2_src) != 0)\n+\t\t\tif (strcmp(ren1_src, ren2_src) != 0) {\n+\t\t\t\tif (o->gentle)\n+\t\t\t\t\treturn error(\"ren1_src != ren2_src\");\n \t\t\t\tdie(\"ren1_src != ren2_src\");\n+\t\t\t}\n \t\t\tren2->dst_entry->processed = 1;\n \t\t\tren2->processed = 1;\n \t\t\tif (strcmp(ren1_dst, ren2_dst) != 0) {\n@@ -1374,8 +1383,11 @@ static int process_renames(struct merge_options *o,\n \t\t\tchar *ren2_dst;\n \t\t\tren2 = lookup->util;\n \t\t\tren2_dst = ren2->pair->two->path;\n-\t\t\tif (strcmp(ren1_dst, ren2_dst) != 0)\n+\t\t\tif (strcmp(ren1_dst, ren2_dst) != 0) {\n+\t\t\t\tif (o->gentle)\n+\t\t\t\t\treturn error(\"ren1_dst != ren2_dst\");\n \t\t\t\tdie(\"ren1_dst != ren2_dst\");\n+\t\t\t}\n \n \t\t\tclean_merge = 0;\n \t\t\tren2->processed = 1;\n@@ -1818,6 +1830,11 @@ int merge_trees(struct merge_options *o,\n \tcode = git_merge_trees(o->call_depth, common, head, merge);\n \n \tif (code != 0) {\n+\t\tif (o->gentle)\n+\t\t\treturn error(_(\"merging of trees %s and %s failed\"),\n+\t\t\t    sha1_to_hex(head->object.sha1),\n+\t\t\t    sha1_to_hex(merge->object.sha1));\n+\n \t\tif (show(o, 4) || o->call_depth)\n \t\t\tdie(_(\"merging of trees %s and %s failed\"),\n \t\t\t    sha1_to_hex(head->object.sha1),\n@@ -1864,8 +1881,8 @@ int merge_trees(struct merge_options *o,\n \telse\n \t\tclean = 1;\n \n-\tif (o->call_depth)\n-\t\t*result = write_tree_from_memory(o);\n+\tif (o->call_depth && !(*result = write_tree_from_memory(o)))\n+\t\treturn -1;\n \n \treturn clean;\n }\n@@ -1940,14 +1957,18 @@ int merge_recursive(struct merge_options *o,\n \t\tsaved_b2 = o->branch2;\n \t\to->branch1 = \"Temporary merge branch 1\";\n \t\to->branch2 = \"Temporary merge branch 2\";\n-\t\tmerge_recursive(o, merged_common_ancestors, iter->item,\n-\t\t\t\tNULL, &merged_common_ancestors);\n+\t\tif (merge_recursive(o, merged_common_ancestors, iter->item,\n+\t\t\t\tNULL, &merged_common_ancestors) < 0)\n+\t\t\treturn -1;\n \t\to->branch1 = saved_b1;\n \t\to->branch2 = saved_b2;\n \t\to->call_depth--;\n \n-\t\tif (!merged_common_ancestors)\n+\t\tif (!merged_common_ancestors) {\n+\t\t\tif (o->gentle)\n+\t\t\t\treturn error(_(\"merge returned no commit\"));\n \t\t\tdie(_(\"merge returned no commit\"));\n+\t\t}\n \t}\n \n \tdiscard_cache();\n@@ -1957,6 +1978,8 @@ int merge_recursive(struct merge_options *o,\n \to->ancestor = \"merged common ancestors\";\n \tclean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,\n \t\t\t    &mrtree);\n+\tif (clean < 0)\n+\t\treturn clean;\n \n \tif (o->call_depth) {\n \t\t*result = make_virtual_commit(mrtree, \"merged tree\");\n@@ -2013,6 +2036,9 @@ int merge_recursive_generic(struct merge_options *o,\n \thold_locked_index(lock, 1);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n+\tif (clean < 0)\n+\t\treturn clean;\n+\n \tif (active_cache_changed &&\n \t    write_locked_index(&the_index, lock, COMMIT_LOCK))\n \t\treturn error(_(\"Unable to write index.\"));\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 9e090a3..06f9b7e 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -15,6 +15,7 @@ struct merge_options {\n \tconst char *subtree_shift;\n \tunsigned buffer_output : 1;\n \tunsigned renormalize : 1;\n+\tunsigned gentle : 1;\n \tlong xdl_opts;\n \tint verbosity;\n \tint diff_rename_limit;\n-- \n2.6.1.windows.1\n"},{"id":"271354","messageId":"47e66f159e8929879f22504a180330e3fa020ee9.1444336120.git.johannes.schindelin@gmx.de","threadId":"40522","inReplyTo":"cover.1444336120.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] pull --rebase: reinstate helpful message on abort","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T20:35:30Z","receivedAt":"2015-10-08T20:35:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When calling `git pull --rebase`, things can go wrong. In such a case,\nwe want to tell the user about the most common ways out of this fix:\n\n\tPatch failed at [...]\n\t[...]\n\n\tWhen you have resolved this problem, run \"git rebase\n\t--continue\". If you prefer to skip this patch, run \"git rebase\n\t--skip\" instead. To check out the original branch and stop\n\trebasing, run \"git rebase --abort\".\n\nHowever, with the switch to the builtin `git-am` we call the merge\nrecursive function directly, which usually die()s in case of an\nerror. Not what we want.\n\nSo let's set the newly-introduced `gentle` flag to get a chance to\nprint the helpful advice.\n\nReported by Brendan Forster.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/am.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4f77e07..c472937 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1653,6 +1653,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \n \tinit_merge_options(&o);\n \n+\to.gentle = 1;\n \to.branch1 = \"HEAD\";\n \this_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n \to.branch2 = his_tree_name;\n-- \n2.6.1.windows.1\n"},{"id":"271358","messageId":"xmqq612grhg7.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"cover.1444336120.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T00:52:56Z","receivedAt":"2015-10-09T00:52:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Brendan Forster noticed that we no longer see the helpful message after\n> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n> the recursive merge function directly, not via a separate process.\n>\n> But that function was not really safe to be called that way, as it\n> die()s pretty liberally.\n\nIf that is the case, I'd thinkg that we'd prefer, as a regression\nfix to correct \"that\", i.e., let recursive-merge die and let the\ncaller catch its exit status.\n\nPaul, what do you think?\n"},{"id":"271360","messageId":"CACRoPnSPVMt+FtK6bwfa7Z3jBheTEkBnhU+B7qL8JrAsSmAmkQ@mail.gmail.com","threadId":"40522","inReplyTo":"xmqq612grhg7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-10-09T01:40:58Z","receivedAt":"2015-10-09T01:40:58Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>\n>> Brendan Forster noticed that we no longer see the helpful message after\n>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>> the recursive merge function directly, not via a separate process.\n>>\n>> But that function was not really safe to be called that way, as it\n>> die()s pretty liberally.\n\nI'm not too familiar with the merge-recursive.c code, but I was under\nthe impression that it only called die() under fatal conditions. In\ncommon use cases, such as merge conflicts, it just errors out and the\nhelpful error message does get printed. Is there a reproduction recipe\nfor this?\n\nThat said, I do agree that even if we die(), we could try to be more\nhelpful by printing additional helpful instructions.\n\n> If that is the case, I'd thinkg that we'd prefer, as a regression\n> fix to correct \"that\", i.e., let recursive-merge die and let the\n> caller catch its exit status.\n\nWe could do that, but I don't think it would be worth the overhead to\nspawn an additional process for every patch just to print an\nadditional message should merge_recursive() call die().\n\nInstead, stepping back a bit, I wonder if we can extend coverage of\nthe helpful message to all die() calls when running git-am. We could\njust install a die routine with set_die_routine() in builtin/am.c.\nThen, should die() be called anywhere, the helpful error message will\nbe printed as well. fast-import.c and http-backend.c seem to do this.\n\nRegards,\nPaul\n"},{"id":"271380","messageId":"4a99ca8724b3604cbbec48d559e134fd@dscho.org","threadId":"40522","inReplyTo":"CACRoPnSPVMt+FtK6bwfa7Z3jBheTEkBnhU+B7qL8JrAsSmAmkQ@mail.gmail.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-09T09:50:29Z","receivedAt":"2015-10-09T09:50:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio & Paul,\n\nOn 2015-10-09 03:40, Paul Tan wrote:\n> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>\n>>> Brendan Forster noticed that we no longer see the helpful message after\n>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>>> the recursive merge function directly, not via a separate process.\n>>>\n>>> But that function was not really safe to be called that way, as it\n>>> die()s pretty liberally.\n> \n> I'm not too familiar with the merge-recursive.c code, but I was under\n> the impression that it only called die() under fatal conditions. In\n> common use cases, such as merge conflicts, it just errors out and the\n> helpful error message does get printed. Is there a reproduction recipe\n> for this?\n\nYes. Sorry, I should have added that as part of the patch series. Actually, I should have written it *before* making those patches. Because it revealed that the underlying problem is completely different: *Normally* you are correct, if `pull --rebase` fails with a merge conflict, the advice is shown.\n\nThe problem occurs with CR/LF.\n\nI have a reproducer and will wiggle it down to a proper test case.\n\n>> If that is the case, I'd thinkg that we'd prefer, as a regression\n>> fix to correct \"that\", i.e., let recursive-merge die and let the\n>> caller catch its exit status.\n> \n> We could do that, but I don't think it would be worth the overhead to\n> spawn an additional process for every patch just to print an\n> additional message should merge_recursive() call die().\n\nI would hope that we do not go that direction! The benefit of making `am` a builtin was to *avoid* spawning, after all. Let's not make the experience for Windows users *worse* again.\n\n> Instead, stepping back a bit, I wonder if we can extend coverage of\n> the helpful message to all die() calls when running git-am. We could\n> just install a die routine with set_die_routine() in builtin/am.c.\n> Then, should die() be called anywhere, the helpful error message will\n> be printed as well. fast-import.c and http-backend.c seem to do this.\n\nThis looks more like a work-around to me. In general, I think it is not really a good idea to treat each and every code path as if it is safe to die(). That would just preclude the code from being used as a library function.\n\nBut it looks more and more as if the problem lies with the CR/LF handling of Git. Will keep you posted.\n\nCiao,\nDscho\n"},{"id":"271381","messageId":"d89d595a223508896db9303c901e7c30@dscho.org","threadId":"40522","inReplyTo":"4a99ca8724b3604cbbec48d559e134fd@dscho.org","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-09T10:11:47Z","receivedAt":"2015-10-09T10:11:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Me again,\n\nOn 2015-10-09 11:50, Johannes Schindelin wrote:\n> \n> On 2015-10-09 03:40, Paul Tan wrote:\n>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>>\n>>>> Brendan Forster noticed that we no longer see the helpful message after\n>>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>>>> the recursive merge function directly, not via a separate process.\n>>>>\n>>>> But that function was not really safe to be called that way, as it\n>>>> die()s pretty liberally.\n>>\n>> I'm not too familiar with the merge-recursive.c code, but I was under\n>> the impression that it only called die() under fatal conditions. In\n>> common use cases, such as merge conflicts, it just errors out and the\n>> helpful error message does get printed. Is there a reproduction recipe\n>> for this?\n> \n> Yes. Sorry, I should have added that as part of the patch series.\n> Actually, I should have written it *before* making those patches.\n> Because it revealed that the underlying problem is completely\n> different: *Normally* you are correct, if `pull --rebase` fails with a\n> merge conflict, the advice is shown.\n> \n> The problem occurs with CR/LF.\n\nI finally have that test case working, took way longer than I wanted to:\n\n-- snip --\nSubject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice when failing\nAuthor: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate:   Fri Oct 9 11:15:30 2015 +0200\n    \nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex a0013ee..bce332f 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -237,6 +237,18 @@ test_expect_success '--rebase' '\n \ttest new = \"$(git show HEAD:file2)\"\n '\n \n+test_expect_success 'failed --rebase shows advice' '\n+\tgit checkout -b diverging &&\n+\ttest_commit attributes .gitattributes \"* text=auto\" attrs &&\n+\tsha1=\"$(printf \"1\\\\r\\\\n\" | git hash-object -w --stdin)\" &&\n+\tgit update-index --cacheinfo 0644 $sha1 file &&\n+\tgit commit -m v1-with-cr &&\n+\tgit checkout -f -b fails-to-rebase HEAD^ &&\n+\ttest_commit v2-without-cr file \"2\" file2-lf &&\n+\ttest_must_fail git pull --rebase . diverging 2>err >out &&\n+\tgrep \"When you have resolved this problem\" out\n+'\n+\n test_expect_success '--rebase fails with multiple branches' '\n \tgit reset --hard before-rebase &&\n \ttest_must_fail git pull --rebase . copy master 2>err &&\n--\n\nSo the reason is that `unpack_trees()` fails with\n\n    error: Your local changes to the following files would be overwritten by merge:\n\tfile\n    Please, commit your changes or stash them before you can merge.\n\nthen returns -1 to its caller, `git_merge_trees()`, which still returns -1 in turn to *its* caller, `merge_trees()`, which then gives up by die()ing:\n\n    Aborting\n\nI think there is more than one fix necessary to truly address the issue: the underlying problem is that Git handles *committed* CR/LF really badly when the corresponding `.gitattributes` label the file as `text=auto`. In fact, those files are labeled as modified in `git status`. If you change the line endings of them, they are labeled as modified in `git status`. And after a `git reset --hard`, they are *still* labeled as modified in `git status`.\n\nI will try to make some time to continue to work on this later today, but in the meantime I would be relatively happy if we could introduce that gentle flag. It is really a very gentle patch, after all, much gentler than reverting to the heavy-handed spawning of `merge-recursive`.\n\nCiao,\nDscho\n"},{"id":"271385","messageId":"xmqqk2qvq570.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"CACRoPnSPVMt+FtK6bwfa7Z3jBheTEkBnhU+B7qL8JrAsSmAmkQ@mail.gmail.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T18:15:15Z","receivedAt":"2015-10-09T18:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> That said, I do agree that even if we die(), we could try to be more\n> helpful by printing additional helpful instructions.\n>\n>> If that is the case, I'd thinkg that we'd prefer, as a regression\n>> fix to correct \"that\", i.e., let recursive-merge die and let the\n>> caller catch its exit status.\n>\n> We could do that, but I don't think it would be worth the overhead to\n> spawn an additional process for every patch just to print an\n> additional message should merge_recursive() call die().\n\nFor a thing that (1) has to be run every time in the whole operation\nand (2) is a very small operation itself whose runtime cost can be\ndwarfed by cost of spawning on some platforms, it is clearly better\nto run it internally instead of running it via run_command()\ninterface.  This is especially so if it (3) wants to just kill the\nwhole operation when it finds a problem anyway.  For example, it\nwould be crazy to run \"update-ref\" via run_command() in the \"am\"\nthat is rewritten in C.\n\nBut does the same reasoning apply to the use of merge-recursive in\n\"am -3\" codepath, where it (1) runs only as a fallback when straight\napplication of the patch fails, (2) runs a heavy-weight recursive\nmerge machinery, and (3) now a regression is pointed out that it\nwants to do more than just calling die() when there is a problem?\n\nYou seem to be viewing the world in black-and-white and thinking\nthat run_command() is unconditionally bad.  You need to stop doing\nthat.  Instead, view it as another tool that gives a much better\nisolation from the main flow of logic (hence flexiblity) that comes\nwith a bigger cost.  I am not convinced with your \"I don't think it\nwould be worth\".\n\n> Instead, stepping back a bit, I wonder if we can extend coverage of\n> the helpful message to all die() calls when running git-am. We could\n> just install a die routine with set_die_routine() in builtin/am.c.\n> Then, should die() be called anywhere, the helpful error message will\n> be printed as well.\n\nThat could certainly be a valid approach and may give us a better\nend result.  If it works, it could be a change that is localized\nwith a lot less impact.\n\nThanks.\n"},{"id":"271387","messageId":"xmqqbnc7q489.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"47e66f159e8929879f22504a180330e3fa020ee9.1444336120.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] pull --rebase: reinstate helpful message on abort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T18:36:06Z","receivedAt":"2015-10-09T18:36:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> When calling `git pull --rebase`, things can go wrong. In such a case,\n> we want to tell the user about the most common ways out of this fix:\n> ...\n>  builtin/am.c | 1 +\n>  1 file changed, 1 insertion(+)\n\nIt is strange to see a patch to am that does not talk anything about\nit, though.  And looking at the codepath, the issue does not have\nmuch to do with \"pull --rebase\".  It doesn't even have much to do\nwith \"rebase\".  This is purely about \"am -3\" fallback codepath.\n\nBecause fall-back-threeway wants to react to an error (i.e. calls\nmerge_recursive_generic() and wants to use its return value), but\nmerge_recursive_generic() can die, it fails to do so.  It would not\neven run rerere(), for example.\n"},{"id":"271388","messageId":"xmqq4mhzq41e.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"xmqqk2qvq570.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T18:40:13Z","receivedAt":"2015-10-09T18:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Instead, stepping back a bit, I wonder if we can extend coverage of\n>> the helpful message to all die() calls when running git-am. We could\n>> just install a die routine with set_die_routine() in builtin/am.c.\n>> Then, should die() be called anywhere, the helpful error message will\n>> be printed as well.\n>\n> That could certainly be a valid approach and may give us a better\n> end result.  If it works, it could be a change that is localized\n> with a lot less impact.\n\nI looked at the codepath involved, and I do not think that is a\nfeasible way forward in this case.  It is not about a \"helpful\nmessage\" at all.  You would have to do everything that is done in\nthe error codepath in your custom die routine, which does not make\nmuch sense.\n\nI think the most sensible regression fix as the first step at this\npoint is to call it as a separate process, just like the code calls\n\"apply\" as a separate process for each patch.  Optimization can come\nlater when it is shown that it matters---we need to regain\ncorrectness first.\n"},{"id":"271389","messageId":"xmqqzizroorx.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"xmqq4mhzq41e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T18:55:14Z","receivedAt":"2015-10-09T18:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I looked at the codepath involved, and I do not think that is a\n> feasible way forward in this case.  It is not about a \"helpful\n> message\" at all.  You would have to do everything that is done in\n> the error codepath in your custom die routine, which does not make\n> much sense.\n>\n> I think the most sensible regression fix as the first step at this\n> point is to call it as a separate process, just like the code calls\n> \"apply\" as a separate process for each patch.  Optimization can come\n> later when it is shown that it matters---we need to regain\n> correctness first.\n\nDon't get me wrong by the above, though.\n\nI would prefer the endgame to be an efficient implementation of\nmerge_recursive_generic(), a function that you can call without you\nhaving to worry about it dying.\n\nBut the patch in this thread is not that, if I am reading Johannes's\ndescription correctly.  \n\nAnd by calling merge_recursive_generic() instead of spawning\nmerge-recursive via run_command(), your GSoC series introduced a\nregression to \"am -3\".  I'd like to see the regression fixed, and\nspawning merge-recursive is an obviously correct way to do so.\n\nThat is how \"am -3\" did it before builtin/am.c after all.\n\nOnce that is done, the users will not have to worry about this\nregression, and merge_recursive_generic() implementation can be\nimproved separately.  The patch in this thread may serve as a good\nstarting point for that.\n"},{"id":"271390","messageId":"xmqqsi5jojmk.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"xmqq4mhzq41e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T20:46:27Z","receivedAt":"2015-10-09T20:46:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think the most sensible regression fix as the first step at this\n> point is to call it as a separate process, just like the code calls\n> \"apply\" as a separate process for each patch.  Optimization can come\n> later when it is shown that it matters---we need to regain\n> correctness first.\n\nAnd the fix would look like this, I think.\n\nIt passes the test Dscho's 3/2 adds to t5520 ;-) but that is not\nsurprising.\n\n---\nSubject: [PATCH] am -3: do not let failed merge abort the error codepath\n\nWhen \"am\" was rewritten in C, the codepath for falling back to\nthree-way merge was mistakenly made to make an internal call to\nmerge-recursive, disabling the error reporting code for certain\ntypes of errors merge-recursive detects and reports by calling\ndie().\n\nThis is a quick-fix for correctness.  The ideal endgame would be to\nreplace run_command() in run_fallback_merge_recursive() with a\ndirect call after making sure that internal call to merge-recursive\ndoes not die().\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/am.c | 49 +++++++++++++++++++++++++++++++++----------------\n 1 file changed, 33 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4f77e07..c869796 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1590,16 +1590,44 @@ static int build_fake_ancestor(const struct am_state *state, const char *index_f\n }\n \n /**\n+ * Do the three-way merge using fake ancestor, his tree constructed\n+ * from the fake ancestor and the postimage of the patch, and our\n+ * state.\n+ */\n+static int run_fallback_merge_recursive(const struct am_state *state,\n+\t\t\t\t\tunsigned char *orig_tree,\n+\t\t\t\t\tunsigned char *our_tree,\n+\t\t\t\t\tunsigned char *his_tree)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tint status;\n+\n+\tcp.git_cmd = 1;\n+\n+\targv_array_pushf(&cp.env_array, \"GITHEAD_%s=%.*s\",\n+\t\t\t sha1_to_hex(his_tree), linelen(state->msg), state->msg);\n+\tif (state->quiet)\n+\t\targv_array_push(&cp.env_array, \"GIT_MERGE_VERBOSITY=0\");\n+\n+\targv_array_push(&cp.args, \"merge-recursive\");\n+\targv_array_push(&cp.args, sha1_to_hex(orig_tree));\n+\targv_array_push(&cp.args, \"--\");\n+\targv_array_push(&cp.args, sha1_to_hex(our_tree));\n+\targv_array_push(&cp.args, sha1_to_hex(his_tree));\n+\n+\tstatus = run_command(&cp) ? (-1) : 0;\n+\tdiscard_cache();\n+\tread_cache();\n+\treturn status;\n+}\n+\n+/**\n  * Attempt a threeway merge, using index_path as the temporary index.\n  */\n static int fall_back_threeway(const struct am_state *state, const char *index_path)\n {\n \tunsigned char orig_tree[GIT_SHA1_RAWSZ], his_tree[GIT_SHA1_RAWSZ],\n \t\t      our_tree[GIT_SHA1_RAWSZ];\n-\tconst unsigned char *bases[1] = {orig_tree};\n-\tstruct merge_options o;\n-\tstruct commit *result;\n-\tchar *his_tree_name;\n \n \tif (get_sha1(\"HEAD\", our_tree) < 0)\n \t\thashcpy(our_tree, EMPTY_TREE_SHA1_BIN);\n@@ -1651,22 +1679,11 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \t * changes.\n \t */\n \n-\tinit_merge_options(&o);\n-\n-\to.branch1 = \"HEAD\";\n-\this_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n-\to.branch2 = his_tree_name;\n-\n-\tif (state->quiet)\n-\t\to.verbosity = 0;\n-\n-\tif (merge_recursive_generic(&o, our_tree, his_tree, 1, bases, &result)) {\n+\tif (run_fallback_merge_recursive(state, orig_tree, our_tree, his_tree)) {\n \t\trerere(state->allow_rerere_autoupdate);\n-\t\tfree(his_tree_name);\n \t\treturn error(_(\"Failed to merge in the changes.\"));\n \t}\n \n-\tfree(his_tree_name);\n \treturn 0;\n }\n \n-- \n2.6.1-296-ge15092e\n"},{"id":"271391","messageId":"xmqqoag7ojh2.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"d89d595a223508896db9303c901e7c30@dscho.org","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T20:49:45Z","receivedAt":"2015-10-09T20:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> I finally have that test case working, took way longer than I wanted to:\n\nThis certainly fails without any fix and passes either with your\ntwo-patch or a more conservative run_command() fix that I sent\nseparately.\n\nHowever, this new test (becomes 5520.20) seems to break the\nprecondition of some later tests---I didn't dig but 5520.22 (which\nused to be .21) fails after letting this new test run and succeed.\n\n> I think there is more than one fix necessary to truly address the\n> issue: the underlying problem is that Git handles *committed*\n> CR/LF really badly when the corresponding `.gitattributes` label\n> the file as `text=auto`.\n\nYeah, that certainly is the right thing to tackle.\n"},{"id":"271396","messageId":"56189ADC.9040207@web.de","threadId":"40522","inReplyTo":"d89d595a223508896db9303c901e7c30@dscho.org","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-10-10T04:58:04Z","receivedAt":"2015-10-10T04:58:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-10-09 12.11, Johannes Schindelin wrote:\n> Me again,\n> \n> On 2015-10-09 11:50, Johannes Schindelin wrote:\n>>\n>> On 2015-10-09 03:40, Paul Tan wrote:\n>>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>>>\n>>>>> Brendan Forster noticed that we no longer see the helpful message after\n>>>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>>>>> the recursive merge function directly, not via a separate process.\n>>>>>\n>>>>> But that function was not really safe to be called that way, as it\n>>>>> die()s pretty liberally.\n>>>\n>>> I'm not too familiar with the merge-recursive.c code, but I was under\n>>> the impression that it only called die() under fatal conditions. In\n>>> common use cases, such as merge conflicts, it just errors out and the\n>>> helpful error message does get printed. Is there a reproduction recipe\n>>> for this?\n>>\n>> Yes. Sorry, I should have added that as part of the patch series.\n>> Actually, I should have written it *before* making those patches.\n>> Because it revealed that the underlying problem is completely\n>> different: *Normally* you are correct, if `pull --rebase` fails with a\n>> merge conflict, the advice is shown.\n>>\n>> The problem occurs with CR/LF.\n> \n> I finally have that test case working, took way longer than I wanted to:\n> \n> -- snip --\n> Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice when failing\n> Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date:   Fri Oct 9 11:15:30 2015 +0200\n>     \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index a0013ee..bce332f 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -237,6 +237,18 @@ test_expect_success '--rebase' '\n>  \ttest new = \"$(git show HEAD:file2)\"\n>  '\n>  \n> +test_expect_success 'failed --rebase shows advice' '\n> +\tgit checkout -b diverging &&\n> +\ttest_commit attributes .gitattributes \"* text=auto\" attrs &&\n> +\tsha1=\"$(printf \"1\\\\r\\\\n\" | git hash-object -w --stdin)\" &&\n> +\tgit update-index --cacheinfo 0644 $sha1 file &&\n> +\tgit commit -m v1-with-cr &&\n> +\tgit checkout -f -b fails-to-rebase HEAD^ &&\n> +\ttest_commit v2-without-cr file \"2\" file2-lf &&\n> +\ttest_must_fail git pull --rebase . diverging 2>err >out &&\n> +\tgrep \"When you have resolved this problem\" out\n> +'\n> +\n>  test_expect_success '--rebase fails with multiple branches' '\n>  \tgit reset --hard before-rebase &&\n>  \ttest_must_fail git pull --rebase . copy master 2>err &&\n> --\n> \n> So the reason is that `unpack_trees()` fails with\n> \n>     error: Your local changes to the following files would be overwritten by merge:\n> \tfile\n>     Please, commit your changes or stash them before you can merge.\n> \n> then returns -1 to its caller, `git_merge_trees()`, which still returns -1 in turn to *its* caller, `merge_trees()`, which then gives up by die()ing:\n> \n>     Aborting\n> \n> I think there is more than one fix necessary to truly address the issue: the underlying problem is that Git handles *committed* CR/LF really badly when the corresponding `.gitattributes` label the file as `text=auto`. In fact, those files are labeled as modified in `git status`. If you change the line endings of them, they are labeled as modified in `git status`. And after a `git reset --hard`, they are *still* labeled as modified in `git status`.\nThis is related to the normalization feature of Git:\nhttps://www.kernel.org/pub/software/scm/git/docs/gitattributes.html\n  *       text=auto\nThis ensures that all files that Git considers to be text will have normalized\n(LF) line endings in the repository.\n\nThe normalization feature has 2 consequences:\na) - Files will get normalized at the next commit,\n     Line endings of the changed lines are normalized\n     Line endings of unchanged lines are normalized\nb) - Not normalized files will get normalized (at the next commit),\n    even if they are unchanged otherwise.\n\n\nAs Git knows (* text=auto), that files are normalized at the next commit,\nthey will change in the repo, and they are marked as changed already now.\nThis is by design.\n\nThe normalization has been disabled for core.autocrlf = true in commit\nfd6cce9e (Eyvind Bernhardsen   2010-05-19 22:43:10 +0200  207) \t\t\t * This is the\nnew safer autocrlf handling.\n(See convert.c)\n\nI'm in the mood to propose a patch that disables/suppresses the normalization\neven for \"* text=auto\", if a file has CRLF in the repo.\nThis would make core.autocrlf = true do the same as \"* text=auto\".\n\nI'm nearly sure, that this change would break things for some users,\nand improve for others.\n\nCurrently t0027 tests this behavior, and as soon as we have the new\nNNO tests establish, I will propose some cleanups in convert.c\n(without change of behavour), and later to make\ncore.autocrlf = true\nto do the same as\n* text=auto\n\n\n\n\n\n\n\n\n\n\n\n> \n> I will try to make some time to continue to work on this later today, but in the meantime I would be relatively happy if we could introduce that gentle flag. It is really a very gentle patch, after all, much gentler than reverting to the heavy-handed spawning of `merge-recursive`.\n> \n> Ciao,\n> Dscho\n"},{"id":"271397","messageId":"56193760.5010307@web.de","threadId":"40522","inReplyTo":"d89d595a223508896db9303c901e7c30@dscho.org","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-10-10T16:05:52Z","receivedAt":"2015-10-10T16:05:52Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 09.10.15 12:11, Johannes Schindelin wrote:\n> Me again,\n> \n> On 2015-10-09 11:50, Johannes Schindelin wrote:\n>>\n>> On 2015-10-09 03:40, Paul Tan wrote:\n>>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>>>\n>>>>> Brendan Forster noticed that we no longer see the helpful message after\n>>>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>>>>> the recursive merge function directly, not via a separate process.\n>>>>>\n>>>>> But that function was not really safe to be called that way, as it\n>>>>> die()s pretty liberally.\n>>>\n>>> I'm not too familiar with the merge-recursive.c code, but I was under\n>>> the impression that it only called die() under fatal conditions. In\n>>> common use cases, such as merge conflicts, it just errors out and the\n>>> helpful error message does get printed. Is there a reproduction recipe\n>>> for this?\n>>\n>> Yes. Sorry, I should have added that as part of the patch series.\n>> Actually, I should have written it *before* making those patches.\n>> Because it revealed that the underlying problem is completely\n>> different: *Normally* you are correct, if `pull --rebase` fails with a\n>> merge conflict, the advice is shown.\n>>\n>> The problem occurs with CR/LF.\n> \n> I finally have that test case working, took way longer than I wanted to:\n> \n> -- snip --\n> Subject: [PATCH 3/2] Verify that `git pull --rebase` shows the helpful advice when failing\n> Author: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Date:   Fri Oct 9 11:15:30 2015 +0200\n>     \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index a0013ee..bce332f 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -237,6 +237,18 @@ test_expect_success '--rebase' '\n>  \ttest new = \"$(git show HEAD:file2)\"\n>  '\n>  \n> +test_expect_success 'failed --rebase shows advice' '\n> +\tgit checkout -b diverging &&\n> +\ttest_commit attributes .gitattributes \"* text=auto\" attrs &&\n> +\tsha1=\"$(printf \"1\\\\r\\\\n\" | git hash-object -w --stdin)\" &&\n> +\tgit update-index --cacheinfo 0644 $sha1 file &&\n> +\tgit commit -m v1-with-cr &&\n> +\tgit checkout -f -b fails-to-rebase HEAD^ &&\n> +\ttest_commit v2-without-cr file \"2\" file2-lf &&\n> +\ttest_must_fail git pull --rebase . diverging 2>err >out &&\n> +\tgrep \"When you have resolved this problem\" out\n> +'\n> +\n\nOne other question:\nIs it good to mix 2 different things in one test case ?\n\"shows the helpful advice when failing\" is one thing,\nand the problematic CRLF handling another.\n\nDoes it make sense to simply create \"really-modified\" file to test the helpful advice ?\n\nAnd may be another one witch test the CRLF handling, (may be)\n"},{"id":"271442","messageId":"d2116a492e14f07c8969798ce21c78a2@dscho.org","threadId":"40522","inReplyTo":"xmqqbnc7q489.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] pull --rebase: reinstate helpful message on abort","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-12T09:16:15Z","receivedAt":"2015-10-12T09:16:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-09 20:36, Junio C Hamano wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>> When calling `git pull --rebase`, things can go wrong. In such a case,\n>> we want to tell the user about the most common ways out of this fix:\n>> ...\n>>  builtin/am.c | 1 +\n>>  1 file changed, 1 insertion(+)\n> \n> It is strange to see a patch to am that does not talk anything about\n> it, though.  And looking at the codepath, the issue does not have\n> much to do with \"pull --rebase\".  It doesn't even have much to do\n> with \"rebase\".  This is purely about \"am -3\" fallback codepath.\n\nI made it a habit of describing the big picture in commit messages, including the original motivation for the patch. Naturally, it is purely an implementation detail that the bug displayed by `git pull --rebase` is fixed by modifying `am.c`.\n\n> Because fall-back-threeway wants to react to an error (i.e. calls\n> merge_recursive_generic() and wants to use its return value), but\n> merge_recursive_generic() can die, it fails to do so.  It would not\n> even run rerere(), for example.\n\nPrecisely, So the symptom triggering the bug fix was seemingly unrelated to the patch, hence the need for the comprehensive commit message.\n\nSince our tastes seem to differ a bit, maybe we can have the best of both worlds by appending the following paragraph to the commit message?\n\n-- snipsnap --\nThis patch actually fixes a deeper-seated bug where the non-gentle death of the recursive merge would prevent not only the message from being shown but *any* code to run after the failed merge, including rerere.\n"},{"id":"271443","messageId":"ed70803ecd73415f1bbafb68502fbbda@dscho.org","threadId":"40522","inReplyTo":"xmqq4mhzq41e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-12T09:40:56Z","receivedAt":"2015-10-12T09:40:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-09 20:40, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>>> Instead, stepping back a bit, I wonder if we can extend coverage of\n>>> the helpful message to all die() calls when running git-am. We could\n>>> just install a die routine with set_die_routine() in builtin/am.c.\n>>> Then, should die() be called anywhere, the helpful error message will\n>>> be printed as well.\n>>\n>> That could certainly be a valid approach and may give us a better\n>> end result.  If it works, it could be a change that is localized\n>> with a lot less impact.\n> \n> I looked at the codepath involved, and I do not think that is a\n> feasible way forward in this case.  It is not about a \"helpful\n> message\" at all.  You would have to do everything that is done in\n> the error codepath in your custom die routine, which does not make\n> much sense.\n> \n> I think the most sensible regression fix as the first step at this\n> point is to call it as a separate process, just like the code calls\n> \"apply\" as a separate process for each patch.  Optimization can come\n> later when it is shown that it matters---we need to regain\n> correctness first.\n\nI fear that you might underestimate the finality of this \"first step\". If you reintroduce that separate process, not only is it a performance regression, but could we really realistically expect any further steps to happen after that? I do not think so.\n\nAlso, please let me clarify why I called reintroducing the separate process \"heavy-handed\" in an earlier message. As pointed out by Paul, the dying code paths indicate non-recoverable problems, i.e. serious problems that not even a rerere could fix. Modulo bugs, of course, but those bugs need to be fixed and not papered over. The real bug, after all, is that a non-recoverable code path is taken when it should just return with an error code.\n\nReintroducing the separate process would not help the endeavor to fix those code paths. Indeed, if we still had the separate process, I would never have discovered that bug!\n\nAnd we should also keep in mind that this whole story demonstrates the rather serious shortcomings of the mindset we display throughout libgit.a where it does not behave like a library at all. Of course, hindsight is 20/20, so it is all too easy, and not exactly fair, to criticise the short-sightedness of writing code that does not clean up after itself \"because it is a short-running process anyway\". I certainly have contributed to these problems myself! All the more eager am I to help *increase* the number of functions in libgit.a that are reentrant, eventually making libgit.a behave like a true library. And in that light, what you called \"the first step\" appears like it would be a huge step backwards.\n\nIn contrast, introducing the \"gentle\" flag would be a step in the right direction. It is a much lighter stop-gap solution, too.\n\nFor the above reasons, I respectfully remain convinced that reintroducing the separate process would be a mistake.\n\nCiao,\nDscho\n"},{"id":"271444","messageId":"793ef29a6d9c77b4f4be478239b74089@dscho.org","threadId":"40522","inReplyTo":"xmqqzizroorx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-12T09:46:59Z","receivedAt":"2015-10-12T09:46:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-09 20:55, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> I would prefer the endgame to be an efficient implementation of\n> merge_recursive_generic(), a function that you can call without you\n> having to worry about it dying.\n> \n> But the patch in this thread is not that, if I am reading Johannes's\n> description correctly.  \n\nAs pointed out by Paul, the recursive merge should only die() in case of a non-recoverable error so serious that not even rerere can do anything (such as a read error).\n\nThe bug is that a code path for a non-recoverable error is taken.\n\nSpawning the recursive merge would be a work-around making the need to fix that bug less urgent, nothing more. (So would introducing the 'gentle' flag, of course, albeit without the performance regression of spawning a new process.)\n\nCiao,\nDscho\n"},{"id":"271445","messageId":"c9dd5858532f402e24c05b630a517ce9@dscho.org","threadId":"40522","inReplyTo":"56193760.5010307@web.de","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-12T10:45:27Z","receivedAt":"2015-10-12T10:45:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn 2015-10-10 18:05, Torsten Bögershausen wrote:\n> On 09.10.15 12:11, Johannes Schindelin wrote:\n>> Me again,\n>>\n>> On 2015-10-09 11:50, Johannes Schindelin wrote:\n>>>\n>>> On 2015-10-09 03:40, Paul Tan wrote:\n>>>> On Fri, Oct 9, 2015 at 8:52 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>>>>\n>>>>>> Brendan Forster noticed that we no longer see the helpful message after\n>>>>>> a failed `git pull --rebase`. It turns out that the builtin `am` calls\n>>>>>> the recursive merge function directly, not via a separate process.\n>>\n>> [... cut lots of unnecessary text ...]\n>>\n>> +test_expect_success 'failed --rebase shows advice' '\n>> +\tgit checkout -b diverging &&\n>> +\ttest_commit attributes .gitattributes \"* text=auto\" attrs &&\n>> +\tsha1=\"$(printf \"1\\\\r\\\\n\" | git hash-object -w --stdin)\" &&\n>> +\tgit update-index --cacheinfo 0644 $sha1 file &&\n>> +\tgit commit -m v1-with-cr &&\n>> +\tgit checkout -f -b fails-to-rebase HEAD^ &&\n>> +\ttest_commit v2-without-cr file \"2\" file2-lf &&\n>> +\ttest_must_fail git pull --rebase . diverging 2>err >out &&\n>> +\tgrep \"When you have resolved this problem\" out\n>> +'\n>> +\n> \n> One other question:\n> Is it good to mix 2 different things in one test case ?\n> \"shows the helpful advice when failing\" is one thing,\n> and the problematic CRLF handling another.\n\nI do not necessarily test things that work, and have been working for a long time, so no, I do not want to split that into two.\n\nI could trigger the regression only via CR/LF, that is the only reason why CR/LF are used here. I do *not* want to test for anything CR/LF specific.\n\nCiao,\nDscho\n"},{"id":"271491","messageId":"xmqqio6bltkq.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"ed70803ecd73415f1bbafb68502fbbda@dscho.org","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-12T20:28:53Z","receivedAt":"2015-10-12T20:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n>> I think the most sensible regression fix as the first step at this\n>> point is to call it as a separate process, just like the code calls\n>> \"apply\" as a separate process for each patch.  Optimization can come\n>> later when it is shown that it matters---we need to regain\n>> correctness first.\n>\n> I fear that you might underestimate the finality of this \"first\n> step\". If you reintroduce that separate process, not only is it a\n> performance regression, but could we really realistically expect any\n> further steps to happen after that? I do not think so.\n> ...\n> For the above reasons, I respectfully remain convinced that\n> reintroducing the separate process would be a mistake.\n\nI am not saying we should forever do run_command() going forward.\n\nBut I do not want to keep the direct call to merge_recursive() in\n'maint'.  The topic was supposed to be \"rewrite in C\".  I do not\nrecall (and do not feel the need to read \"git log\" output to find\nout) exactly how the series progressed, but a logical progression\nwould have been to run merge-recursive via run_command() like I\nshowed in the quick-fix in an early part of the series, followed by\na patch to turn it into a direct call to merge_recursive() as an\noptimization change.\n\nAnd the latter step turned out to be a regression caused by a\npremature optimization.  If something introduces a regression, it\ngets reverted.  As the scripted version certainly did not make an\ninternal call, we should just run_command().  And that is what we\nwant to have in the stable version people use every day.\n\nThe only thing I am saying is that the change to make a direct call\nshould come on top of the run_command() version with its own\njustification as an optimization patch.\n\nGoing that route may require you to redo your patch, measure\nperformance improvements, ensure there is no unintended fallout in\nother callers and longer term maintainability of the codebaths\ninvolved, write a good log message, etc.  And such a fix to\nmerge_recursive() needs to be cooked sufficiently in 'pu' and\n'next'.  And I view all that as a good thing.  I really hate to see\nthat this premature optimization to come back and bite us again---we\ndidn't see it while reviewing because \"builtin-am: implement --3way\"\nwas done in a single step with premature optimization from the\nbeginning.\n\nNow, there are many reasons why the \"first step\" might turn out to\nbe the permanent optimal solution.\n\nI did an unscientific experiment to rebase each of the 25 topics\nthat are cooking in 'next' on top of 'master'.  Only 3 of them will\nfall back to the three-way merge machinery.  One possible reason why\nthe \"first step\" could stay a good-enough solution is that people\nwould not care and/or notice, because it is not like you are paying\nunnecessary cost to spawn merge-recursive for each and every change.\nIt only kicks in when the patch does not apply.\n\nThen I randomly picked one (jc/merge-drop-old-syntax) of the three\ntopics that does fall back, made it into a patch and ran \"am -3\" on\ntop of 'master' with and without the \"first step\".  The numbers from\n5 runs of each look like this:\n\n    real    0m0.109s                      real    0m0.109s\n    user    0m0.080s                      user    0m0.079s\n    sys     0m0.034s                      sys     0m0.035s\n\n    real    0m0.109s                      real    0m0.105s\n    user    0m0.095s                      user    0m0.087s\n    sys     0m0.018s                      sys     0m0.022s\n\n    real    0m0.109s                      real    0m0.110s\n    user    0m0.075s                      user    0m0.086s\n    sys     0m0.038s                      sys     0m0.028s\n\n    real    0m0.107s                      real    0m0.108s\n    user    0m0.083s                      user    0m0.075s\n    sys     0m0.029s                      sys     0m0.038s\n\n    real    0m0.106s                      real    0m0.108s\n    user    0m0.086s                      user    0m0.090s\n    sys     0m0.025s                      sys     0m0.023s\n\nI am curious to see a similar number on platforms with slower\nrun_command().  From the above numbers alone, I cannot even see\nwhich ones are with run_command(), even though I know the numbers on\nthe right hand side column were taken with run_command() and the\nnumbers on the left hand side column were taken with internal call.\n\nAnother possible reason why the \"first step\" could stay a\ngood-enough solution is that merge_recursive() in itself is a\nheavy-weight operation that the cost of spawning a process is not\neven felt [*1*].  After all, it's not like we are talking about\nspawning the cost of \"update-ref HEAD\" dominating the cost of the\nactual operation.\n\nBy the way, in order to make sure that I am running the correct\nbinary, I did \"strace -f -e execve\" on \"am -3\".  \"mailsplit\" and\n\"mailinfo\", both of which are a lot more likely to be affected by\nthe cost of spawning because they are mostly dumb and straight\ntext-to-text filters, are spawned via run_command() interface.  And\nthen we do \"apply --index\" (which is always done), and after seeing\nthat fail, we do \"apply --build-fake-ancestor\" and \"apply --cached\"\nbefore finally spawning merge-recursive (or making a direct call\ninternally before the \"quick-fix\") when we fall back to threeway.\n\nAmong the run_commands() invocations in the \"am -3\", I do not think\nthe one that spawns \"merge-recursive\" is the most profitable one to\nturn into an internal call.  Libifying mailsplit and/or mailinfo and\nmaking them directly callable may be a lot more useful thing to do\nif you want to avoid run_command().\n\n\n[Footnote]\n\n*1* That is what I am seeing here, but this _is_ platform dependent\nand that is why I said I am curious.\n"},{"id":"271492","messageId":"xmqqeggzltcy.fsf@gitster.mtv.corp.google.com","threadId":"40522","inReplyTo":"d2116a492e14f07c8969798ce21c78a2@dscho.org","subject":"Re: [PATCH 2/2] pull --rebase: reinstate helpful message on abort","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-12T20:33:33Z","receivedAt":"2015-10-12T20:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Hi Junio,\n>\n> On 2015-10-09 20:36, Junio C Hamano wrote:\n>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>> \n>>> When calling `git pull --rebase`, things can go wrong. In such a case,\n>>> we want to tell the user about the most common ways out of this fix:\n>>> ...\n>>>  builtin/am.c | 1 +\n>>>  1 file changed, 1 insertion(+)\n>> \n>> It is strange to see a patch to am that does not talk anything about\n>> it, though.  And looking at the codepath, the issue does not have\n>> much to do with \"pull --rebase\".  It doesn't even have much to do\n>> with \"rebase\".  This is purely about \"am -3\" fallback codepath.\n>\n> I made it a habit of describing the big picture in commit messages,\n> including the original motivation for the patch. Naturally, it is\n> purely an implementation detail that the bug displayed by `git pull\n> --rebase` is fixed by modifying `am.c`.\n\nYup, but that is \"I happened to notice that bug first in a command\nthat uses another command that happens to use this buggy one\".  That\nmay be interesting in the \"peeing in the snow\" sense, but not very\ninteresting in the big picture of ensuring the health of the entire\ncodebase.\n\nThe \"common ways out of this\" helpful message is not even coming\nfrom \"pull --rebase\" or even \"rebase\" in the first place.  What you\nare reinstating helpful message on abort is \"am -3\".\n\n\"This fixes am -3, hence incidentally fixes rebase and hence fixes\npull --rebase, too\" would be the most useful way to describe this\nchange.  The initial report being about \"pull --rebase\" is of much\nlessor importance, I would think.\n"},{"id":"271572","messageId":"0d0983480d85118291a058dcd3ef727d@dscho.org","threadId":"40522","inReplyTo":"xmqqio6bltkq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-13T11:48:16Z","receivedAt":"2015-10-13T11:48:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-12 22:28, Junio C Hamano wrote:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>>> I think the most sensible regression fix as the first step at this\n>>> point is to call it as a separate process, just like the code calls\n>>> \"apply\" as a separate process for each patch.  Optimization can come\n>>> later when it is shown that it matters---we need to regain\n>>> correctness first.\n>>\n>> I fear that you might underestimate the finality of this \"first\n>> step\". If you reintroduce that separate process, not only is it a\n>> performance regression, but could we really realistically expect any\n>> further steps to happen after that? I do not think so.\n>> ...\n>> For the above reasons, I respectfully remain convinced that\n>> reintroducing the separate process would be a mistake.\n> \n> I am not saying we should forever do run_command() going forward.\n\nFine, I will stop arguing about this and go back grumble in my corner.\n\nCiao,\nDscho\n"}]}