{"thread":{"id":"46014","subject":"`pull --rebase --autostash` fails when fast forward in dirty repo","startedAt":"2017-05-21T05:17:13Z","lastAt":"2017-05-26T05:58:20Z","messageCount":8,"participants":["Tyler Brazier","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"320366","messageId":"CAAZatrCaoB7EXVrCvC9RKmO02G5xcp8GPBaJefHfv7zAXVpL3Q@mail.gmail.com","threadId":"46014","inReplyTo":null,"subject":"`pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Tyler Brazier","fromEmail":"tylerbrazier@gmail.com","sentAt":"2017-05-21T05:17:06Z","receivedAt":"2017-05-21T05:17:13Z","isPatch":false,"sender":{"key":"tylerbrazier@gmail.com","avatar":null},"body":"Hi,\n\nThis script explains and tests what's going on:\nhttps://gist.github.com/tylerbrazier/4478e76fe44bf6657d4d3da6c789531d\n\npull is failing because it shortcuts to --ff-only then calls\nrun_merge(), which does not know how to autostash. Removing the\nshortcut fixes the problem:\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex dd1a4a94e..225a59f5f 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -868,11 +868,6 @@ int cmd_pull(int argc, const char **argv, const\nchar *prefix)\n head = lookup_commit_reference(orig_head.hash);\n commit_list_insert(head, &list);\n merge_head = lookup_commit_reference(merge_heads.oid[0].hash);\n- if (is_descendant_of(merge_head, list)) {\n- /* we can fast-forward this without invoking rebase */\n- opt_ff = \"--ff-only\";\n- return run_merge();\n- }\n return run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n } else {\n return run_merge();\n"},{"id":"320559","messageId":"20170523131231.zqjkymypbilv6tyf@sigill.intra.peff.net","threadId":"46014","inReplyTo":"CAAZatrCaoB7EXVrCvC9RKmO02G5xcp8GPBaJefHfv7zAXVpL3Q@mail.gmail.com","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-23T13:12:31Z","receivedAt":"2017-05-23T13:12:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Junio, whose code this is touching]\n\nOn Sun, May 21, 2017 at 12:17:06AM -0500, Tyler Brazier wrote:\n\n> This script explains and tests what's going on:\n> https://gist.github.com/tylerbrazier/4478e76fe44bf6657d4d3da6c789531d\n> \n> pull is failing because it shortcuts to --ff-only then calls\n> run_merge(), which does not know how to autostash. Removing the\n> shortcut fixes the problem:\n\nSo I guess the ideal solution would be for us to do the autostash\nourselves, run the fast-forward merge, and then pop the stash. In theory\nthat's just \"git stash push\" followed by \"git stash pop\", but looking at\nthe implementation in git-rebase.sh, it looks like it's a little more\ncomplicated than that.\n\nDisabling the optimization sounds like a reasonable interim workaround,\nbut...\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index dd1a4a94e..225a59f5f 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -868,11 +868,6 @@ int cmd_pull(int argc, const char **argv, const\n> char *prefix)\n> head = lookup_commit_reference(orig_head.hash);\n> commit_list_insert(head, &list);\n> merge_head = lookup_commit_reference(merge_heads.oid[0].hash);\n> - if (is_descendant_of(merge_head, list)) {\n> - /* we can fast-forward this without invoking rebase */\n> - opt_ff = \"--ff-only\";\n> - return run_merge();\n> - }\n\n...we can probably restrict it to when autostash is in use, like:\n\n /*\n * If this is a fast-forward, we can skip calling rebase and\n * just do the merge ourselves. But we don't know about\n * autostash, so use the real rebase command when it's in effect.\n */\n if (!autostash && is_descendant_of(merge_head, list)) {\n\topt_ff = \"--ff-only\";\n\treturn run_merge();\n }\n\nAFAICT from the commit introducing this code (33b842a1e9), and the\nsurrounding discussion:\n\n http://public-inbox.org/git/OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D@dakosy.de/T/#u\n\nthis is purely an optimization to avoid invoking rebase, so it's OK to\nskip (and interestingly, the autostash question was raised in that\nthread but not resolved).\n\nBut I notice on the run_merge() code path that we do still invoke\ngit-merge. And rebase has been getting faster as it is moved to C code\nitself. So is this optimization even worth doing anymore?\n\n-Peff\n"},{"id":"320602","messageId":"xmqqa863jiyf.fsf@gitster.mtv.corp.google.com","threadId":"46014","inReplyTo":"20170523131231.zqjkymypbilv6tyf@sigill.intra.peff.net","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-23T22:40:08Z","receivedAt":"2017-05-23T22:40:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...we can probably restrict it to when autostash is in use, like:\n>\n> /*\n> * If this is a fast-forward, we can skip calling rebase and\n> * just do the merge ourselves. But we don't know about\n> * autostash, so use the real rebase command when it's in effect.\n> */\n> if (!autostash && is_descendant_of(merge_head, list)) {\n> \topt_ff = \"--ff-only\";\n> \treturn run_merge();\n> }\n>\n> AFAICT from the commit introducing this code (33b842a1e9), and the\n> surrounding discussion:\n>\n> http://public-inbox.org/git/OF95D98CB6.47969C1C-ONC1257FE1.0058D980-C1257FE1.0059986D@dakosy.de/T/#u\n\nSounds like a sensible solution.\n\n> But I notice on the run_merge() code path that we do still invoke\n> git-merge.\n\n... wouldn't that what we want even when the merge happens to be a\nfast-forward one? I am not sure what you meant by this, but...\n\n> And rebase has been getting faster as it is moved to C code\n> itself. So is this optimization even worth doing anymore?\n\n...that might be something worth thinking about---my gut feeling\ntells me something but we should go by a measurement, not by gut\nfeeling of a random somebody.\n\nThanks.\n\n"},{"id":"320717","messageId":"20170525180407.ni2oed5wk3qsd2ch@sigill.intra.peff.net","threadId":"46014","inReplyTo":"xmqqa863jiyf.fsf@gitster.mtv.corp.google.com","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-25T18:04:07Z","receivedAt":"2017-05-25T18:04:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 24, 2017 at 07:40:08AM +0900, Junio C Hamano wrote:\n\n> > But I notice on the run_merge() code path that we do still invoke\n> > git-merge.\n> \n> ... wouldn't that what we want even when the merge happens to be a\n> fast-forward one? I am not sure what you meant by this, but...\n\nI just meant that if the point of the optimization was to avoid invoking\ngit-rebase (because it's slow), then we're still not optimizing out a\nprocess. It only helps at all because \"rebase\" (being a shell script)\nmay be slower to start and realize it's a fast-forward than \"merge\".\n\nBut once that is no longer true of git-rebase, then there is no purpose\nto the optimization.\n\n> > And rebase has been getting faster as it is moved to C code\n> > itself. So is this optimization even worth doing anymore?\n> \n> ...that might be something worth thinking about---my gut feeling\n> tells me something but we should go by a measurement, not by gut\n> feeling of a random somebody.\n\nYeah, I'd agree. I had the impression the original change was motivated\nby gut feeling.\n\n-Peff\n"},{"id":"320718","messageId":"20170525182230.552dif62zqxuufk3@sigill.intra.peff.net","threadId":"46014","inReplyTo":"20170525180407.ni2oed5wk3qsd2ch@sigill.intra.peff.net","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-25T18:22:31Z","receivedAt":"2017-05-25T18:22:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 25, 2017 at 02:04:07PM -0400, Jeff King wrote:\n\n> > ...that might be something worth thinking about---my gut feeling\n> > tells me something but we should go by a measurement, not by gut\n> > feeling of a random somebody.\n> \n> Yeah, I'd agree. I had the impression the original change was motivated\n> by gut feeling.\n\nHmph. On Linux, at least, I do not see that using \"git merge\" to\nfast-forward is appreciably faster:\n\nHere are timings for:\n\n git reset --hard HEAD~10 && git pull --rebase\n\nin a git.git repo, using builds of git from various commits (all\nbest-of-five; the timings include the reset for simplicity, but\npresumably it costs the same in each case):\n\n - 33b842a1e^ (just prior to the switch to git-merge)\n real 0m0.256s\n user 0m0.096s\n sys\t 0m0.020s\n\n - 33b842a1e (using git-merge)\n real 0m0.227s\n user 0m0.092s\n sys\t 0m0.020s\n\nSo a little faster, but there seems to be 20-30ms of noise in my timings\nanyway (the average for the \"prior\" case did seem to be higher, though).\nIt's possible that the difference would be more stark on Windows, where\nthe cost of the shell script would be higher.\n\nThe same test with the current master performs the same as 33b842a1e.\nBut if I then remove the optimization, as Tyler's patch did at the start\nof this thread, the timings are similar to 33b842a1e^.\n\nSo I dunno. It does not seem appreciably faster, but what little speedup\nit does provide is the same even with a more modern rebase. Which is\nprobably because that rebase isn't actually doing much in the first\nplace, so the optimized bits from Dscho's rebase--helper are not kicking\nin yet.\n\nAnyway. All this has shown me is that it's probably pointless to do this\ntiming at all on Linux. Somebody on Windows might get better results.\n\nBut regardless, we need to do something. Correctness must trump\noptimizations, and the question is whether we can throw out the whole\nconditional, or if we should just restrict when it kicks in.\n\n-Peff\n"},{"id":"320776","messageId":"xmqqpoewcy05.fsf@gitster.mtv.corp.google.com","threadId":"46014","inReplyTo":"20170525182230.552dif62zqxuufk3@sigill.intra.peff.net","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-25T23:33:46Z","receivedAt":"2017-05-25T23:33:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Anyway. All this has shown me is that it's probably pointless to do this\n> timing at all on Linux. Somebody on Windows might get better results.\n>\n> But regardless, we need to do something. Correctness must trump\n> optimizations, and the question is whether we can throw out the whole\n> conditional, or if we should just restrict when it kicks in.\n\nYes. I personally do not mind going with the simplest approach.\nThe optimization thing is relatively new and we were perfectly happy\nwithout it before ;-).\n\n"},{"id":"320804","messageId":"CAAZatrCQZq5_Jc=O-6guyKx1BcOcTNOmWoqwGNrbotnK-VXacg@mail.gmail.com","threadId":"46014","inReplyTo":"xmqqpoewcy05.fsf@gitster.mtv.corp.google.com","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Tyler Brazier","fromEmail":"tylerbrazier@gmail.com","sentAt":"2017-05-26T04:38:04Z","receivedAt":"2017-05-26T04:38:11Z","isPatch":false,"sender":{"key":"tylerbrazier@gmail.com","avatar":null},"body":"Does git accept outside pull requests? I wouldn't mind committing the\nfix for this once it's been decided what the fix should be. (It might\nhelp my resume ;)\n\nOn Thu, May 25, 2017 at 6:33 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Anyway. All this has shown me is that it's probably pointless to do this\n>> timing at all on Linux. Somebody on Windows might get better results.\n>>\n>> But regardless, we need to do something. Correctness must trump\n>> optimizations, and the question is whether we can throw out the whole\n>> conditional, or if we should just restrict when it kicks in.\n>\n> Yes. I personally do not mind going with the simplest approach.\n> The optimization thing is relatively new and we were perfectly happy\n> without it before ;-).\n>\n"},{"id":"320806","messageId":"xmqqr2zcb1my.fsf@gitster.mtv.corp.google.com","threadId":"46014","inReplyTo":"CAAZatrCQZq5_Jc=O-6guyKx1BcOcTNOmWoqwGNrbotnK-VXacg@mail.gmail.com","subject":"Re: `pull --rebase --autostash` fails when fast forward in dirty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-26T05:58:13Z","receivedAt":"2017-05-26T05:58:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tyler Brazier writes:\n\n> On Thu, May 25, 2017 at 6:33 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> Anyway. All this has shown me is that it's probably pointless to do this\n>>> timing at all on Linux. Somebody on Windows might get better results.\n>>>\n>>> But regardless, we need to do something. Correctness must trump\n>>> optimizations, and the question is whether we can throw out the whole\n>>> conditional, or if we should just restrict when it kicks in.\n>>\n>> Yes. I personally do not mind going with the simplest approach.\n>> The optimization thing is relatively new and we were perfectly happy\n>> without it before ;-).\n\n[administrivia: please do not top-post]\n\n> Does git accept outside pull requests? I wouldn't mind committing the\n> fix for this once it's been decided what the fix should be. (It might\n> help my resume ;)\n\nPlease see Documentation/SubmittingPatches.\n\nThanks.\n"}]}