{"thread":{"id":"55915","subject":"[PATCH 3/3] pull: trivial whitespace style fix","startedAt":"2021-06-13T04:59:59Z","lastAt":"2021-06-17T16:49:10Z","messageCount":17,"participants":["Felipe Contreras","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"427211","messageId":"20210613045949.255090-4-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-1-felipe.contreras@gmail.com","subject":"[PATCH 3/3] pull: trivial whitespace style fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T04:59:49Z","receivedAt":"2021-06-13T04:59:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Two spaces unaligned to anything is not part of the coding-style. A\nsingle tab is.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 80e2f55cbc..3e13f81084 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -126,9 +126,9 @@ static struct option pull_options[] = {\n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n \tOPT_CALLBACK_F('r', \"rebase\", &opt_rebase,\n-\t \"(false|true|merges|preserve|interactive)\",\n-\t N_(\"incorporate changes by rebasing rather than merging\"),\n-\t PARSE_OPT_OPTARG, parse_opt_rebase),\n+\t\t\"(false|true|merges|preserve|interactive)\",\n+\t\tN_(\"incorporate changes by rebasing rather than merging\"),\n+\t\tPARSE_OPT_OPTARG, parse_opt_rebase),\n \tOPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n-- \n2.32.0\n\n"},{"id":"427212","messageId":"20210613045949.255090-3-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-1-felipe.contreras@gmail.com","subject":"[PATCH 2/3] pull: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T04:59:48Z","receivedAt":"2021-06-13T05:00:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to store ran_ff. Now it's obvious from the conditionals.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex a22293b7db..80e2f55cbc 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tif (opt_rebase) {\n \t\tint ret = 0;\n-\t\tint ran_ff = 0;\n \n \t\tstruct object_id newbase;\n \t\tstruct object_id upstream;\n@@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (can_ff) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\topt_ff = \"--ff-only\";\n-\t\t\tran_ff = 1;\n \t\t\tret = run_merge();\n-\t\t}\n-\t\tif (!ran_ff)\n+\t\t} else {\n \t\t\tret = run_rebase(&newbase, &upstream);\n+\t\t}\n \n \t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n-- \n2.32.0\n\n"},{"id":"427213","messageId":"20210613045949.255090-1-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":null,"subject":"[PATCH 0/3] pull: obvious fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T04:59:46Z","receivedAt":"2021-06-13T05:00:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"These are obvious fixes that I sent many times in series like [1], but\nfor some reason they were never merged.\n\nThere's absolutely no reason not to merge these.\n\n[1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/\n\nFelipe Contreras (3):\n pull: cleanup autostash check\n pull: trivial cleanup\n pull: trivial whitespace style fix\n\n builtin/pull.c | 26 +++++++++++---------------\n 1 file changed, 11 insertions(+), 15 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"427214","messageId":"20210613045949.255090-2-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-1-felipe.contreras@gmail.com","subject":"[PATCH 1/3] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T04:59:47Z","receivedAt":"2021-06-13T05:00:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently \"git pull --rebase\" takes a shortcut in the case a\nfast-forward merge is possible; run_merge() is called with --ff-only.\n\nHowever, \"git merge\" didn't have an --autostash option, so, when \"git\npull --rebase --autostash\" was called *and* the fast-forward merge\nshortcut was taken, then the pull failed.\n\nThis was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash\nworks in dirty repo, 2017-06-01) by simply skipping the fast-forward\nmerge shortcut.\n\nLater on \"git merge\" learned the --autostash option [a03b55530a\n(merge: teach --autostash option, 2020-04-07)], and so did \"git pull\"\n[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].\n\nTherefore it's not necessary to skip the fast-forward merge shortcut\nanymore when called with --rebase --autostash.\n\nLet's always take the fast-forward merge shortcut by essentially\nreverting f15e7cf5cc.\n\nReviewed-by: Elijah Newren \nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e8927fc2ff..a22293b7db 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct oid_array merge_heads = OID_ARRAY_INIT;\n \tstruct object_id orig_head, curr_head;\n \tstruct object_id rebase_fork_point;\n-\tint autostash;\n \tint rebase_unspecified = 0;\n \tint can_ff;\n \n@@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_oid(\"HEAD\", &orig_head))\n \t\toidclr(&orig_head);\n \n-\tautostash = config_autostash;\n \tif (opt_rebase) {\n+\t\tint autostash = config_autostash;\n \t\tif (opt_autostash != -1)\n \t\t\tautostash = opt_autostash;\n \n@@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n-\t\tif (!autostash) {\n-\t\t\tif (can_ff) {\n-\t\t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\t\topt_ff = \"--ff-only\";\n-\t\t\t\tran_ff = 1;\n-\t\t\t\tret = run_merge();\n-\t\t\t}\n+\n+\t\tif (can_ff) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\tran_ff = 1;\n+\t\t\tret = run_merge();\n \t\t}\n \t\tif (!ran_ff)\n \t\t\tret = run_rebase(&newbase, &upstream);\n-- \n2.32.0\n\n"},{"id":"427354","messageId":"CABPp-BFEc2aQV_kTTiDh3WV5oBwr4tOzO6LOGnMAHudZdXVo6Q@mail.gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-1-felipe.contreras@gmail.com","subject":"Re: [PATCH 0/3] pull: obvious fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-14T14:47:58Z","receivedAt":"2021-06-14T14:49:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n wrote:\n>\n> These are obvious fixes that I sent many times in series like [1], but\n> for some reason they were never merged.\n>\n> There's absolutely no reason not to merge these.\n>\n> [1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/\n\nI was really surprised to see the Reviewed-by on patch 1, and did not\nremember what review I had done. Unfortunately, since your new patch\nseries aren't posted as responses to old ones (see\nhttps://lore.kernel.org/git/CABPp-BEEiPP7AEk4Wexw4_MDHcin2n8xkMowO=OXTn9pNPaG0A@mail.gmail.com/T/#u\nfor an example of what I mean), and since the cover letter you linked\nto didn't reference previous series, there's no trace of where it came\nfrom. I had to go digging to try to find it. Any chance you could\ntweak your posts in the future to help reviewers follow how things\nhave evolved?\n\n> Felipe Contreras (3):\n> pull: cleanup autostash check\n> pull: trivial cleanup\n> pull: trivial whitespace style fix\n>\n> builtin/pull.c | 26 +++++++++++---------------\n> 1 file changed, 11 insertions(+), 15 deletions(-)\n>\n> --\n> 2.32.0\n"},{"id":"427355","messageId":"CABPp-BGstXDbzxpySw7q_jn22HD05MsrZeHNv+kXFHOFS2_WCQ@mail.gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/3] pull: cleanup autostash check","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-14T14:56:54Z","receivedAt":"2021-06-14T14:58:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n wrote:\n>\n> Currently \"git pull --rebase\" takes a shortcut in the case a\n> fast-forward merge is possible; run_merge() is called with --ff-only.\n>\n> However, \"git merge\" didn't have an --autostash option, so, when \"git\n> pull --rebase --autostash\" was called *and* the fast-forward merge\n> shortcut was taken, then the pull failed.\n>\n> This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash\n> works in dirty repo, 2017-06-01) by simply skipping the fast-forward\n> merge shortcut.\n>\n> Later on \"git merge\" learned the --autostash option [a03b55530a\n> (merge: teach --autostash option, 2020-04-07)], and so did \"git pull\"\n> [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].\n>\n> Therefore it's not necessary to skip the fast-forward merge shortcut\n> anymore when called with --rebase --autostash.\n>\n> Let's always take the fast-forward merge shortcut by essentially\n> reverting f15e7cf5cc.\n>\n> Reviewed-by: Elijah Newren \n\nI think you are basing the Reviewed-by on\nhttps://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/;\nis that correct? Messages from folks that they seem to like the patch\nor believe it looks good should be translated into an Acked-by rather\nthan a Reviewed-by; from Documentation/SubmittingPatches:\n\n* `Reviewed-by:`, unlike the other tags, can only be offered by the\n reviewer and means that she is completely satisfied that the patch\n is ready for application. It is usually offered only after a\n detailed review.\n\nSorry for not catching this when you posted v3 & v4 of your earlier\nseries. When your series exploded in size and seemed to just be\naccumulating additional changes you wanted to make in the area that\nweren't in response to reviewer feedback (I wasn't sure why the new\npatches in subsequent rerolls weren't just separate series), I didn't\nhave the bandwidth to keep up and review them, so I just missed it.\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/pull.c | 16 +++++++---------\n> 1 file changed, 7 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index e8927fc2ff..a22293b7db 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> struct oid_array merge_heads = OID_ARRAY_INIT;\n> struct object_id orig_head, curr_head;\n> struct object_id rebase_fork_point;\n> - int autostash;\n> int rebase_unspecified = 0;\n> int can_ff;\n>\n> @@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> if (get_oid(\"HEAD\", &orig_head))\n> oidclr(&orig_head);\n>\n> - autostash = config_autostash;\n> if (opt_rebase) {\n> + int autostash = config_autostash;\n> if (opt_autostash != -1)\n> autostash = opt_autostash;\n>\n> @@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n> submodule_touches_in_range(the_repository, &upstream, &curr_head))\n> die(_(\"cannot rebase with locally recorded submodule modifications\"));\n> - if (!autostash) {\n> - if (can_ff) {\n> - /* we can fast-forward this without invoking rebase */\n> - opt_ff = \"--ff-only\";\n> - ran_ff = 1;\n> - ret = run_merge();\n> - }\n> +\n> + if (can_ff) {\n> + /* we can fast-forward this without invoking rebase */\n> + opt_ff = \"--ff-only\";\n> + ran_ff = 1;\n> + ret = run_merge();\n> }\n> if (!ran_ff)\n> ret = run_rebase(&newbase, &upstream);\n> --\n> 2.32.0\n"},{"id":"427356","messageId":"CABPp-BGuqo9Mrp--ofUP-TnbyBQhuUYU6i3DWHBZr2oKyOu1JA@mail.gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-3-felipe.contreras@gmail.com","subject":"Re: [PATCH 2/3] pull: trivial cleanup","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-14T14:57:50Z","receivedAt":"2021-06-14T14:59:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n wrote:\n>\n> There's no need to store ran_ff. Now it's obvious from the conditionals.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/pull.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index a22293b7db..80e2f55cbc 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>\n> if (opt_rebase) {\n> int ret = 0;\n> - int ran_ff = 0;\n>\n> struct object_id newbase;\n> struct object_id upstream;\n> @@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> if (can_ff) {\n> /* we can fast-forward this without invoking rebase */\n> opt_ff = \"--ff-only\";\n> - ran_ff = 1;\n> ret = run_merge();\n> - }\n> - if (!ran_ff)\n> + } else {\n> ret = run_rebase(&newbase, &upstream);\n> + }\n>\n> if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n> recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n> --\n> 2.32.0\n\nMakes sense.\n"},{"id":"427357","messageId":"CABPp-BFEHcX0CRgbYr4Y_hqiAzM=YUp-wzdtwRdOxcZ-jPWyow@mail.gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-4-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/3] pull: trivial whitespace style fix","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-14T15:03:17Z","receivedAt":"2021-06-14T15:04:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n wrote:\n>\n> Two spaces unaligned to anything is not part of the coding-style. A\n> single tab is.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/pull.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 80e2f55cbc..3e13f81084 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -126,9 +126,9 @@ static struct option pull_options[] = {\n> /* Options passed to git-merge or git-rebase */\n> OPT_GROUP(N_(\"Options related to merging\")),\n> OPT_CALLBACK_F('r', \"rebase\", &opt_rebase,\n> - \"(false|true|merges|preserve|interactive)\",\n> - N_(\"incorporate changes by rebasing rather than merging\"),\n> - PARSE_OPT_OPTARG, parse_opt_rebase),\n> + \"(false|true|merges|preserve|interactive)\",\n> + N_(\"incorporate changes by rebasing rather than merging\"),\n> + PARSE_OPT_OPTARG, parse_opt_rebase),\n> OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,\n> N_(\"do not show a diffstat at the end of the merge\"),\n> PARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> --\n> 2.32.0\n\nNot only does this change bring this code in alignment with the coding\nstyle, it also makes it more consistent with the other code around it.\nNone of the other options parsing in this file used a\ntab-and-two-space indent, so it's curious why this one was added this\nway. Anyway, thanks for fixing.\n"},{"id":"427397","messageId":"xmqqfsxjzzpp.fsf@gitster.g","threadId":"55915","inReplyTo":"CABPp-BFEc2aQV_kTTiDh3WV5oBwr4tOzO6LOGnMAHudZdXVo6Q@mail.gmail.com","subject":"Re: [PATCH 0/3] pull: obvious fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-15T01:25:06Z","receivedAt":"2021-06-15T02:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I was really surprised to see the Reviewed-by on patch 1, and did not\n> remember what review I had done. Unfortunately, ...\n> from. I had to go digging to try to find it. Any chance you could\n> tweak your posts in the future to help reviewers follow how things\n> have evolved?\n\nSounds like a sensible request.\n\nAside from that, I read the three patches and think they are good.\nSo I'll queue them as-is on 'seen' (unless they crash with something\nelse, which is always the case without being said, but it is safer\nto be explicit) but won't merge it down until the above is resolved.\n\nOne way to resolve it is for you to review this round and give a\nrefreshed Reviewed-by ;-)\n\nThanks, both.\n\n"},{"id":"427442","messageId":"60c883c5baf07_e63320830@natae.notmuch","threadId":"55915","inReplyTo":"CABPp-BFEc2aQV_kTTiDh3WV5oBwr4tOzO6LOGnMAHudZdXVo6Q@mail.gmail.com","subject":"Re: [PATCH 0/3] pull: obvious fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T10:41:09Z","receivedAt":"2021-06-15T10:41:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n> wrote:\n> >\n> > These are obvious fixes that I sent many times in series like [1], but\n> > for some reason they were never merged.\n> >\n> > There's absolutely no reason not to merge these.\n> >\n> > [1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/\n> \n> I was really surprised to see the Reviewed-by on patch 1, and did not\n> remember what review I had done. Unfortunately, since your new patch\n> series aren't posted as responses to old ones (see\n> https://lore.kernel.org/git/CABPp-BEEiPP7AEk4Wexw4_MDHcin2n8xkMowO=OXTn9pNPaG0A@mail.gmail.com/T/#u\n> for an example of what I mean), and since the cover letter you linked\n> to didn't reference previous series,\n\nBut my cover letter did reference a previous series:\n\n https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/\n\nSee patch 3, 4, and 5.\n\nThe problem is that these patches (along with many others) were part of\ndifferent series, that I reordered, split, and joined in order to make it\nclear why all of them were needed. When I split them people didn't\nunderstand the context, and when I joined them, suddenly there were too\nmany.\n\n> there's no trace of where it came from. I had to go digging to try to\n> find it. Any chance you could tweak your posts in the future to help\n> reviewers follow how things have evolved?\n\nI always do that, including this series.\n\nIn order to properly dig through all the versions of these particualr 3\npatches it would probably take me an hour, and I don't know how much\nvalue that would provide. I just picked the latest one I could find that\ncontained them.\n\n-- \nFelipe Contreras\n"},{"id":"427445","messageId":"60c887f678c88_e63320846@natae.notmuch","threadId":"55915","inReplyTo":"CABPp-BGstXDbzxpySw7q_jn22HD05MsrZeHNv+kXFHOFS2_WCQ@mail.gmail.com","subject":"Re: [PATCH 1/3] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T10:59:02Z","receivedAt":"2021-06-15T10:59:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras\n> wrote:\n> >\n> > Currently \"git pull --rebase\" takes a shortcut in the case a\n> > fast-forward merge is possible; run_merge() is called with --ff-only.\n> >\n> > However, \"git merge\" didn't have an --autostash option, so, when \"git\n> > pull --rebase --autostash\" was called *and* the fast-forward merge\n> > shortcut was taken, then the pull failed.\n> >\n> > This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash\n> > works in dirty repo, 2017-06-01) by simply skipping the fast-forward\n> > merge shortcut.\n> >\n> > Later on \"git merge\" learned the --autostash option [a03b55530a\n> > (merge: teach --autostash option, 2020-04-07)], and so did \"git pull\"\n> > [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].\n> >\n> > Therefore it's not necessary to skip the fast-forward merge shortcut\n> > anymore when called with --rebase --autostash.\n> >\n> > Let's always take the fast-forward merge shortcut by essentially\n> > reverting f15e7cf5cc.\n> >\n> > Reviewed-by: Elijah Newren \n> \n> I think you are basing the Reviewed-by on\n> https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/;\n> is that correct?\n\nNo, more like:\n\n[1] https://lore.kernel.org/git/20201205195313.1557473-5-felipe.contreras@gmail.com/\n\n> Messages from folks that they seem to like the patch\n> or believe it looks good should be translated into an Acked-by rather\n> than a Reviewed-by; from Documentation/SubmittingPatches:\n\nTo me an acknowledgment means something entirely different, and must be\nexpressly given.\n\n> * `Reviewed-by:`, unlike the other tags, can only be offered by the\n> reviewer and means that she is completely satisfied that the patch\n> is ready for application. It is usually offered only after a\n> detailed review.\n\nYeah, I read that after I sent v3. In this series I simply cherry-picked\nit from a previous series.\n\nI guess I'll just avoid both.\n\n-- \nFelipe Contreras\n"},{"id":"427448","messageId":"60c88a502d323_e633208fc@natae.notmuch","threadId":"55915","inReplyTo":"CAPUEspg_MmerWb7h8MyhgcJXbWrJeeSyeJ7z2S6eHgDfRDPKvA@mail.gmail.com","subject":"Re: [PATCH 1/3] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-15T11:09:04Z","receivedAt":"2021-06-15T11:09:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Carlo Arenas wrote:\n> On Sat, Jun 12, 2021 at 10:11 PM Felipe Contreras <\n> felipe.contreras@gmail.com> wrote:\n> \n> > - autostash = config_autostash;\n> > if (opt_rebase) {\n> > + int autostash = config_autostash;\n> > if (opt_autostash != -1)\n> > autostash = opt_autostash;\n> >\n> \n> since you are reducing the scope of the autostash variable anyway, why\n> not refactor it additionally for clarity with (something like):\n> \n> int autostash = (opt_autostash != -1) ? opt_autostash : config_autostash;\n\nBecause I had like 15 branches on top of this, and wanted 1) to minimize\nmodifications, and 2) to minimize the chance of the patch being\nrejected, and this is how the code was before f15e7cf5cc (pull: ff\n--rebase --autostash works in dirty repo, 2017-06-01), so in theory\nnobody could object.\n\nBut yeah, that version makes sense, and in fact I probably had such\ncleanup in some branch.\n\n-- \nFelipe Contreras\n"},{"id":"427767","messageId":"20210617161710.81730-1-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210613045949.255090-1-felipe.contreras@gmail.com","subject":"[PATCH v2 0/3] pull: obvious fixes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T16:17:07Z","receivedAt":"2021-06-17T16:18:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"These are obvious fixes that I sent many times in series like [1], but\nfor some reason they were never merged.\n\nNo changes since v1, except the removal of a reviewed-by trailer that\nwas not expressly given.\n\n[1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/\n\nFelipe Contreras (3):\n pull: cleanup autostash check\n pull: trivial cleanup\n pull: trivial whitespace style fix\n\n builtin/pull.c | 26 +++++++++++---------------\n 1 file changed, 11 insertions(+), 15 deletions(-)\n\nRange-diff against v1:\n1: f9520dbf78 ! 1: bc5d3227a9 pull: cleanup autostash check\n @@ Commit message\n Let's always take the fast-forward merge shortcut by essentially\n reverting f15e7cf5cc.\n \n - Reviewed-by: Elijah Newren \n Signed-off-by: Felipe Contreras \n \n ## builtin/pull.c ##\n2: e677256db0 = 2: d3c944d2fd pull: trivial cleanup\n3: 34a9e2d50f = 3: aadc7e17dc pull: trivial whitespace style fix\n-- \n2.32.0\n\n"},{"id":"427768","messageId":"20210617161710.81730-2-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210617161710.81730-1-felipe.contreras@gmail.com","subject":"[PATCH v2 1/3] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T16:17:08Z","receivedAt":"2021-06-17T16:18:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently \"git pull --rebase\" takes a shortcut in the case a\nfast-forward merge is possible; run_merge() is called with --ff-only.\n\nHowever, \"git merge\" didn't have an --autostash option, so, when \"git\npull --rebase --autostash\" was called *and* the fast-forward merge\nshortcut was taken, then the pull failed.\n\nThis was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash\nworks in dirty repo, 2017-06-01) by simply skipping the fast-forward\nmerge shortcut.\n\nLater on \"git merge\" learned the --autostash option [a03b55530a\n(merge: teach --autostash option, 2020-04-07)], and so did \"git pull\"\n[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].\n\nTherefore it's not necessary to skip the fast-forward merge shortcut\nanymore when called with --rebase --autostash.\n\nLet's always take the fast-forward merge shortcut by essentially\nreverting f15e7cf5cc.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 16 +++++++---------\n 1 file changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e8927fc2ff..a22293b7db 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct oid_array merge_heads = OID_ARRAY_INIT;\n \tstruct object_id orig_head, curr_head;\n \tstruct object_id rebase_fork_point;\n-\tint autostash;\n \tint rebase_unspecified = 0;\n \tint can_ff;\n \n@@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_oid(\"HEAD\", &orig_head))\n \t\toidclr(&orig_head);\n \n-\tautostash = config_autostash;\n \tif (opt_rebase) {\n+\t\tint autostash = config_autostash;\n \t\tif (opt_autostash != -1)\n \t\t\tautostash = opt_autostash;\n \n@@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n-\t\tif (!autostash) {\n-\t\t\tif (can_ff) {\n-\t\t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\t\topt_ff = \"--ff-only\";\n-\t\t\t\tran_ff = 1;\n-\t\t\t\tret = run_merge();\n-\t\t\t}\n+\n+\t\tif (can_ff) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\tran_ff = 1;\n+\t\t\tret = run_merge();\n \t\t}\n \t\tif (!ran_ff)\n \t\t\tret = run_rebase(&newbase, &upstream);\n-- \n2.32.0\n\n"},{"id":"427769","messageId":"20210617161710.81730-3-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210617161710.81730-1-felipe.contreras@gmail.com","subject":"[PATCH v2 2/3] pull: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T16:17:09Z","receivedAt":"2021-06-17T16:18:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to store ran_ff. Now it's obvious from the conditionals.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex a22293b7db..80e2f55cbc 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tif (opt_rebase) {\n \t\tint ret = 0;\n-\t\tint ran_ff = 0;\n \n \t\tstruct object_id newbase;\n \t\tstruct object_id upstream;\n@@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (can_ff) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\topt_ff = \"--ff-only\";\n-\t\t\tran_ff = 1;\n \t\t\tret = run_merge();\n-\t\t}\n-\t\tif (!ran_ff)\n+\t\t} else {\n \t\t\tret = run_rebase(&newbase, &upstream);\n+\t\t}\n \n \t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n-- \n2.32.0\n\n"},{"id":"427770","messageId":"20210617161710.81730-4-felipe.contreras@gmail.com","threadId":"55915","inReplyTo":"20210617161710.81730-1-felipe.contreras@gmail.com","subject":"[PATCH v2 3/3] pull: trivial whitespace style fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T16:17:10Z","receivedAt":"2021-06-17T16:18:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Two spaces unaligned to anything is not part of the coding-style. A\nsingle tab is.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 80e2f55cbc..3e13f81084 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -126,9 +126,9 @@ static struct option pull_options[] = {\n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n \tOPT_CALLBACK_F('r', \"rebase\", &opt_rebase,\n-\t \"(false|true|merges|preserve|interactive)\",\n-\t N_(\"incorporate changes by rebasing rather than merging\"),\n-\t PARSE_OPT_OPTARG, parse_opt_rebase),\n+\t\t\"(false|true|merges|preserve|interactive)\",\n+\t\tN_(\"incorporate changes by rebasing rather than merging\"),\n+\t\tPARSE_OPT_OPTARG, parse_opt_rebase),\n \tOPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n-- \n2.32.0\n\n"},{"id":"427771","messageId":"CABPp-BG4gCt260MhAgZMO1nuV=VdPfRoQsPaQBzbv0Qi2JujEg@mail.gmail.com","threadId":"55915","inReplyTo":"20210617161710.81730-1-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 0/3] pull: obvious fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-17T16:48:56Z","receivedAt":"2021-06-17T16:49:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 17, 2021 at 9:17 AM Felipe Contreras\n wrote:\n>\n> These are obvious fixes that I sent many times in series like [1], but\n> for some reason they were never merged.\n>\n> No changes since v1, except the removal of a reviewed-by trailer that\n> was not expressly given.\n\nThanks for correcting this.\n"}]}