{"thread":{"id":"57201","subject":"[PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","startedAt":"2022-01-06T15:03:06Z","lastAt":"2022-01-06T19:36:09Z","messageCount":5,"participants":["John Cai","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"445625","messageId":"20220106150226.77458-1-johncai86@gmail.com","threadId":"57201","inReplyTo":null,"subject":"[PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-06T15:02:26Z","receivedAt":"2022-01-06T15:03:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"On a git pull --rebase, if fast forward is possible we run merge.\nHowever, merge will not honor rebase.autostash if it is configured. This\nhas the unfortunate result of\n\n$ git config rebase.autostash true\n$ git pull --rebase\n\nto ignore the rebase.autostash value.\n\nAllow run_merge() to honor rebase.autostash by passing in\nconfig_autostash if --autostash or --no-autostash flags are not\nexplicitly set.\n\nReported-by: \"Tilman Vogel\" \nCo-authored-by: \"Junio C Hamano\" \nSigned-off-by: \"John Cai\" \n---\n\nNotes:\n Fix a bug that prevents git pull --rebase from honoring the rebase.autostash\n config value.\n\n Changes since V2:\n - fixed Junio's email in trailer\n\n Changes since V1:\n - used simpler fix as proposed by Junio\n - removed redundant test cases\n\n builtin/pull.c | 9 ++++++++-\n t/t5521-pull-options.sh | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 100cbf9fb8..8423e420ee 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \n \t\tif (can_ff) {\n-\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\t/*\n+\t\t\t* We can fast-forward without invoking\n+\t\t\t* rebase, by calling run_merge(). But we\n+\t\t\t* have to allow rebase.autostash=true to kick\n+\t\t\t* in.\n+\t\t\t*/\n+\t\t\tif (opt_autostash < 0)\n+\t\t\t\topt_autostash = config_autostash;\n \t\t\topt_ff = \"--ff-only\";\n \t\t\tret = run_merge();\n \t\t} else {\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 66cfcb09c5..66fac99d2b 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -252,4 +252,16 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\n '\n \n+test_expect_success 'git pull --rebase --autostash succeeds on ff' '\n+\ttest_when_finished \"rm -fr src dst actual\" &&\n+\tgit init src &&\n+\ttest_commit -C src \"initial\" file \"content\" &&\n+\tgit clone src dst &&\n+\ttest_commit -C src --printf \"more_content\" file \"more content\\ncontent\\n\" &&\n+\techo \"dirty\" >>dst/file &&\n+\tgit -C dst pull --rebase --autostash >actual 2>&1 &&\n+\tgrep -q \"Fast-forward\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"445631","messageId":"YdcjCKTrK+W/FCw1@nand.local","threadId":"57201","inReplyTo":"20220106150226.77458-1-johncai86@gmail.com","subject":"Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T17:12:40Z","receivedAt":"2022-01-06T17:12:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 06, 2022 at 10:02:26AM -0500, John Cai wrote:\n> On a git pull --rebase, if fast forward is possible we run merge.\n> However, merge will not honor rebase.autostash if it is configured. This\n> has the unfortunate result of\n>\n> $ git config rebase.autostash true\n> $ git pull --rebase\n>\n> to ignore the rebase.autostash value.\n>\n> Allow run_merge() to honor rebase.autostash by passing in\n> config_autostash if --autostash or --no-autostash flags are not\n> explicitly set.\n>\n> Reported-by: \"Tilman Vogel\" \n> Co-authored-by: \"Junio C Hamano\" \n> Signed-off-by: \"John Cai\" \n> ---\n>\n> Notes:\n> Fix a bug that prevents git pull --rebase from honoring the rebase.autostash\n> config value.\n>\n> Changes since V2:\n> - fixed Junio's email in trailer\n>\n> Changes since V1:\n> - used simpler fix as proposed by Junio\n> - removed redundant test cases\n>\n> builtin/pull.c | 9 ++++++++-\n> t/t5521-pull-options.sh | 12 ++++++++++++\n> 2 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 100cbf9fb8..8423e420ee 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n>\n> \t\tif (can_ff) {\n> -\t\t\t/* we can fast-forward this without invoking rebase */\n> +\t\t\t/*\n> +\t\t\t* We can fast-forward without invoking\n> +\t\t\t* rebase, by calling run_merge(). But we\n> +\t\t\t* have to allow rebase.autostash=true to kick\n> +\t\t\t* in.\n> +\t\t\t*/\n> +\t\t\tif (opt_autostash < 0)\n> +\t\t\t\topt_autostash = config_autostash;\n\nThis looks OK, and prefers the value of autostash given over options\nover the configured one. But it may be a little clearer to construct it\nthat way explicitly (see the conditional \"if (opt_rebase)\" inside of\ncmd_pull()).\n\n> \t\t\topt_ff = \"--ff-only\";\n> \t\t\tret = run_merge();\n> \t\t} else {\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index 66cfcb09c5..66fac99d2b 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -252,4 +252,16 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n> \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\n> '\n>\n> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '\n> +\ttest_when_finished \"rm -fr src dst actual\" &&\n> +\tgit init src &&\n> +\ttest_commit -C src \"initial\" file \"content\" &&\n\nA handful of these arguments do not need extra quotes. It would be fine\nto instead write:\n\n test_commit -C src initial file content &&\n\nbut if you don't care about the commit message being \"content\", you can\nshorten this to just:\n\n test_commit -C src content file &&\n\nsince test_commit defaults all of its arguments to the value of\n\"\".\n\n> +\tgit clone src dst &&\n> +\ttest_commit -C src --printf \"more_content\" file \"more content\\ncontent\\n\" &&\n\nSame note here, but I think the `--printf` is unnecessary. Running\n`echo` with \"\\n\" characters in its argument is fine, so this could be\nshortened to:\n\n test_commit -C src blah file \"more\\ncontent\"\n\nUnfortunately using `--append` isn't an option here, since the \"content\"\nanchor needs to be at the end of the file in order to apply the stash\nwithout conflict.\n\nThanks,\nTaylor\n"},{"id":"445632","messageId":"YdcjWG/67JlxTjfF@nand.local","threadId":"57201","inReplyTo":"YdcjCKTrK+W/FCw1@nand.local","subject":"Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T17:14:00Z","receivedAt":"2022-01-06T17:14:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 06, 2022 at 12:12:40PM -0500, Taylor Blau wrote:\n> On Thu, Jan 06, 2022 at 10:02:26AM -0500, John Cai wrote:\n> > diff --git a/builtin/pull.c b/builtin/pull.c\n> > index 100cbf9fb8..8423e420ee 100644\n> > --- a/builtin/pull.c\n> > +++ b/builtin/pull.c\n> > @@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> > \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n> >\n> > \t\tif (can_ff) {\n> > -\t\t\t/* we can fast-forward this without invoking rebase */\n> > +\t\t\t/*\n> > +\t\t\t* We can fast-forward without invoking\n> > +\t\t\t* rebase, by calling run_merge(). But we\n> > +\t\t\t* have to allow rebase.autostash=true to kick\n> > +\t\t\t* in.\n> > +\t\t\t*/\n> > +\t\t\tif (opt_autostash < 0)\n> > +\t\t\t\topt_autostash = config_autostash;\n>\n> This looks OK, and prefers the value of autostash given over options\n> over the configured one. But it may be a little clearer to construct it\n> that way explicitly (see the conditional \"if (opt_rebase)\" inside of\n> cmd_pull()).\n\nOops, ignore this suggestion. run_merge() looks at the static variable\nopt_autostash, so you really do need to be manipulating it directly.\n\nNevermind: what you wrote here looks fine.\n\nThanks,\nTaylor\n"},{"id":"445637","messageId":"xmqqee5kemxj.fsf@gitster.g","threadId":"57201","inReplyTo":"YdcjCKTrK+W/FCw1@nand.local","subject":"Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T18:31:04Z","receivedAt":"2022-01-06T18:31:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> +\tgit clone src dst &&\n>> +\ttest_commit -C src --printf \"more_content\" file \"more content\\ncontent\\n\" &&\n>\n> Same note here, but I think the `--printf` is unnecessary. Running\n> `echo` with \"\\n\" characters in its argument is fine, so this could be\n> shortened to:\n>\n> test_commit -C src blah file \"more\\ncontent\"\n\nIs that true for everybody's shell, or just dash?\n\n$ bash -c 'echo \"a\\nb\\nc\"'\na\\nb\\nc\n$ dash -c 'echo \"a\\nb\\nc\"'\na\nb\nc\n"},{"id":"445645","messageId":"YddEpUEA9Hnxqam+@nand.local","threadId":"57201","inReplyTo":"xmqqee5kemxj.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-01-06T19:36:05Z","receivedAt":"2022-01-06T19:36:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 06, 2022 at 10:31:04AM -0800, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> +\tgit clone src dst &&\n> >> +\ttest_commit -C src --printf \"more_content\" file \"more content\\ncontent\\n\" &&\n> >\n> > Same note here, but I think the `--printf` is unnecessary. Running\n> > `echo` with \"\\n\" characters in its argument is fine, so this could be\n> > shortened to:\n> >\n> > test_commit -C src blah file \"more\\ncontent\"\n>\n> Is that true for everybody's shell, or just dash?\n>\n> $ bash -c 'echo \"a\\nb\\nc\"'\n> a\\nb\\nc\n> $ dash -c 'echo \"a\\nb\\nc\"'\n> a\n> b\n> c\n\nAh, of course: thanks for the reminder. Do ignore my comment about\n`--printf` being unnecessary, since it is quite necessary depending on\nyour shell.\n\nIn either case, I still think the quoting around \"more_content\" is\nunnecessary (but the same is not true for the string containing\nnewlines).\n\nThanks,\nTaylor\n"}]}