{"thread":{"id":"57182","subject":"[PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash","startedAt":"2022-01-04T21:45:34Z","lastAt":"2022-01-14T23:33:46Z","messageCount":15,"participants":["John Cai","Junio C Hamano","Philippe Blain","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"445477","messageId":"20220104214522.10692-1-johncai86@gmail.com","threadId":"57182","inReplyTo":null,"subject":"[PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-04T21:45:21Z","receivedAt":"2022-01-04T21:45:34Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"NOTE: this is my first patch I'm submitting through git send-email. Please let\nme know if there is some convention that I'm missing. thank you in advance :)\n\nA bug in pull.c causes merge and rebase functions to ignore\nrebase.autostash if it is only set in the config.\n\nThere are a couple of different scenarios that we need to be mindful of:\n\n--autostash passed in through command line\n$ git pull --autostash\n\nmerge/rebase should get --autostashed passed through\n\n--rebase passed in, rebase.autostash set in config\n$ git config rebase.autostash true\n$ git pull --rebase\n\nmerge/rebase should get --autostash from config\n\n--no-autostash passed in\n$ git pull --no-autostash\n\n--no-autostash should be passed into merge/rebase\n\nrebase.autostash set but --rebase not used\n$ git config rebase.autostash true\n$ git pull\n\n--autostash should not be passed into merge but not rebase\n\nThis change adjusts variable names to make it more clear which autostash\nsetting it is modifying, and ensures --autostash is passed into the\nmerge/rebase where appropriate.\n\nJohn Cai (1):\n builtin/pull.c: use config value of autostash\n\n builtin/pull.c | 15 ++++++------\n t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 7 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"445478","messageId":"20220104214522.10692-2-johncai86@gmail.com","threadId":"57182","inReplyTo":"20220104214522.10692-1-johncai86@gmail.com","subject":"[PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-04T21:45:22Z","receivedAt":"2022-01-04T21:45:37Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"A bug in pull.c causes merge and rebase functions to ignore\nrebase.autostash if it is only set in the config.\n\nThere are a couple of different scenarios that we need to be mindful of:\n1. --autostash passed in through command line\n$ git pull --autostash\nmerge/rebase should get --autostashed passed through\n\n2. --rebase passed in, rebase.autostash set in config\n$ git config rebase.autostash true\n$ git pull --rebase\n\nmerge/rebase should get --autostash from config\n\n3. --no-autostash passed in\n$ git pull --no-autostash\n--no-autostash should be passed into merge/rebase\n\n4. rebase.autostash set but --rebase not used\n\n$ git config rebase.autostash true\n$ git pull\n--autostash should not be passed into merge but not rebase\n\nThis change adjusts variable names to make it more clear which autostash\nsetting it is modifying, and ensures --autostash is passed into the\nmerge/rebase where appropriate.\n\nReported-by: \"Tilman Vogel\" \nCo-authored-by: \"Philippe Blain\" \nSigned-Off-by: \"John Cai\" \n---\n builtin/pull.c | 15 ++++++------\n t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 100cbf9fb8..fb700c2d39 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,7 +86,8 @@ static char *opt_ff;\n static char *opt_verify_signatures;\n static char *opt_verify;\n static int opt_autostash = -1;\n-static int config_autostash;\n+static int rebase_autostash = 0;\n+static int cfg_rebase_autostash;\n static int check_trust_level = 1;\n static struct strvec opt_strategies = STRVEC_INIT;\n static struct strvec opt_strategy_opts = STRVEC_INIT;\n@@ -361,7 +362,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)\n \tint status;\n \n \tif (!strcmp(var, \"rebase.autostash\")) {\n-\t\tconfig_autostash = git_config_bool(var, value);\n+\t\tcfg_rebase_autostash = git_config_bool(var, value);\n \t\treturn 0;\n \t} else if (!strcmp(var, \"submodule.recurse\")) {\n \t\trecurse_submodules = git_config_bool(var, value) ?\n@@ -689,7 +690,7 @@ static int run_merge(void)\n \t\tstrvec_push(&args, opt_gpg_sign);\n \tif (opt_autostash == 0)\n \t\tstrvec_push(&args, \"--no-autostash\");\n-\telse if (opt_autostash == 1)\n+\telse if (rebase_autostash == 1 || opt_autostash == 1)\n \t\tstrvec_push(&args, \"--autostash\");\n \tif (opt_allow_unrelated_histories > 0)\n \t\tstrvec_push(&args, \"--allow-unrelated-histories\");\n@@ -901,7 +902,7 @@ static int run_rebase(const struct object_id *newbase,\n \t\tstrvec_push(&args, opt_signoff);\n \tif (opt_autostash == 0)\n \t\tstrvec_push(&args, \"--no-autostash\");\n-\telse if (opt_autostash == 1)\n+\telse if (rebase_autostash == 1)\n \t\tstrvec_push(&args, \"--autostash\");\n \tif (opt_verify_signatures &&\n \t !strcmp(opt_verify_signatures, \"--verify-signatures\"))\n@@ -1038,14 +1039,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\toidclr(&orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = config_autostash;\n+\t\trebase_autostash = cfg_rebase_autostash;\n \t\tif (opt_autostash != -1)\n-\t\t\tautostash = opt_autostash;\n+\t\t\trebase_autostash = opt_autostash;\n \n \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (!autostash)\n+\t\tif (!rebase_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 66cfcb09c5..28f551db8e 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n \ttest_commit -C src two &&\n \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\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_expect_success 'git pull --rebase with 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+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Fast-forward\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n+test_expect_success 'git pull --rebase --autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n+\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n+\techo \"dirty\" >>dst/file &&\n+\tgit -C dst pull --rebase --autostash >actual 2>&1 &&\n+\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n+test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n+\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n+\techo \"dirty\" >>dst/file &&\n+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n \n test_done\n-- \n2.34.1\n\n"},{"id":"445485","messageId":"xmqqbl0r9l0l.fsf@gitster.g","threadId":"57182","inReplyTo":"20220104214522.10692-2-johncai86@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T22:46:50Z","receivedAt":"2022-01-04T22:46:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai writes:\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 100cbf9fb8..fb700c2d39 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -86,7 +86,8 @@ static char *opt_ff;\n> static char *opt_verify_signatures;\n> static char *opt_verify;\n> static int opt_autostash = -1;\n> -static int config_autostash;\n> +static int rebase_autostash = 0;\n> +static int cfg_rebase_autostash;\n\nDo not explicitly initialize statics to '0' (or NULL for that matter).\n\nBut more importantly, I have a feeling that you are making a piece\nof code that is already hard to read impossible to follow by adding\nyet another variable.\n\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index 66cfcb09c5..28f551db8e 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n> \ttest_commit -C src two &&\n> \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\n> '\n> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '\n\nMissing blank line between tests.\n\n\nI thought that the root cause of the problem is that run_merge() is\ncalled even when rebase was asked for (either via pull.rebase=true\nconfiguration or \"pull --rebase\" option), when the other side is a\ndescendant of HEAD. The basic idea behind that behaviour may be\nsound (i.e. if we do not have any of our own development on top of\ntheir history, rebase vs merge shouldn't make any difference while\nfast-forwarding HEAD to their history), except that rebase vs merge\nlook at different configuration variables.\n\nI wonder if the following two-liner patch is a simpler fix that is\neasier to understand. run_merge() decides if we should pass the\n\"--[no-]autostash\" option based on the value of opt_autostash, and\nwe know the value of rebase.autostash in config_autostash variable.\n\nIt appears to pass all four tests your patch adds.\n\n builtin/pull.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/pull.c w/builtin/pull.c\nindex 100cbf9fb8..d459a91a64 100644\n--- c/builtin/pull.c\n+++ w/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 {\n"},{"id":"445493","messageId":"19ce4f49-1b68-0cac-1936-12d451244ab1@gmail.com","threadId":"57182","inReplyTo":"20220104214522.10692-1-johncai86@gmail.com","subject":"Re: [PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-04T23:32:13Z","receivedAt":"2022-01-04T23:32:18Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi John,\n\nLe 2022-01-04 à 16:45, John Cai a écrit :\n> NOTE: this is my first patch I'm submitting through git send-email. Please let\n> me know if there is some convention that I'm missing. thank you in advance :)\n\nUsually, for one-patch series as this one, it's preferred in this project to not\nsend a separate cover letter, but instead to include the cover letter material below\nthe three dash line. You can do that manually when using 'git send-email --annotate',\nor you can use the 'git notes' command along with git format-patch's '--notes'\narguments to include it automatically.\n\nCheers,\nPhilippe.\n"},{"id":"445498","messageId":"fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com","threadId":"57182","inReplyTo":"20220104214522.10692-2-johncai86@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-05T03:40:15Z","receivedAt":"2022-01-05T03:40:19Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi John,\n\nLe 2022-01-04 à 16:45, John Cai a écrit :\n> A bug in pull.c causes merge and rebase functions to ignore\n> rebase.autostash if it is only set in the config.\n\nThe reported bug only affects fast-forwards as far as I understand, so I\ndon't think \"merge and rebase\" is the best wording here. Also, 'functions' is\nnot super clear. The actual functions in the code are 'run_rebase'\nand 'run_merge', if that is what you are referring to. If you\nmean the different underlying \"modes\" of 'git pull', I'd phrase\nit more like \"the underlying 'merge' or 'rebase' invocations\"\nor something like that - but again, only the underlying 'git merge'\nis affected, and only for fast-forwards.\n\n> \n> There are a couple of different scenarios that we need to be mindful of:\n> 1. --autostash passed in through command line\n> $ git pull --autostash\n> merge/rebase should get --autostashed passed through\n> \n> 2. --rebase passed in, rebase.autostash set in config\n> $ git config rebase.autostash true\n> $ git pull --rebase\n> \n> merge/rebase should get --autostash from config\n> \n> 3. --no-autostash passed in\n> $ git pull --no-autostash\n> --no-autostash should be passed into merge/rebase\n> \n> 4. rebase.autostash set but --rebase not used\n> \n> $ git config rebase.autostash true\n> $ git pull\n> --autostash should not be passed into merge but not rebase\n\nUsually we start the commit message by a description of the current behaviour,\nso in the case of a bugfix like here, a description of the exact behaviour\nthat triggers the bug. As Tilman reported, this only affects fast-forwards,\nso that should be mentioned in your commit message.\nWhile what you wrote is not wrong per se (although I'm not sure what you meant\nwith point 4), almost all of the behaviour is\ncorrect, apart from the (rebase + ff) case, so I would focus on that.\n\n> \n> This change adjusts variable names to make it more clear which autostash\n> setting it is modifying, and ensures --autostash is passed into the\n> merge/rebase where appropriate.\n\nAs Junio already pointed out, I'm not sure the changes you propose\nare really clearer... I agree that adding yet another variable is unneeded.\n\n> \n> Reported-by: \"Tilman Vogel\" \n> Co-authored-by: \"Philippe Blain\" \n\nAs I remarked in the other thread, I'd prefer if you remove that trailer.\n\n> Signed-Off-by: \"John Cai\" \n> ---\n> builtin/pull.c | 15 ++++++------\n> t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++\n\nThe existing tests for 'git pull --autostash' are in t5520, so I think it\nmight make more sense to add any new tests there instead of t5521.\n\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index 66cfcb09c5..28f551db8e 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n> \ttest_commit -C src two &&\n> \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\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\nThis seems to test the same thing as t5520's \"--rebase --autostash fast forward\",\nso I don't think it's necessary to add this one.\n\n> +\n> +test_expect_success 'git pull --rebase with 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> +\ttest_config -C dst rebase.autostash true &&\n> +\tgit -C dst pull --rebase >actual 2>&1 &&\n> +\tgrep -q \"Fast-forward\" actual &&\n> +\tgrep -q \"Applied autostash.\" actual\n> +'\n\nOK, this is the actual test that was failing.\n\n> +\n> +test_expect_success 'git pull --rebase --autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n> +\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n> +\techo \"dirty\" >>dst/file &&\n> +\tgit -C dst pull --rebase --autostash >actual 2>&1 &&\n> +\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n> +\tgrep -q \"Applied autostash.\" actual\n> +'\n\nThis seems to test the same thing as t5520's \"pull --rebase --autostash & rebase.autostash unset\"\n\n> +\n> +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n> +\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n> +\techo \"dirty\" >>dst/file &&\n> +\ttest_config -C dst rebase.autostash true &&\n> +\tgit -C dst pull --rebase >actual 2>&1 &&\n> +\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n> +\tgrep -q \"Applied autostash.\" actual\n> +'\n\nThis seems to test the same thing as t5520's\n\"pull --rebase succeeds with dirty working directory and rebase.autostash set\".\n\n\nThanks for working on this ! :)\n\nPhilippe.\n"},{"id":"445499","messageId":"4fe0e850-1c30-da70-5535-e2a4105ba734@gmail.com","threadId":"57182","inReplyTo":"xmqqbl0r9l0l.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-05T03:58:32Z","receivedAt":"2022-01-05T03:58:36Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2022-01-04 à 17:46, Junio C Hamano a écrit :\n> \n> I wonder if the following two-liner patch is a simpler fix that is\n> easier to understand. run_merge() decides if we should pass the\n> \"--[no-]autostash\" option based on the value of opt_autostash, and\n> we know the value of rebase.autostash in config_autostash variable.\n> \n> It appears to pass all four tests your patch adds.\n> \n> builtin/pull.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git c/builtin/pull.c w/builtin/pull.c\n> index 100cbf9fb8..d459a91a64 100644\n> --- c/builtin/pull.c\n> +++ w/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 {\n> \n\nWe already have a quite useless 'int autostash' local variable in cmd_pull\na few lines up, so an equivalent fix, I think, would be the following:\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1cfaf9f343..9f8a8dd716 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\toidclr(&orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = config_autostash;\n-\t\tif (opt_autostash != -1)\n-\t\t\tautostash = opt_autostash;\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n \n \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (!autostash)\n+\t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n\nThanks,\nPhilippe.\n"},{"id":"445500","messageId":"e90dfdb8-afdd-d565-63ab-ce767236f87f@gmail.com","threadId":"57182","inReplyTo":"fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-05T04:02:22Z","receivedAt":"2022-01-05T04:02:26Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi again John,\n\nLe 2022-01-04 à 22:40, Philippe Blain a écrit :\n> Hi John,\n> \n> Le 2022-01-04 à 16:45, John Cai a écrit :\n> \n> Usually we start the commit message by a description of the current behaviour,\n> so in the case of a bugfix like here, a description of the exact behaviour\n> that triggers the bug. As Tilman reported, this only affects fast-forwards,\n> so that should be mentioned in your commit message.\n> While what you wrote is not wrong per se (although I'm not sure what you meant\n> with point 4), almost all of the behaviour is\n> correct, apart from the (rebase + ff) case, so I would focus on that.\n\nI forgot to mention in my previous mail, but in the same vein, I think your\ncommit message title could reflect a little more the bug you are fixing, maybe\nsomething like\n\npull: honor 'rebase.autostash' if rebase fast-forwards\n\nor something similar.\n"},{"id":"445514","messageId":"9e36813e-2573-35c9-442c-f5a06fa6a739@gmail.com","threadId":"57182","inReplyTo":"xmqqbl0r9l0l.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-05T11:21:16Z","receivedAt":"2022-01-05T11:21:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John and Junio\n\nOn 04/01/2022 22:46, Junio C Hamano wrote:\n> John Cai writes:\n> \n>> diff --git a/builtin/pull.c b/builtin/pull.c\n>> index 100cbf9fb8..fb700c2d39 100644\n>> --- a/builtin/pull.c\n>> +++ b/builtin/pull.c\n>> @@ -86,7 +86,8 @@ static char *opt_ff;\n>> static char *opt_verify_signatures;\n>> static char *opt_verify;\n>> static int opt_autostash = -1;\n>> -static int config_autostash;\n>> +static int rebase_autostash = 0;\n>> +static int cfg_rebase_autostash;\n> \n> Do not explicitly initialize statics to '0' (or NULL for that matter).\n> \n> But more importantly, I have a feeling that you are making a piece\n> of code that is already hard to read impossible to follow by adding\n> yet another variable.\n> \n>> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n>> index 66cfcb09c5..28f551db8e 100755\n>> --- a/t/t5521-pull-options.sh\n>> +++ b/t/t5521-pull-options.sh\n>> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '\n>> \ttest_commit -C src two &&\n>> \ttest_must_fail git -C dst pull --no-ff --no-verify --verify\n>> '\n>> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '\n> \n> Missing blank line between tests.\n> \n> \n> I thought that the root cause of the problem is that run_merge() is\n> called even when rebase was asked for (either via pull.rebase=true\n> configuration or \"pull --rebase\" option), when the other side is a\n> descendant of HEAD. The basic idea behind that behaviour may be\n> sound (i.e. if we do not have any of our own development on top of\n> their history, rebase vs merge shouldn't make any difference while\n> fast-forwarding HEAD to their history), except that rebase vs merge\n> look at different configuration variables.\n> \n> I wonder if the following two-liner patch is a simpler fix that is\n> easier to understand. run_merge() decides if we should pass the\n> \"--[no-]autostash\" option based on the value of opt_autostash, and\n> we know the value of rebase.autostash in config_autostash variable.\n> \n> It appears to pass all four tests your patch adds.\n\nI think this is a better approach - it's simpler and it is clear that we \nonly use rebase.autostash when a rebase was requested.\n\nBest Wishes\n\nPhillip\n\n> builtin/pull.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git c/builtin/pull.c w/builtin/pull.c\n> index 100cbf9fb8..d459a91a64 100644\n> --- c/builtin/pull.c\n> +++ w/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 {\n> \n\n"},{"id":"445525","messageId":"nycvar.QRO.7.76.6.2201051649060.7076@tvgsbejvaqbjf.bet","threadId":"57182","inReplyTo":"20220104214522.10692-2-johncai86@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-05T15:50:32Z","receivedAt":"2022-01-05T15:50:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi John,\n\nOn Tue, 4 Jan 2022, John Cai wrote:\n\n> Reported-by: \"Tilman Vogel\" \n> Co-authored-by: \"Philippe Blain\" \n> Signed-Off-by: \"John Cai\" \n\nWe spell the 'o' in 'Signed-off-by' with a lower-case 'o'. That's what\n`git commit -s` does automatically.\n\nWas this the problem why you stopped using GitGitGadget? It would also\nhave helped you avoid the frowned-upon cover letter for single patch\ncontributions.\n\nThe entire point of GitGitGadget is to _not_ force contributors to know\nabout all these things.\n\nCiao,\nDscho\n"},{"id":"445642","messageId":"xmqq35m0el28.fsf@gitster.g","threadId":"57182","inReplyTo":"4fe0e850-1c30-da70-5535-e2a4105ba734@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T19:11:27Z","receivedAt":"2022-01-06T19:11:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> We already have a quite useless 'int autostash' local variable in cmd_pull\n> a few lines up, so an equivalent fix, I think, would be the following:\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 1cfaf9f343..9f8a8dd716 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \t\toidclr(&orig_head);\n> \tif (opt_rebase) {\n> -\t\tint autostash = config_autostash;\n> -\t\tif (opt_autostash != -1)\n> -\t\t\tautostash = opt_autostash;\n> +\t\tif (opt_autostash == -1)\n> +\t\t\topt_autostash = config_autostash;\n> \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n> \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n> -\t\tif (!autostash)\n> +\t\tif (!opt_autostash)\n> \t\t\trequire_clean_work_tree(the_repository,\n> \t\t\t\tN_(\"pull with rebase\"),\n> \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n\nOK, so the idea is to make opt_autostash the _primary_ thing that\nmatters when deciding to do the auto-stashing. We may make it\naffected by the value we read from the configuration, if there is no\ncommand line option that sets it.\n\nIf there is no place that cares about what is in config_autostash\n(which is rebase.autostash; nobody reads merge.autostash in this\ncommand) other than this part, I think that is an even cleaner\napproach than what I sent. I very much like it.\n\nHere is how I would explain your change.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: pull --rebase: honor rebase.autostash even when fast-forwarding\n\n\"pull --rebase\" internally uses the merge machinery when the other\nhistory is a descendant of ours (i.e. perform fast-forward). This\ncame from [1], where the discussion was started from a feature\nrequest to do so. It is a bit hard to read the rationale behind it\nin the discussion, but it seems that it was an established fact for\neverybody involved that does not even need to be mentioned that\nfast-forwarding done with \"rebase\" was much undesirable than done\nwith \"merge\", and more importantly, the result left by \"merge\" is as\ngood as (or better than) that by \"rebase\".\n\nExcept for one thing. Because \"git merge\" does not (and should not)\nhonor rebase.autostash, \"git pull\" needs to read it and forward it\nwhen we use \"git merge\" as a (hopefully better) substitute for \"git\nrebase\" during the fast-forwarding. But we forgot to do so (we only\nadd \"--[no-]autostash\" to the \"git merge\" command when \"git pull\" was\ninvoked with \"--[no-]autostash\" command line option, so that \"git merge\"\ncan honor merge.autostash in such a case).\n\nMake sure \"git merge\" is run with \"--autostash\" when\nrebase.autostash is set and used to fast-forward the history on\nbehalf of \"git rebase\". Incidentally this change also takes care of\nthe case where\n\n - \"git pull --rebase\" (without other command line options) is run\n - \"rebase.autostash\" is not set\n - The history fast-forwards\n\nIn such a case, \"git merge\" is run with an explicit \"--no-autostash\"\nto prevent it from honoring merge.autostash configuration, which is\nwhat we want. After all, we want the \"git merge\" to pretend as if\nit is \"git rebase\" while being used for this purpose.\n\n[1]\nhttps://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/\n\n"},{"id":"446192","messageId":"xmqqee5bfaof.fsf@gitster.g","threadId":"57182","inReplyTo":"xmqq35m0el28.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-14T00:00:48Z","receivedAt":"2022-01-14T00:00:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Here is how I would explain your change.\n\nThis topic is in \"Expecting an ack or two\" state for some time.\n\n - Philippe, are you OK with the attached patch? If so throw your\n Signed-off-by to this thread.\n\n - John, if you find Philippe's implementation a good idea (I think\n it is, as it is simpler and cleaner) after reading and\n understanding it, please throw an Acked-by or Reviewed-by to this\n thread.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nFrom: Philippe Blain \nDate: Tue, 4 Jan 2022 22:58:32 -0500\nSubject: [PATCH] pull --rebase: honor rebase.autostash even when\n fast-forwarding\n\n\"pull --rebase\" internally uses the merge machinery when the other\nhistory is a descendant of ours (i.e. perform fast-forward). This\ncame from [1], where the discussion was started from a feature\nrequest to do so. It is a bit hard to read the rationale behind it\nin the discussion, but it seems that it was an established fact for\neverybody involved that does not even need to be mentioned that\nfast-forwarding done with \"rebase\" was much undesirable than done\nwith \"merge\", and more importantly, the result left by \"merge\" is as\ngood as (or better than) that by \"rebase\".\n\nExcept for one thing. Because \"git merge\" does not (and should not)\nhonor rebase.autostash, \"git pull\" needs to read it and forward it\nwhen we use \"git merge\" as a (hopefully better) substitute for \"git\nrebase\" during the fast-forwarding. But we forgot to do so (we only\nadd \"--[no-]autostash\" to the \"git merge\" command when \"git pull\" was\ninvoked with \"--[no-]autostash\" command line option, so that \"git merge\"\ncan honor merge.autostash in such a case).\n\nMake sure \"git merge\" is run with \"--autostash\" when\nrebase.autostash is set and used to fast-forward the history on\nbehalf of \"git rebase\". Incidentally this change also takes care of\nthe case where\n\n - \"git pull --rebase\" (without other command line options) is run\n - \"rebase.autostash\" is not set\n - The history fast-forwards\n\nIn such a case, \"git merge\" is run with an explicit \"--no-autostash\"\nto prevent it from honoring merge.autostash configuration, which is\nwhat we want. After all, we want the \"git merge\" to pretend as if\nit is \"git rebase\" while being used for this purpose.\n\n[1]\nhttps://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/\n---\n builtin/pull.c | 7 +++---\n t/t5521-pull-options.sh | 52 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1cfaf9f343..9f8a8dd716 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\toidclr(&orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = config_autostash;\n-\t\tif (opt_autostash != -1)\n-\t\t\tautostash = opt_autostash;\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n \n \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (!autostash)\n+\t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 66cfcb09c5..4046a74cad 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -252,4 +252,56 @@ 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_expect_success 'git pull --rebase with 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+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Fast-forward\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n+test_expect_success 'git pull --rebase --autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n+\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n+\techo \"dirty\" >>dst/file &&\n+\tgit -C dst pull --rebase --autostash >actual 2>&1 &&\n+\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n+test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 \"src_content\" file \"src content\\ncontent\\n\" &&\n+\ttest_commit -C dst --append \"dst_content\" file \"dst content\" &&\n+\techo \"dirty\" >>dst/file &&\n+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Successfully rebased and updated refs/heads/main.\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n test_done\n-- \n2.35.0-rc0-182-gd99ee8e0c1\n\n"},{"id":"446196","messageId":"8d19623b-4b11-8ecc-8f27-17ad4ba4167a@gmail.com","threadId":"57182","inReplyTo":"xmqqee5bfaof.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-14T03:14:51Z","receivedAt":"2022-01-14T03:14:55Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2022-01-13 à 19:00, Junio C Hamano a écrit :\n> Junio C Hamano writes:\n> \n>> Here is how I would explain your change.\n> \n> This topic is in \"Expecting an ack or two\" state for some time.\n> \n> - Philippe, are you OK with the attached patch? If so throw your\n> Signed-off-by to this thread.\n\nI'm not 100% OK since as I remarked to John in [1], I don't think all 4\ntests are necessary, 3 out of 4 are duplicates of existing tests, and I\nwould have put the new test in t5520 with 'git pull's other \"autostash\"\ntests. I hoped that John would incorporate my suggestions in a v2, but he\nseems to be busy, so I'm including an updated patch at the end of this email.\nI only slightly edited the commit message you wrote, so thanks for that.\n'pb/pull-rebase-autostash-fix' could be replaced by the patch below,\nI would think.\n\n> - John, if you find Philippe's implementation a good idea (I think\n> it is, as it is simpler and cleaner) after reading and\n> understanding it, please throw an Acked-by or Reviewed-by to this\n> thread.\n> \n\n\nThanks,\n\nPhilippe.\n\n[1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001\nFrom: Philippe Blain \nDate: Thu, 13 Jan 2022 21:58:05 -0500\nSubject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding\n\n\"pull --rebase\" internally uses the merge machinery when the other\nhistory is a descendant of ours (i.e. perform fast-forward). This\ncame from [1], where the discussion was started from a feature\nrequest to do so. It is a bit hard to read the rationale behind it\nin the discussion, but it seems that it was an established fact for\neverybody involved that does not even need to be mentioned that\nfast-forwarding done with \"rebase\" was much undesirable than done\nwith \"merge\", and more importantly, the result left by \"merge\" is as\ngood as (or better than) that by \"rebase\".\n\nExcept for one thing. Because \"git merge\" does not (and should not)\nhonor rebase.autostash, \"git pull\" needs to read it and forward it\nwhen we use \"git merge\" as a (hopefully better) substitute for \"git\nrebase\" during the fast-forwarding. But we forgot to do so (we only\nadd \"--[no-]autostash\" to the \"git merge\" command when \"git pull\" itself\nwas invoked with \"--[no-]autostash\" command line option.\n\nMake sure \"git merge\" is run with \"--autostash\" when\nrebase.autostash is set and used to fast-forward the history on\nbehalf of \"git rebase\". Incidentally this change also takes care of\nthe case where\n\n - \"git pull --rebase\" (without other command line options) is run\n - \"rebase.autostash\" is not set\n - The history fast-forwards\n\nIn such a case, \"git merge\" is run with an explicit \"--no-autostash\"\nto prevent it from honoring merge.autostash configuration, which is\nwhat we want. After all, we want the \"git merge\" to pretend as if\nit is \"git rebase\" while being used for this purpose.\n\n[1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/\n\nReported-by: Tilman Vogel \nCo-authored-by: Junio C Hamano \nSigned-off-by: Philippe Blain \n---\n builtin/pull.c | 7 +++----\n t/t5520-pull.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1cfaf9f343..9f8a8dd716 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\toidclr(&orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = config_autostash;\n-\t\tif (opt_autostash != -1)\n-\t\t\tautostash = opt_autostash;\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n \n \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (!autostash)\n+\t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 93ecfcdd24..3e784f18a6 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' '\n \ttest_cmp_rev HEAD to-rebase-ff\n '\n \n+test_expect_success '--rebase with 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+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Fast-forward\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n test_expect_success '--rebase with conflicts shows advice' '\n \ttest_when_finished \"git rebase --abort; git checkout -f to-rebase\" &&\n \tgit checkout -b seq &&\n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\nprerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816\n-- \n2.29.2\n"},{"id":"446218","messageId":"ACAECB82-3F1C-4731-BF23-A316EFE82FB5@gmail.com","threadId":"57182","inReplyTo":"8d19623b-4b11-8ecc-8f27-17ad4ba4167a@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-14T14:09:03Z","receivedAt":"2022-01-14T14:09:09Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\n> On Jan 13, 2022, at 10:14 PM, Philippe Blain wrote:\n> \n> Hi Junio,\n> \n>> Le 2022-01-13 à 19:00, Junio C Hamano a écrit :\n>> Junio C Hamano writes:\n>>> Here is how I would explain your change.\n>> This topic is in \"Expecting an ack or two\" state for some time.\n>> - Philippe, are you OK with the attached patch? If so throw your\n>> Signed-off-by to this thread.\n> \n> I'm not 100% OK since as I remarked to John in [1], I don't think all 4\n> tests are necessary, 3 out of 4 are duplicates of existing tests, and I\n> would have put the new test in t5520 with 'git pull's other \"autostash\"\n> tests. I hoped that John would incorporate my suggestions in a v2, but he\n> seems to be busy, so I'm including an updated patch at the end of this email.\n> I only slightly edited the commit message you wrote, so thanks for that.\n> 'pb/pull-rebase-autostash-fix' could be replaced by the patch below,\n> I would think.\n> \n>> - John, if you find Philippe's implementation a good idea (I think\n>> it is, as it is simpler and cleaner) after reading and\n>> understanding it, please throw an Acked-by or Reviewed-by to this\n>> thread.\n\nI agree-think Philippe’s implementation is a better solution.\n\nI don’t have the bandwidth these couple of days to reroll the patch so if someone else can take over that would be great!\n\n> \n> \n> Thanks,\n> \n> Philippe.\n> \n> [1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001\n> From: Philippe Blain \n> Date: Thu, 13 Jan 2022 21:58:05 -0500\n> Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding\n> \n> \"pull --rebase\" internally uses the merge machinery when the other\n> history is a descendant of ours (i.e. perform fast-forward). This\n> came from [1], where the discussion was started from a feature\n> request to do so. It is a bit hard to read the rationale behind it\n> in the discussion, but it seems that it was an established fact for\n> everybody involved that does not even need to be mentioned that\n> fast-forwarding done with \"rebase\" was much undesirable than done\n> with \"merge\", and more importantly, the result left by \"merge\" is as\n> good as (or better than) that by \"rebase\".\n> \n> Except for one thing. Because \"git merge\" does not (and should not)\n> honor rebase.autostash, \"git pull\" needs to read it and forward it\n> when we use \"git merge\" as a (hopefully better) substitute for \"git\n> rebase\" during the fast-forwarding. But we forgot to do so (we only\n> add \"--[no-]autostash\" to the \"git merge\" command when \"git pull\" itself\n> was invoked with \"--[no-]autostash\" command line option.\n> \n> Make sure \"git merge\" is run with \"--autostash\" when\n> rebase.autostash is set and used to fast-forward the history on\n> behalf of \"git rebase\". Incidentally this change also takes care of\n> the case where\n> \n> - \"git pull --rebase\" (without other command line options) is run\n> - \"rebase.autostash\" is not set\n> - The history fast-forwards\n> \n> In such a case, \"git merge\" is run with an explicit \"--no-autostash\"\n> to prevent it from honoring merge.autostash configuration, which is\n> what we want. After all, we want the \"git merge\" to pretend as if\n> it is \"git rebase\" while being used for this purpose.\n> \n> [1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/\n> \n> Reported-by: Tilman Vogel \n> Co-authored-by: Junio C Hamano \n> Signed-off-by: Philippe Blain \n> ---\n> builtin/pull.c | 7 +++----\n> t/t5520-pull.sh | 13 +++++++++++++\n> 2 files changed, 16 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 1cfaf9f343..9f8a8dd716 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> oidclr(&orig_head);\n> if (opt_rebase) {\n> - int autostash = config_autostash;\n> - if (opt_autostash != -1)\n> - autostash = opt_autostash;\n> + if (opt_autostash == -1)\n> + opt_autostash = config_autostash;\n> if (is_null_oid(&orig_head) && !is_cache_unborn())\n> die(_(\"Updating an unborn branch with changes added to the index.\"));\n> - if (!autostash)\n> + if (!opt_autostash)\n> require_clean_work_tree(the_repository,\n> N_(\"pull with rebase\"),\n> _(\"please commit or stash them.\"), 1, 0);\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 93ecfcdd24..3e784f18a6 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' '\n> test_cmp_rev HEAD to-rebase-ff\n> '\n> +test_expect_success '--rebase with rebase.autostash succeeds on ff' '\n> + test_when_finished \"rm -fr src dst actual\" &&\n> + git init src &&\n> + test_commit -C src \"initial\" file \"content\" &&\n> + git clone src dst &&\n> + test_commit -C src --printf \"more_content\" file \"more content\\ncontent\\n\" &&\n> + echo \"dirty\" >>dst/file &&\n> + test_config -C dst rebase.autostash true &&\n> + git -C dst pull --rebase >actual 2>&1 &&\n> + grep -q \"Fast-forward\" actual &&\n> + grep -q \"Applied autostash.\" actual\n> +'\n> +\n> test_expect_success '--rebase with conflicts shows advice' '\n> test_when_finished \"git rebase --abort; git checkout -f to-rebase\" &&\n> git checkout -b seq &&\n> \n> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n> prerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816\n> -- \n> 2.29.2\n"},{"id":"446241","messageId":"xmqqr19aayxp.fsf@gitster.g","threadId":"57182","inReplyTo":"8d19623b-4b11-8ecc-8f27-17ad4ba4167a@gmail.com","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-14T19:40:18Z","receivedAt":"2022-01-14T19:40:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> tests. I hoped that John would incorporate my suggestions in a v2, but he\n> seems to be busy, so I'm including an updated patch at the end of this email.\n\nWas about to say \"Will replace the in-tree version with this. Thanks\",\nbefore I realized that your message is \"text/plain; format=flawed\" X-<.\n\nI think I fixed it up correctly. I'll pick this version up from the\nlist and replace what's in-tree with it.\n\nThanks.\n\n----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----\nFrom: Philippe Blain \nDate: Thu, 13 Jan 2022 22:14:51 -0500\nSubject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding\n\n\"pull --rebase\" internally uses the merge machinery when the other\nhistory is a descendant of ours (i.e. perform fast-forward). This\ncame from [1], where the discussion was started from a feature\nrequest to do so. It is a bit hard to read the rationale behind it\nin the discussion, but it seems that it was an established fact for\neverybody involved that does not even need to be mentioned that\nfast-forwarding done with \"rebase\" was much undesirable than done\nwith \"merge\", and more importantly, the result left by \"merge\" is as\ngood as (or better than) that by \"rebase\".\n\nExcept for one thing. Because \"git merge\" does not (and should not)\nhonor rebase.autostash, \"git pull\" needs to read it and forward it\nwhen we use \"git merge\" as a (hopefully better) substitute for \"git\nrebase\" during the fast-forwarding. But we forgot to do so (we only\nadd \"--[no-]autostash\" to the \"git merge\" command when \"git pull\" itself\nwas invoked with \"--[no-]autostash\" command line option.\n\nMake sure \"git merge\" is run with \"--autostash\" when\nrebase.autostash is set and used to fast-forward the history on\nbehalf of \"git rebase\". Incidentally this change also takes care of\nthe case where\n\n - \"git pull --rebase\" (without other command line options) is run\n - \"rebase.autostash\" is not set\n - The history fast-forwards\n\nIn such a case, \"git merge\" is run with an explicit \"--no-autostash\"\nto prevent it from honoring merge.autostash configuration, which is\nwhat we want. After all, we want the \"git merge\" to pretend as if\nit is \"git rebase\" while being used for this purpose.\n\n[1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/\n\nReported-by: Tilman Vogel \nHelped-by: Junio C Hamano \nSigned-off-by: Philippe Blain \n---\n builtin/pull.c | 7 +++----\n t/t5520-pull.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1cfaf9f343..9f8a8dd716 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\toidclr(&orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = config_autostash;\n-\t\tif (opt_autostash != -1)\n-\t\t\tautostash = opt_autostash;\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n \n \t\tif (is_null_oid(&orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (!autostash)\n+\t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n \t\t\t\t_(\"please commit or stash them.\"), 1, 0);\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 93ecfcdd24..081808009b 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' '\n \ttest_cmp_rev HEAD to-rebase-ff\n '\n \n+test_expect_success '--rebase with 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+\ttest_config -C dst rebase.autostash true &&\n+\tgit -C dst pull --rebase >actual 2>&1 &&\n+\tgrep -q \"Fast-forward\" actual &&\n+\tgrep -q \"Applied autostash.\" actual\n+'\n+\n test_expect_success '--rebase with conflicts shows advice' '\n \ttest_when_finished \"git rebase --abort; git checkout -f to-rebase\" &&\n \tgit checkout -b seq &&\n-- \n2.35.0-rc0-182-g7444c100dc\n\n"},{"id":"446265","messageId":"cc3408c6-856a-edd3-9f84-5073ea0fac1a@gmail.com","threadId":"57182","inReplyTo":"xmqqr19aayxp.fsf@gitster.g","subject":"Re: [PATCH 1/1] builtin/pull.c: use config value of autostash","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-01-14T23:33:42Z","receivedAt":"2022-01-14T23:33:46Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2022-01-14 à 14:40, Junio C Hamano a écrit :\n> Philippe Blain writes:\n> \n>> tests. I hoped that John would incorporate my suggestions in a v2, but he\n>> seems to be busy, so I'm including an updated patch at the end of this email.\n> \n> Was about to say \"Will replace the in-tree version with this. Thanks\",\n> before I realized that your message is \"text/plain; format=flawed\" X-<.\n\nI'm sorry for that. I was in a rush and *tought* that I had Thunderbird configured\ncorrectly so that pasting the patch would work out. It seems not. I'll\nbe more careful next time:)\n\n> \n> I think I fixed it up correctly. I'll pick this version up from the\n> list and replace what's in-tree with it.\n> \n> Thanks.\n\nThanks!\n\nPhilippe.\n"}]}