{"thread":{"id":"41554","subject":"[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","startedAt":"2016-02-27T17:41:41Z","lastAt":"2016-03-18T03:53:16Z","messageCount":44,"participants":["Mehul Jain","Junio C Hamano","Matthieu Moy","Paul Tan","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"279651","messageId":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":null,"subject":"[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-02-27T17:41:41Z","receivedAt":"2016-02-27T17:41:41Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"git pull --rebase understands --[no-]autostash flag.\n\nThis flag overrides config variable \"rebase.autoStash\" \nif set (default is false).\n\nWhen calling \"git pull --rebase\" with \"--autostash\",\npull passes the \"--autostash\" option to rebase, \nwhich then runs rebase on a dirty worktree.\n\nWith \"--no-autostash\" option, the command will die\nif the worktree is dirty, before calling rebase.\n\nSigned-off-by: Mehul Jain \n---\n\nThanks to Paul and Matthieu for comments on previous round.\nChanges:\n \t- --autostash flag added\n\t- OPT_COLOR_FLAG replaced by OPT_BOOL\n\t- Default value of opt_rebase changed\n\t- Few changes in code\n\t- Commit message improvements\n\t- Documentation added\n\t- Few tests removed as suggested by Paul\n\t- Added test for --autostash flag\nAll tests passed: https://travis-ci.org/mehul2029/git \n\n builtin/pull.c | 13 ++++++++-----\n t/t5520-pull.sh | 19 +++++++++++++++++++\n t/t5521-pull-options.sh | 16 ++++++++++++++++\n 3 files changed, 43 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..60b320e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -85,6 +85,7 @@ static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n+static int opt_autostash = 0;\n static char *opt_verify_signatures;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -146,6 +147,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_BOOL(0,\"autostash\",&opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n-\n+\tif(opt_autostash)\n+\t\targv_array_push(&args,\"--autostash\");\n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n \n@@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n \n+\tgit_config_get_bool(\"rebase.autostash\",&opt_autostash);\n+\n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n \tparse_repo_refspecs(argc, argv, &repo, &refspecs);\n@@ -835,13 +841,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n-\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (!opt_autostash)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..d80b6cc 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '\n \ttest modified = \"$(git show HEAD:file)\"\n '\n \n+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set false' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy\n+'\n+\n+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '\n \ttest_config rebase.autostash true &&\n \tgit reset --hard before-rebase &&\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 18372ca..3930e45 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '\n \ttest_must_be_empty out)\n '\n \n+test_expect_success 'git pull --rebase --autostash' '\n+\tmkdir clonedrbas &&\n+\t(cd clonedrbas && git init &&\n+\tgit pull --rebase --autostash \"../parent\" >out 2>err &&\n+\ttest -s err &&\n+\ttest_must_be_empty out)\n+'\n+\n+test_expect_success 'git pull --rebase --no-autostash' '\n+\tmkdir clonedrbnas &&\n+\t(cd clonedrbnas && git init &&\n+\tgit pull --rebase --no-autostash \"../parent\" >out 2>err &&\n+\ttest -s err &&\n+\ttest_must_be_empty out)\n+'\n+\n test_expect_success 'git pull -v -q' '\n \tmkdir clonedvq &&\n \t(cd clonedvq && git init &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"279652","messageId":"1456594902-21182-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v2 2/2] Documentation/git-pull.txt: teach 'git pull --rebase' the --[no-]autostash option.","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-02-27T17:41:42Z","receivedAt":"2016-02-27T17:41:42Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"git pull --rebase understands --[no-]autostash flag.\n\nThis flag overrides config variable \"rebase.autoStash\" \nif set (default is false).\n\nWhen calling \"git pull --rebase\" with \"--autostash\",\npull passes the \"--autostash\" option to rebase, \nwhich then runs rebase on a dirty worktree.\n\nWith \"--no-autostash\" option, the command will die\nif the worktree is dirty, before calling rebase.\n\t\nSigned-off-by: Mehul Jain \n---\n Documentation/git-pull.txt | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..ce22c52 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,22 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+\tAutomatically create a temporary stash before the operation\n+\tbegins, and apply it after the operation ends. This means\n+\tthat you can run rebase on a dirty worktree.\n++\n+This option is only valid when '--rebase' option is used.\n++\n+[NOTE]\n+Use with care: the final stash application after a successful\n+rebase might result in non-trivial conflicts.\n+\n+--no-autostash::\n+\tIf the '--autostash' option is enabled by default using the\n+\tconfiguration variable `rebase.autoStash`, this option can be\n+\tused to override this setting.\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"279668","messageId":"xmqqk2lqeyzv.fsf@gitster.mtv.corp.google.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-27T19:26:12Z","receivedAt":"2016-02-27T19:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mehul Jain writes:\n\n> git pull --rebase understands --[no-]autostash flag.\n>\n> This flag overrides config variable \"rebase.autoStash\" \n> if set (default is false).\n\nIs that a statement of a fact? If so, is it true before this patch\nis applied, or after?\n\nEach project has local convention for log messages, and we do too.\nA long log message typically start by explaining how the world is,\nwhy that is not desirable, present a description of a possible world\nthat is better, and then give commands to somebody who is updating\nthe code telling him what to do to make that better world a reality\n(and optionally how).\n\nSo perhaps (I am totally making this up; you need to fact check and\nadjust):\n\n If you enable rebase.autoStash option in your repository, there\n is no way to override it for \"git pull --rebase\" from the\n command line.\n\n Teach \"git pull\" a new \"--[no-]autostash\" option so that a\n rebase.autoStash configuration can be overridden. As \"git\n rebase\" already knows \"--[no-]autostash\" option, it is just the\n matter of passing one when we spawn the command as necessary.\n\nor something. The first one gives the readers how the current world\nworks, and why it is not ideal. The proposed better world in this\ncase is too simple--the first paragraph complained that \"we cannot\ndo X\" and X is something reader would likely to agree is a good\nthing to do, so it can be left unsaid that a better world is one in\nwhich X can be done.\n\n> When calling \"git pull --rebase\" with \"--autostash\",\n> pull passes the \"--autostash\" option to rebase, \n> which then runs rebase on a dirty worktree.\n>\n> With \"--no-autostash\" option, the command will die\n> if the worktree is dirty, before calling rebase.\n\nThese two paragraphs are too obvious and probably are better left\nunsaid. Especially the latter--you are changing \"pull\" and do not\ncontrol what \"rebase\" would do in the future. It could be that a\nbetter rebase in the future may be able to do its job in a dirty\nworktree without doing an autostash.\n\n> Signed-off-by: Mehul Jain \n> ---\n>\n> Thanks to Paul and Matthieu for comments on previous round.\n> Changes:\n> \t- --autostash flag added\n> \t- OPT_COLOR_FLAG replaced by OPT_BOOL\n> \t- Default value of opt_rebase changed\n> \t- Few changes in code\n> \t- Commit message improvements\n> \t- Documentation added\n> \t- Few tests removed as suggested by Paul\n> \t- Added test for --autostash flag\n> All tests passed: https://travis-ci.org/mehul2029/git \n>\n> builtin/pull.c | 13 ++++++++-----\n> t/t5520-pull.sh | 19 +++++++++++++++++++\n> t/t5521-pull-options.sh | 16 ++++++++++++++++\n> 3 files changed, 43 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 10eff03..60b320e 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -85,6 +85,7 @@ static char *opt_squash;\n> static char *opt_commit;\n> static char *opt_edit;\n> static char *opt_ff;\n> +static int opt_autostash = 0;\n\nDo not explicitly initialize a static to 0 (or NULL).\n\n> static char *opt_verify_signatures;\n> static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n> static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n> @@ -146,6 +147,8 @@ static struct option pull_options[] = {\n> \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n> \t\tN_(\"abort if fast-forward is not possible\"),\n> \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> +\tOPT_BOOL(0,\"autostash\",&opt_autostash,\n> +\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n> \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n> \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n> \t\tPARSE_OPT_NOARG),\n> @@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,\n> \targv_array_pushv(&args, opt_strategy_opts.argv);\n> \tif (opt_gpg_sign)\n> \t\targv_array_push(&args, opt_gpg_sign);\n> -\n> +\tif(opt_autostash)\n\nStyle: control keywords are followed by a single SP before the next '('.\n\n> +\t\targv_array_push(&args,\"--autostash\");\n\nStyle: a single SP after a comma.\n\nHow would --no-autostash defeat a configured rebase.autostash with this?\n\nBy the way, how would this affect \"git pull --autostash\" that is run\nwithout \"--rebase\"? If this is an option to \"git pull\", shouldn't\nthe stashing done even when you are not doing a rebase but making a\nmerge?\n\n> \targv_array_push(&args, \"--onto\");\n> \targv_array_push(&args, sha1_to_hex(merge_head));\n> \n> @@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n> \t\tset_reflog_message(argc, argv);\n> \n> +\tgit_config_get_bool(\"rebase.autostash\",&opt_autostash);\n> +\n\nWhy is this change even necessary?\n\n> \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n> \n> \tparse_repo_refspecs(argc, argv, &repo, &refspecs);\n> @@ -835,13 +841,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \t\thashclr(orig_head);\n> \n> \tif (opt_rebase) {\n> -\t\tint autostash = 0;\n> -\n> \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n> \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n> \n> -\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n> -\t\tif (!autostash)\n> +\t\tif (!opt_autostash)\n> \t\t\tdie_on_unclean_work_tree(prefix);\n\nI would have expected that\n\n * a global opt_autostash is initialized to -1 (unspecified);\n\n * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;\n\n * existing \"rebase.autostash\" configuration check inside \"git pull\"\n code gets removed;\n\n * and the code that builds \"git rebase\" invocation command line\n will do\n\n \tif (opt_autostash < 0)\n \t; /* do nothing */\n\telse if (opt_autostash == 0)\n \targv_array_push(&args, \"--no-autostash\");\n\telse\n \targv_array_push(&args, \"--autostash\");\n \t\nThen when \"git pull --rebase\" is run without \"--[no-]autostash\", the\nunderlying \"git rebase\" would be run without that option, and does its\nusual thing, including reading rebase.autostash and deciding to do\n\"git stash\". And when \"git pull\" is run with \"--[no-]autostash\",\nthe underlying \"git rebase\" would be given the same option, and\nwould do what it was told to do, ignoring rebase.autostash setting.\n\nSo why does \"git pull\" still need to look at rebase.autostash\nconfiguration after this change?\n"},{"id":"279702","messageId":"CA+DCAeTJ=cTJzVO4ttgMT-Penh1=eVZtPYDQhcF4a4zueYrCAA@mail.gmail.com","threadId":"41554","inReplyTo":"xmqqk2lqeyzv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-02-28T09:51:55Z","receivedAt":"2016-02-28T09:51:55Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Sun, Feb 28, 2016 at 12:56 AM, Junio C Hamano wrote:\n> Mehul Jain writes:\n>\n>> git pull --rebase understands --[no-]autostash flag.\n>>\n>> This flag overrides config variable \"rebase.autoStash\"\n>> if set (default is false).\n>\n> Is that a statement of a fact? If so, is it true before this patch\n> is applied, or after?\n>\n> Each project has local convention for log messages, and we do too.\n> A long log message typically start by explaining how the world is,\n> why that is not desirable, present a description of a possible world\n> that is better, and then give commands to somebody who is updating\n> the code telling him what to do to make that better world a reality\n> (and optionally how).\n>\n> So perhaps (I am totally making this up; you need to fact check and\n> adjust):\n>\n> If you enable rebase.autoStash option in your repository, there\n> is no way to override it for \"git pull --rebase\" from the\n> command line.\n>\n> Teach \"git pull\" a new \"--[no-]autostash\" option so that a\n> rebase.autoStash configuration can be overridden. As \"git\n> rebase\" already knows \"--[no-]autostash\" option, it is just the\n> matter of passing one when we spawn the command as necessary.\n>\n> or something. The first one gives the readers how the current world\n> works, and why it is not ideal. The proposed better world in this\n> case is too simple--the first paragraph complained that \"we cannot\n> do X\" and X is something reader would likely to agree is a good\n> thing to do, so it can be left unsaid that a better world is one in\n> which X can be done.\n>\n>> When calling \"git pull --rebase\" with \"--autostash\",\n>> pull passes the \"--autostash\" option to rebase,\n>> which then runs rebase on a dirty worktree.\n>>\n>> With \"--no-autostash\" option, the command will die\n>> if the worktree is dirty, before calling rebase.\n>\n> These two paragraphs are too obvious and probably are better left\n> unsaid. Especially the latter--you are changing \"pull\" and do not\n> control what \"rebase\" would do in the future. It could be that a\n> better rebase in the future may be able to do its job in a dirty\n> worktree without doing an autostash.\n\nOK. I will follow up with a better commit message.\n\n>> Signed-off-by: Mehul Jain \n>> ---\n>>\n>> Thanks to Paul and Matthieu for comments on previous round.\n>> Changes:\n>> - --autostash flag added\n>> - OPT_COLOR_FLAG replaced by OPT_BOOL\n>> - Default value of opt_rebase changed\n>> - Few changes in code\n>> - Commit message improvements\n>> - Documentation added\n>> - Few tests removed as suggested by Paul\n>> - Added test for --autostash flag\n>> All tests passed: https://travis-ci.org/mehul2029/git\n>>\n>> builtin/pull.c | 13 ++++++++-----\n>> t/t5520-pull.sh | 19 +++++++++++++++++++\n>> t/t5521-pull-options.sh | 16 ++++++++++++++++\n>> 3 files changed, 43 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/builtin/pull.c b/builtin/pull.c\n>> index 10eff03..60b320e 100644\n>> --- a/builtin/pull.c\n>> +++ b/builtin/pull.c\n>> @@ -85,6 +85,7 @@ static char *opt_squash;\n>> static char *opt_commit;\n>> static char *opt_edit;\n>> static char *opt_ff;\n>> +static int opt_autostash = 0;\n>\n> Do not explicitly initialize a static to 0 (or NULL).\n>\n>> static char *opt_verify_signatures;\n>> static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n>> static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n>> @@ -146,6 +147,8 @@ static struct option pull_options[] = {\n>> OPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n>> N_(\"abort if fast-forward is not possible\"),\n>> PARSE_OPT_NOARG | PARSE_OPT_NONEG),\n>> + OPT_BOOL(0,\"autostash\",&opt_autostash,\n>> + N_(\"automatically stash/stash pop before and after rebase\")),\n>> OPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n>> N_(\"verify that the named commit has a valid GPG signature\"),\n>> PARSE_OPT_NOARG),\n>> @@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,\n>> argv_array_pushv(&args, opt_strategy_opts.argv);\n>> if (opt_gpg_sign)\n>> argv_array_push(&args, opt_gpg_sign);\n>> -\n>> + if(opt_autostash)\n>\n> Style: control keywords are followed by a single SP before the next '('.\n>\n>> + argv_array_push(&args,\"--autostash\");\n>\n> Style: a single SP after a comma.\n>\n> How would --no-autostash defeat a configured rebase.autostash with this?\n>\n> By the way, how would this affect \"git pull --autostash\" that is run\n> without \"--rebase\"? If this is an option to \"git pull\", shouldn't\n> the stashing done even when you are not doing a rebase but making a\n> merge?\n\nAs \"git rebase\" takes \"--[no-]autostash\" as an option whereas \"git\nmerge\" does not,\nhence \"--[no-]autostash\" option is only valid when \"--rebase\" option is used in\n\"git pull\".\n\nIf user uses \"git pull --[no-]autostash\" then two possible things can be done:\n\n * Either \"git pull\" ignores \"--[no-]autostash\" and calls\nunderlying \"git merge\",\n as merge stashes the untracked files by itself. Thus\nignoring --no-autostash\n flag given by user.\n\n * Or \"git pull\" dies with the following error:\n\n \"--[no-]autostash is only valid when --rebase is used.\n Example: git pull --rebase --[no-]autostash\"\n\nI suggest that the latter option should be used in this case as user should know\nthat stashing is performed by \"git merge\" anyway and \"--no-autostash\"\nflag is not\na way to tell \"git merge\" to not to do stashing. Also this error will\nfit perfectly with the\ndocumentation of \"--[no-]autostash\" option given in the [PATCH v2 2/2].\n\nPlease suggest what are your views on this.\n\n>> argv_array_push(&args, \"--onto\");\n>> argv_array_push(&args, sha1_to_hex(merge_head));\n>>\n>> @@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>> if (!getenv(\"GIT_REFLOG_ACTION\"))\n>> set_reflog_message(argc, argv);\n>>\n>> + git_config_get_bool(\"rebase.autostash\",&opt_autostash);\n>> +\n>\n> Why is this change even necessary?\n>\n>> argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n>>\n>> parse_repo_refspecs(argc, argv, &repo, &refspecs);\n>> @@ -835,13 +841,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>> hashclr(orig_head);\n>>\n>> if (opt_rebase) {\n>> - int autostash = 0;\n>> -\n>> if (is_null_sha1(orig_head) && !is_cache_unborn())\n>> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>>\n>> - git_config_get_bool(\"rebase.autostash\", &autostash);\n>> - if (!autostash)\n>> + if (!opt_autostash)\n>> die_on_unclean_work_tree(prefix);\n>\n> I would have expected that\n>\n> * a global opt_autostash is initialized to -1 (unspecified);\n>\n> * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;\n>\n> * existing \"rebase.autostash\" configuration check inside \"git pull\"\n> code gets removed;\n>\n> * and the code that builds \"git rebase\" invocation command line\n> will do\n>\n> if (opt_autostash < 0)\n> ; /* do nothing */\n> else if (opt_autostash == 0)\n> argv_array_push(&args, \"--no-autostash\");\n> else\n> argv_array_push(&args, \"--autostash\");\n>\n> Then when \"git pull --rebase\" is run without \"--[no-]autostash\", the\n> underlying \"git rebase\" would be run without that option, and does its\n> usual thing, including reading rebase.autostash and deciding to do\n> \"git stash\". And when \"git pull\" is run with \"--[no-]autostash\",\n> the underlying \"git rebase\" would be given the same option, and\n> would do what it was told to do, ignoring rebase.autostash setting.\n>\n> So why does \"git pull\" still need to look at rebase.autostash\n> configuration after this change?\n\nI agree with your point that future rebase might be able to do it's job on\ndirty working tree without autostash option, so it's always better to\nlet rebase\ncheck for dirty working tree and do the task of looking at rebase.autostash.\n\nThanks for a thorough review.\n\nCheers,\nMehul Jain\n"},{"id":"279703","messageId":"vpq37sdnnye.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"CA+DCAeTJ=cTJzVO4ttgMT-Penh1=eVZtPYDQhcF4a4zueYrCAA@mail.gmail.com","subject":"Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-02-28T10:12:09Z","receivedAt":"2016-02-28T10:12:09Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> If user uses \"git pull --[no-]autostash\" then two possible things can be done:\n>\n> * Either \"git pull\" ignores \"--[no-]autostash\" and calls\n> underlying \"git merge\",\n> as merge stashes the untracked files by itself. Thus\n> ignoring --no-autostash\n> flag given by user.\n>\n> * Or \"git pull\" dies with the following error:\n>\n> \"--[no-]autostash is only valid when --rebase is used.\n> Example: git pull --rebase --[no-]autostash\"\n>\n> I suggest that the latter option should be used in this case as user should know\n> that stashing is performed by \"git merge\" anyway\n\nNot exactly. \"git merge\" doesn't do a stash, but it accepts to run if\nlocal changes do not touch the same files as the merge. So, --autostash\nis usually not needed for merge.\n\nBut I agree that erroring out is better. Silently accepting the option\nand doing nothing with it would be confusing for users. This would mean\nthat \"git pull --autostash\" would work, but still sometimes show the error:\n\n error: Your local changes to the following files would be overwritten by merge:\n\nThe situation is different for command-line options and for config\nvariables: the config variable wasn't explicitly specified for each run\nof \"git pull\", hence it's acceptable to ignore its value when we have\nnothing to do with it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"279714","messageId":"CACRoPnT6Yc_vfrYvzKctL1dDkZCzme-4emLF0MysJq4f_H2OvQ@mail.gmail.com","threadId":"41554","inReplyTo":"xmqqk2lqeyzv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-02-28T12:28:49Z","receivedAt":"2016-02-28T12:28:49Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi Junio,\n\nOn Sun, Feb 28, 2016 at 3:26 AM, Junio C Hamano wrote:\n> Mehul Jain writes:\n>> @@ -835,13 +841,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>> hashclr(orig_head);\n>>\n>> if (opt_rebase) {\n>> - int autostash = 0;\n>> -\n>> if (is_null_sha1(orig_head) && !is_cache_unborn())\n>> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>>\n>> - git_config_get_bool(\"rebase.autostash\", &autostash);\n>> - if (!autostash)\n>> + if (!opt_autostash)\n>> die_on_unclean_work_tree(prefix);\n>\n> I would have expected that\n>\n> * a global opt_autostash is initialized to -1 (unspecified);\n>\n> * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;\n>\n> * existing \"rebase.autostash\" configuration check inside \"git pull\"\n> code gets removed;\n\nRemoving the \"rebase.autostash\" configuration check would bring back\nthe problem which 53c76dc (pull: allow dirty tree when\nrebase.autostash enabled, 2015-07-04) fixed.\n\n> * and the code that builds \"git rebase\" invocation command line\n> will do\n>\n> if (opt_autostash < 0)\n> ; /* do nothing */\n> else if (opt_autostash == 0)\n> argv_array_push(&args, \"--no-autostash\");\n> else\n> argv_array_push(&args, \"--autostash\");\n>\n> Then when \"git pull --rebase\" is run without \"--[no-]autostash\", the\n> underlying \"git rebase\" would be run without that option, and does its\n> usual thing, including reading rebase.autostash and deciding to do\n> \"git stash\". And when \"git pull\" is run with \"--[no-]autostash\",\n> the underlying \"git rebase\" would be given the same option, and\n> would do what it was told to do, ignoring rebase.autostash setting.\n>\n> So why does \"git pull\" still need to look at rebase.autostash\n> configuration after this change?\n\nUltimately, git-pull needs to be aware of whether autostash is active\nor not (and this means rebase.autostash needs to be looked at as well)\nbecause if autostash is disabled, git-pull needs to perform the\n\"worktree is clean\" check. And this \"worktree is clean\" check needs to\nbe done *before* git-fetch and git-rebase is run.\n\nSee f9189cf (pull --rebase: exit early when the working directory is\ndirty, 2008-05-21).\n\nRegards,\nPaul\n"},{"id":"279728","messageId":"xmqqd1rgeiad.fsf@gitster.mtv.corp.google.com","threadId":"41554","inReplyTo":"CACRoPnT6Yc_vfrYvzKctL1dDkZCzme-4emLF0MysJq4f_H2OvQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] pull --rebase: add --[no-]autostash flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-28T19:39:22Z","receivedAt":"2016-02-28T19:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Ultimately, git-pull needs to be aware of whether autostash is active\n> or not (and this means rebase.autostash needs to be looked at as well)\n> because if autostash is disabled, git-pull needs to perform the\n> \"worktree is clean\" check. And this \"worktree is clean\" check needs to\n> be done *before* git-fetch and git-rebase is run.\n\nAhh, right. I forgot about that discussion.\n"},{"id":"280123","messageId":"1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-03T16:13:19Z","receivedAt":"2016-03-03T16:13:19Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is \nset, there is no way to override it for \n\"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no]autostash\ncommand line flag which overrides the current\nvalue of rebase.autostash, if set. As \"git rebase\"\nunderstands the --[no]autostash option, it's \njust a matter of passing the option to underlying \n\"git rebase\" when \"git pull --rebase\" is called.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nSigned-off-by: Mehul Jain \n---\nSorry for a late follow up. I had my mid-semester\nexamination going on.\n\nPrevious patch: $gname287709\n\nChanges:\n\t* error message is produced when \"git pull\n\t --[no]autostash\" is called.\n \t* opt_autostash intialized with -1.\n\n builtin/pull.c | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..b338b83 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -85,6 +85,7 @@ static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n+static int opt_autostash = -1;\n static char *opt_verify_signatures;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -146,6 +147,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n-\n+\tif (opt_autostash)\n+\t\targv_array_push(&args, \"--autostash\");\n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n \n@@ -835,18 +839,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n-\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (opt_autostash == -1)\n+\t\t\tgit_config_get_bool(\"rebase.autostash\", &opt_autostash);\n+\n+\t\tif (opt_autostash == 0 || opt_autostash == -1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n \t}\n+\telse {\n+\t\t/* If --[no-]autostash option is called without --rebase */\n+\t\tif (opt_autostash == 0)\n+\t\t\tdie(_(\"--no-autostash option is only valid with --rebase.\"));\n+\t\telse if (opt_autostash == 1)\n+\t\t\tdie(_(\"--autostash option is only valid with --rebase.\"));\n+\t}\n \n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280124","messageId":"1457021601-9099-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v3 2/3] test: add test for --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-03T16:13:20Z","receivedAt":"2016-03-03T16:13:20Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Signed-off-by: Mehul Jain \n---\n t/t5520-pull.sh | 19 +++++++++++++++++++\n t/t5521-pull-options.sh | 16 ++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..f5d1d31 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '\n \ttest modified = \"$(git show HEAD:file)\"\n '\n \n+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy\n+'\n+\n+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '\n \ttest_config rebase.autostash true &&\n \tgit reset --hard before-rebase &&\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 18372ca..3930e45 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '\n \ttest_must_be_empty out)\n '\n \n+test_expect_success 'git pull --rebase --autostash' '\n+\tmkdir clonedrbas &&\n+\t(cd clonedrbas && git init &&\n+\tgit pull --rebase --autostash \"../parent\" >out 2>err &&\n+\ttest -s err &&\n+\ttest_must_be_empty out)\n+'\n+\n+test_expect_success 'git pull --rebase --no-autostash' '\n+\tmkdir clonedrbnas &&\n+\t(cd clonedrbnas && git init &&\n+\tgit pull --rebase --no-autostash \"../parent\" >out 2>err &&\n+\ttest -s err &&\n+\ttest_must_be_empty out)\n+'\n+\n test_expect_success 'git pull -v -q' '\n \tmkdir clonedvq &&\n \t(cd clonedvq && git init &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280125","messageId":"1457021601-9099-3-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-03T16:13:21Z","receivedAt":"2016-03-03T16:13:21Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Signed-off-by: Mehul Jain \n---\n Documentation/git-pull.txt | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..a593972 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tAutomatically create a temporary stash before the operation\n+\tbegins, and apply it after the operation ends. This means\n+\tthat you can run rebase on a dirty worktree.\n++\n+This option is only valid when '--rebase' option is used.\n++\n+The default is --no-autostash, unless rebase.autoStash configuration \n+is set.\n++\n+[NOTE]\n+Use with care: the final stash application after a successful\n+rebase might result in non-trivial conflicts.\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280129","messageId":"xmqqbn6v32ms.fsf@gitster.mtv.corp.google.com","threadId":"41554","inReplyTo":"1457021601-9099-3-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-03T17:14:19Z","receivedAt":"2016-03-03T17:14:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mehul Jain writes:\n\n> Signed-off-by: Mehul Jain \n> ---\n> Documentation/git-pull.txt | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index a62a2a6..a593972 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.\n> --no-rebase::\n> \tOverride earlier --rebase.\n> \n> +--autostash::\n> +--no-autostash::\n> +\tAutomatically create a temporary stash before the operation\n> +\tbegins, and apply it after the operation ends. This means\n> +\tthat you can run rebase on a dirty worktree.\n> ++\n> +This option is only valid when '--rebase' option is used.\n> ++\n> +The default is --no-autostash, unless rebase.autoStash configuration \n> +is set.\n> ++\n> +[NOTE]\n> +Use with care: the final stash application after a successful\n> +rebase might result in non-trivial conflicts.\n> +\n\nShould this entry this verbose?\n\n - Is there a non-temporary stash?\n\n - I think \"This means that ...\" is totally unnecessary.\n\n - It probably makes sense to have \"This option is only valid...\" as\n a separate second paragraph as you did.\n\n - \"The default is...\" is misleading. Even if rebase.autostash is\n set to false, we won't autostash, but that is different from the\n default being \"--no-autostash\".\n\n Think of \"--[no-]autostash\" option as *ONE* way to affect the\n auto-stashing behaviour, and treat \"options\" and \"behaviours\" two\n different things.\n\nThere is no default \"option\" for this. It is that \"autostash\"\nbehaviour defaults to what is given to rebase.autostash if\nexists, and can be explicitly set by --[no-]autostash if given.\n\nBut that is the norm for any configuration and option that overrides\nthe configuration, so it probably is a better use of the ink to say\nsomething like this perhaps?\n\n --autostash::\n --no-autostash::\n Before starting \"pull --rebase\", create a stash to save\n local modifications, and apply the stash when done (this\n option is only valid when \"--rebase\" is used).\n +\n '--no-autostash' is useful to override the 'rebase.autoStash'\n configuration variable (see linkgit:git-config[1]).\n\nBy the way, some other patches in this series say --noautostash\nwithout a dash after --no, which you would want to correct.\n\nThanks.\n"},{"id":"280131","messageId":"vpqd1rbiifi.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-03T17:24:01Z","receivedAt":"2016-03-03T17:24:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> If rebase.autoStash configuration variable is \n> set, there is no way to override it for \n> \"git pull --rebase\" from the command line.\n>\n> Teach \"git pull --rebase\" the --[no]autostash\n> command line flag which overrides the current\n> value of rebase.autostash, if set. As \"git rebase\"\n> understands the --[no]autostash option, it's \n> just a matter of passing the option to underlying \n> \"git rebase\" when \"git pull --rebase\" is called.\n\nWe normally wrap text with a bit less than 80 columns. Yours is wrappet\nat 50 columns which makes it look weird.\n\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -85,6 +85,7 @@ static char *opt_squash;\n> static char *opt_commit;\n> static char *opt_edit;\n> static char *opt_ff;\n> +static int opt_autostash = -1;\n\nInstead of going through this 3-valued \"true/false/unset\", I would have\nlet opt_autostash = 0 by default, and read the configuration before the\ncall to parse_options (the usual way to apply precedence: read from low\nprecedence to high precedence).\n\nBut this is a bit less easy than it seems, since the code currently\nchecks the configuration variable only when --rebase is given, so my\nversion would do a useless call to git_config_get_bool() when --rebase\nis not given. So I think your version is OK.\n\n> +\telse {\n> +\t\t/* If --[no-]autostash option is called without --rebase */\n> +\t\tif (opt_autostash == 0)\n> +\t\t\tdie(_(\"--no-autostash option is only valid with --rebase.\"));\n> +\t\telse if (opt_autostash == 1)\n\nThe else is not needed since the other branch dies.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280132","messageId":"vpq1t7rii3u.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"1457021601-9099-2-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v3 2/3] test: add test for --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-03T17:31:01Z","receivedAt":"2016-03-03T17:31:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> Signed-off-by: Mehul Jain \n> ---\n> t/t5520-pull.sh | 19 +++++++++++++++++++\n> t/t5521-pull-options.sh | 16 ++++++++++++++++\n\nThere's no need to split code/test/doc into separate patches, except if\nyour patches are getting really huge. As a reviewer, I like having tests\nand doc in the same patch because they describe the intention of the\nprogrammer.\n\nWe try to have a history where each commit is equally good, and with\nyour version there are two commits where --autostash exists and is\nundocumented (which is not catastrophic, though).\n\n> +test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '\n> +\ttest_config rebase.autostash true &&\n> +\tgit reset --hard before-rebase &&\n> +\techo dirty >new_file &&\n> +\tgit add new_file &&\n> +\ttest_must_fail git pull --rebase --no-autostash . copy\n> +'\n> +\n> +test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '\n> +\ttest_config rebase.autostash false &&\n> +\tgit reset --hard before-rebase &&\n> +\techo dirty >new_file &&\n> +\tgit add new_file &&\n> +\tgit pull --rebase --autostash . copy &&\n> +\ttest_cmp_rev HEAD^ copy &&\n> +\ttest \"$(cat new_file)\" = dirty &&\n> +\ttest \"$(cat file)\" = \"modified again\"\n> +'\n\nSounds good.\n\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '\n> \ttest_must_be_empty out)\n> '\n> \n> +test_expect_success 'git pull --rebase --autostash' '\n> +\tmkdir clonedrbas &&\n> +\t(cd clonedrbas && git init &&\n> +\tgit pull --rebase --autostash \"../parent\" >out 2>err &&\n> +\ttest -s err &&\n> +\ttest_must_be_empty out)\n> +'\n> +\n> +test_expect_success 'git pull --rebase --no-autostash' '\n> +\tmkdir clonedrbnas &&\n> +\t(cd clonedrbnas && git init &&\n> +\tgit pull --rebase --no-autostash \"../parent\" >out 2>err &&\n> +\ttest -s err &&\n> +\ttest_must_be_empty out)\n> +'\n\nNot sure these tests are needed if you have the ones in t/t5520-pull.sh.\nMore tests means more time to run so testing twice the same thing has a\ncost.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280164","messageId":"CAPig+cTC-fkJS5Lskww+qRW5XEqFK4f6fBbY6MBqJWKv5UN_ug@mail.gmail.com","threadId":"41554","inReplyTo":"vpqd1rbiifi.fsf@anie.imag.fr","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-04T01:01:47Z","receivedAt":"2016-03-04T01:01:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 3, 2016 at 12:24 PM, Matthieu Moy\n wrote:\n> Mehul Jain writes:\n>> + else {\n>> + /* If --[no-]autostash option is called without --rebase */\n>> + if (opt_autostash == 0)\n>> + die(_(\"--no-autostash option is only valid with --rebase.\"));\n>> + else if (opt_autostash == 1)\n>\n> The else is not needed since the other branch dies.\n\nA couple other minor comments (to be considered or ignored):\n\nThe comment \"/* If --[no-]autostash ... */\" merely repeats what the\ncode itself already says, thus is not really helpful and can be\ndropped.\n\nIt would be reasonable to combine the two cases into one:\n\n if (opt_autostash != -1)\n die(_(\"--[no]-autostash option is only valid with --rebase.\"));\n"},{"id":"280167","messageId":"CA+DCAeS90-mbyPVqpJJ6LwRwgN2rWycDtuENsCKuu6ktGpDC_A@mail.gmail.com","threadId":"41554","inReplyTo":"xmqqbn6v32ms.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-04T05:04:27Z","receivedAt":"2016-03-04T05:04:27Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano wrote:\n> Should this entry this verbose?\n>\n> - Is there a non-temporary stash?\n>\n> - I think \"This means that ...\" is totally unnecessary.\n>\n> - It probably makes sense to have \"This option is only valid...\" as\n> a separate second paragraph as you did.\n>\n> - \"The default is...\" is misleading. Even if rebase.autostash is\n> set to false, we won't autostash, but that is different from the\n> default being \"--no-autostash\".\n>\n> Think of \"--[no-]autostash\" option as *ONE* way to affect the\n> auto-stashing behaviour, and treat \"options\" and \"behaviours\" two\n> different things.\n>\n> There is no default \"option\" for this. It is that \"autostash\"\n> behaviour defaults to what is given to rebase.autostash if\n> exists, and can be explicitly set by --[no-]autostash if given.\n>\n> But that is the norm for any configuration and option that overrides\n> the configuration, so it probably is a better use of the ink to say\n> something like this perhaps?\n>\n> --autostash::\n> --no-autostash::\n> Before starting \"pull --rebase\", create a stash to save\n> local modifications, and apply the stash when done (this\n> option is only valid when \"--rebase\" is used).\n\nOK, but according to the definition of --[no-]autostash given in\ngit-rebase documentation (https://git-scm.com/docs/git-rebase),\ntemporary stash is created. So shouldn't we be consistent with this\ndefinition by writing \"... , create a temporary stash ...\" instead of\n\"... , create a stash ...\".\n\n> +\n> '--no-autostash' is useful to override the 'rebase.autoStash'\n> configuration variable (see linkgit:git-config[1]).\n\nThanks,\nMehul\n"},{"id":"280168","messageId":"CA+DCAeSRN=nqhnv14kvbnxExiSBPnA3hLP8hY0fbd+YgTR3TNw@mail.gmail.com","threadId":"41554","inReplyTo":"vpqd1rbiifi.fsf@anie.imag.fr","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-04T05:37:40Z","receivedAt":"2016-03-04T05:37:40Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy\n wrote:\n> Mehul Jain writes:\n>\n>> If rebase.autoStash configuration variable is\n>> set, there is no way to override it for\n>> \"git pull --rebase\" from the command line.\n>>\n>> Teach \"git pull --rebase\" the --[no]autostash\n>> command line flag which overrides the current\n>> value of rebase.autostash, if set. As \"git rebase\"\n>> understands the --[no]autostash option, it's\n>> just a matter of passing the option to underlying\n>> \"git rebase\" when \"git pull --rebase\" is called.\n>\n> We normally wrap text with a bit less than 80 columns. Yours is wrappet\n> at 50 columns which makes it look weird.\n\nOK. I will change it.\n\n>> + else {\n>> + /* If --[no-]autostash option is called without --rebase */\n>> + if (opt_autostash == 0)\n>> + die(_(\"--no-autostash option is only valid with --rebase.\"));\n>> + else if (opt_autostash == 1)\n>\n> The else is not needed since the other branch dies.\n\nI'm bit confused here. Which \"else\" you are talking about. I think both the\n\"else\" and \"else if\" are needed here because:\n\n- for the first \"else\", it is necessary that the case is only executed\nwhen --rebase option is not given. If \"else\" is removed then in some case\nwhere user calls \"git pull --rebase --autostash\" will lead to the execution of\n\"else if (opt_autostash == 1)\" case.\n\n- Also removal of \"else if (opt_autostash == 1)\" is not the right thing. As\nthe possibility of opt_autostash = -1 is there and this change may lead to\nthe execution of \"die(_(\"--no-autostash ... \"));\" in case user calls \"git pull\".\n\nThough I agree with Eric on combining the \"if and else if\" cases.\n\nThanks,\nMehul\n"},{"id":"280169","messageId":"CA+DCAeSWhtDuQmYkppz2iR61b6dcaT6SsF-=wcqE1dM9XoTfEw@mail.gmail.com","threadId":"41554","inReplyTo":"vpq1t7rii3u.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/3] test: add test for --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-04T05:43:08Z","receivedAt":"2016-03-04T05:43:08Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Thu, Mar 3, 2016 at 11:01 PM, Matthieu Moy\n wrote:\n> There's no need to split code/test/doc into separate patches, except if\n> your patches are getting really huge. As a reviewer, I like having tests\n> and doc in the same patch because they describe the intention of the\n> programmer.\n>\n> We try to have a history where each commit is equally good, and with\n> your version there are two commits where --autostash exists and is\n> undocumented (which is not catastrophic, though).\n\nAlright, I will make a single patch for the next version.\n\n>> --- a/t/t5521-pull-options.sh\n>> +++ b/t/t5521-pull-options.sh\n>> @@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '\n>> test_must_be_empty out)\n>> '\n>>\n>> +test_expect_success 'git pull --rebase --autostash' '\n>> + mkdir clonedrbas &&\n>> + (cd clonedrbas && git init &&\n>> + git pull --rebase --autostash \"../parent\" >out 2>err &&\n>> + test -s err &&\n>> + test_must_be_empty out)\n>> +'\n>> +\n>> +test_expect_success 'git pull --rebase --no-autostash' '\n>> + mkdir clonedrbnas &&\n>> + (cd clonedrbnas && git init &&\n>> + git pull --rebase --no-autostash \"../parent\" >out 2>err &&\n>> + test -s err &&\n>> + test_must_be_empty out)\n>> +'\n>\n> Not sure these tests are needed if you have the ones in t/t5520-pull.sh.\n> More tests means more time to run so testing twice the same thing has a\n> cost.\n\nI agree with you. This test may not be needed as t/t5520-pull.sh already test\nthis option.\n\nThanks,\nMehul\n"},{"id":"280172","messageId":"vpqvb52g2jk.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"CAPig+cTC-fkJS5Lskww+qRW5XEqFK4f6fBbY6MBqJWKv5UN_ug@mail.gmail.com","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-04T06:50:07Z","receivedAt":"2016-03-04T06:50:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> It would be reasonable to combine the two cases into one:\n>\n> if (opt_autostash != -1)\n> die(_(\"--[no]-autostash option is only valid with --rebase.\"));\n\nNit (in case Mehul copy/paste this): that would be --[no-], not --[no]-.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280173","messageId":"vpqpovag2gz.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"CA+DCAeSRN=nqhnv14kvbnxExiSBPnA3hLP8hY0fbd+YgTR3TNw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-04T06:51:40Z","receivedAt":"2016-03-04T06:51:40Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy\n> wrote:\n>> Mehul Jain writes:\n>>> + else {\n>>> + /* If --[no-]autostash option is called without --rebase */\n>>> + if (opt_autostash == 0)\n>>> + die(_(\"--no-autostash option is only valid with --rebase.\"));\n>>> + else if (opt_autostash == 1)\n>>\n>> The else is not needed since the other branch dies.\n>\n> I'm bit confused here. Which \"else\" you are talking about.\n\nThe last \"else\" keyword. Not the \"else\" branch. It would just work like\nthis, and be a bit more pleasing to my eyes:\n\nif (...)\n\tdie(...);\nif (...)\n\tdie(...);\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280174","messageId":"vpqegbqg22u.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"CA+DCAeS90-mbyPVqpJJ6LwRwgN2rWycDtuENsCKuu6ktGpDC_A@mail.gmail.com","subject":"Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-04T07:00:09Z","receivedAt":"2016-03-04T07:00:09Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano wrote:\n>> Should this entry this verbose?\n>>\n>> - Is there a non-temporary stash?\n>>\n>> - I think \"This means that ...\" is totally unnecessary.\n>>\n>> - It probably makes sense to have \"This option is only valid...\" as\n>> a separate second paragraph as you did.\n>>\n>> - \"The default is...\" is misleading. Even if rebase.autostash is\n>> set to false, we won't autostash, but that is different from the\n>> default being \"--no-autostash\".\n>>\n>> Think of \"--[no-]autostash\" option as *ONE* way to affect the\n>> auto-stashing behaviour, and treat \"options\" and \"behaviours\" two\n>> different things.\n>>\n>> There is no default \"option\" for this. It is that \"autostash\"\n>> behaviour defaults to what is given to rebase.autostash if\n>> exists, and can be explicitly set by --[no-]autostash if given.\n>>\n>> But that is the norm for any configuration and option that overrides\n>> the configuration, so it probably is a better use of the ink to say\n>> something like this perhaps?\n>>\n>> --autostash::\n>> --no-autostash::\n>> Before starting \"pull --rebase\", create a stash to save\n>> local modifications, and apply the stash when done (this\n>> option is only valid when \"--rebase\" is used).\n>\n> OK, but according to the definition of --[no-]autostash given in\n> git-rebase documentation (https://git-scm.com/docs/git-rebase),\n> temporary stash is created.\n\nYou shouldn't justify your change by copy-paste from another place. If\nthe other place is wrong, then replicating it is the worse thing to do\nbecause this would mean we end up with two suboptimal pieces of\ndocumentation instead of one (keep in mind: maintaining stuff is usually\nharder than writing it in the first place).\n\nI agree with Junio that \"temporary stash\" is pleonasm. OTOH, for someone\nnot familiar with \"git stash\", the explanation makes no sense anyway.\nSo, I'd drop the \"temporary\", and instead add a link to the doc of git\nstash. Also, this is not really before starting \"pull --rebase\" but\nafter the user calls \"pull\" and before the actual rebase starst. I'd\nwrite something like this:\n\n\tBefore starting rebase, stash local modifications away (see\n\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n\tdone\n\n(I added \"if needed\" which makes sense technically since we don't create\nan empty stash if not needed)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280197","messageId":"CACRoPnTqX+Qswfxmj07LRFThV1gjAvZ7c=sr1XQjxc1KHgvVPg@mail.gmail.com","threadId":"41554","inReplyTo":"1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-03-04T15:52:43Z","receivedAt":"2016-03-04T15:52:43Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain wrote:\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 10eff03..b338b83 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -85,6 +85,7 @@ static char *opt_squash;\n> static char *opt_commit;\n> static char *opt_edit;\n> static char *opt_ff;\n> +static int opt_autostash = -1;\n> static char *opt_verify_signatures;\n> static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n> static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n> @@ -146,6 +147,8 @@ static struct option pull_options[] = {\n> OPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n> N_(\"abort if fast-forward is not possible\"),\n> PARSE_OPT_NOARG | PARSE_OPT_NONEG),\n> + OPT_BOOL(0, \"autostash\", &opt_autostash,\n> + N_(\"automatically stash/stash pop before and after rebase\")),\n> OPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n> N_(\"verify that the named commit has a valid GPG signature\"),\n> PARSE_OPT_NOARG),\n> @@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,\n> argv_array_pushv(&args, opt_strategy_opts.argv);\n> if (opt_gpg_sign)\n> argv_array_push(&args, opt_gpg_sign);\n> -\n\nMinor nit: but when I wrote the code for run_rebase() I separated the\noptions for \"Shared options\" and \"Options passed to git-rebase\" into\ndifferent code block groups from the other code, and I would like it\nif it remained that way :-(\n\n> + if (opt_autostash)\n> + argv_array_push(&args, \"--autostash\");\n\nHmm, interesting. If rebase.autostash=true !opt_autostash, we don't\nneed to pass --no-autostash to git-rebase because it will only stash\nif the worktree is dirty, but a dirty worktree will be caught by\ngit-pull's die_on_unclean_worktree() anyway.\n\nStill, it may be a problem because the worktree may become dirty\nin-between our \"worktree is clean\" check and when git-rebase is run\n(during the git-fetch), and the user may be surprised if git-rebase\nattempted to stash in that case.\n\nSo we may wish to pass \"--no-autostash\" to git-rebase as well.\n\n> argv_array_push(&args, \"--onto\");\n> argv_array_push(&args, sha1_to_hex(merge_head));\n>\n> @@ -835,18 +839,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> hashclr(orig_head);\n>\n> if (opt_rebase) {\n> - int autostash = 0;\n> -\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>\n> - git_config_get_bool(\"rebase.autostash\", &autostash);\n> - if (!autostash)\n> + if (opt_autostash == -1)\n> + git_config_get_bool(\"rebase.autostash\", &opt_autostash);\n> +\n> + if (opt_autostash == 0 || opt_autostash == -1)\n> die_on_unclean_work_tree(prefix);\n>\n> if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n> hashclr(rebase_fork_point);\n> }\n> + else {\n\nGit code style puts the else on the same line, not on a new one.\n\n> + /* If --[no-]autostash option is called without --rebase */\n\nYeah, I agree with Eric that this comment should be dropped,\n\n> + if (opt_autostash == 0)\n> + die(_(\"--no-autostash option is only valid with --rebase.\"));\n> + else if (opt_autostash == 1)\n> + die(_(\"--autostash option is only valid with --rebase.\"));\n> + }\n\nand these error messages combined.\n\n>\n> if (run_fetch(repo, refspecs))\n> return 1;\n> --\n> 2.7.1.340.g69eb491.dirty\n\nRegards,\nPaul\n"},{"id":"280198","messageId":"CACRoPnT1BsddXC9oqODkxHb==coKU6EKQiijk9=T0HYTFeCZVQ@mail.gmail.com","threadId":"41554","inReplyTo":"1457021601-9099-3-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-03-04T15:56:21Z","receivedAt":"2016-03-04T15:56:21Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain wrote:\n> Signed-off-by: Mehul Jain \n> ---\n> Documentation/git-pull.txt | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index a62a2a6..a593972 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.\n> --no-rebase::\n> Override earlier --rebase.\n>\n> +--autostash::\n> +--no-autostash::\n> + Automatically create a temporary stash before the operation\n> + begins, and apply it after the operation ends. This means\n> + that you can run rebase on a dirty worktree.\n> ++\n> +This option is only valid when '--rebase' option is used.\n> ++\n> +The default is --no-autostash, unless rebase.autoStash configuration\n\nBy the way, there is a trailing space on this line.\n\n> +is set.\n> ++\n> +[NOTE]\n> +Use with care: the final stash application after a successful\n> +rebase might result in non-trivial conflicts.\n> +\n> Options related to fetching\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>\n> --\n> 2.7.1.340.g69eb491.dirty\n\nRegards,\nPaul\n"},{"id":"280243","messageId":"1457171545-14496-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v4] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-05T09:52:25Z","receivedAt":"2016-03-05T09:52:25Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is set, there is no way to\noverride it for \"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no-]autostash command line flag which\noverrides the current value of rebase.autoStash, if set. As \"git rebase\"\nunderstands the --[no-]autostash option, it's just a matter of passing\nthe option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\n\nPrevious patches: $gname287709\n\nChanges:\n\t* --no-autostash is passed to git-rebase (suggested by Paul)\n\t\n\t* Error message changed when \"git pull --[no-]autostash\" is called.\n\n\t* If rebase.autoStash is unset and user don't pass --[no-]autostash\n\t flag then nothing is passed to git-rebase (i.e. if rebase.autoStash = -1),\n\t as it should be left to git-rebase to decide what to do.\n\n Documentation/git-pull.txt | 15 +++++++++++++++\n builtin/pull.c | 18 ++++++++++++++----\n t/t5520-pull.sh | 19 +++++++++++++++++++\n 3 files changed, 48 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..06e5ddd 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tBefore starting rebase, stash local modifications away (see\n+\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n+\tdone.\n++\n+This option is only valid when '--rebase' is used.\n++\n+'--no-autostash' is useful to override the 'rebase.autoStash'\n+configuration variable (see linkgit:git-config[1]).\n++\n+[NOTE]\n+Use with care: the final stash application after a successful\n+rebase might result in non-trivial conflicts.\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..e3f5fbf 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -85,6 +85,7 @@ static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n+static int opt_autostash = -1;\n static char *opt_verify_signatures;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -146,6 +147,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -789,6 +792,10 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n+\tif (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n+\telse if (opt_autostash == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -835,17 +842,20 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n-\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (opt_autostash == -1)\n+\t\t\tgit_config_get_bool(\"rebase.autostash\", &opt_autostash);\n+\n+\t\tif (opt_autostash == 0 || opt_autostash == -1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n+\t} else {\n+\t\tif (opt_autostash != -1)\n+\t\t\tdie(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \t}\n \n \tif (run_fetch(repo, refspecs))\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..f5d1d31 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '\n \ttest modified = \"$(git show HEAD:file)\"\n '\n \n+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy\n+'\n+\n+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '\n \ttest_config rebase.autostash true &&\n \tgit reset --hard before-rebase &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280248","messageId":"CA+DCAeTsh8Ff8ifiE+e4_ng0tDB7z2uvkT2Nf5OhtZOBaoj9Kg@mail.gmail.com","threadId":"41554","inReplyTo":"1457171545-14496-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v4] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-05T12:26:13Z","receivedAt":"2016-03-05T12:26:13Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Sat, Mar 5, 2016 at 3:22 PM, Mehul Jain wrote:\n\n> Changes:\n> * --no-autostash is passed to git-rebase (suggested by Paul)\n>\n> * Error message changed when \"git pull --[no-]autostash\" is called.\n>\n> * If rebase.autoStash is unset and user don't pass --[no-]autostash\n> flag then nothing is passed to git-rebase (i.e. if rebase.autoStash = -1),\n> as it should be left to git-rebase to decide what to do.\n\nI'm sorry. Here instead of \" ... (i.e. if rebase.autoStash = -1), ...\n\" it should be \" ... (i.e. if opt_autostash = -1), ... \" .\n\nThanks,\nMehul\n"},{"id":"280250","messageId":"xmqqfuw4x3e8.fsf@gitster.mtv.corp.google.com","threadId":"41554","inReplyTo":"1457171545-14496-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v4] pull --rebase: add --[no-]autostash flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-05T17:04:15Z","receivedAt":"2016-03-05T17:04:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mehul Jain writes:\n\n> +--autostash::\n> +--no-autostash::\n> +\tBefore starting rebase, stash local modifications away (see\n> +\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n> +\tdone.\n> ++\n> +This option is only valid when '--rebase' is used.\n> ++\n> +'--no-autostash' is useful to override the 'rebase.autoStash'\n> +configuration variable (see linkgit:git-config[1]).\n> ++\n> +[NOTE]\n> +Use with care: the final stash application after a successful\n> +rebase might result in non-trivial conflicts.\n> +\n\nShould this entry be this verbose and have four separate paragraphs?\n\nI think \"This option is...\" can and should be a part of the first\nparagraph, and *NOTE* paragraph does not have to be there. The\nreaders have already been referred to rebase.autoStash that warns\nthem to be prepared to deal with possible conflicts when replaying\nthe stashed change.\n\n> -\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n> -\t\tif (!autostash)\n> +\t\tif (opt_autostash == -1)\n> +\t\t\tgit_config_get_bool(\"rebase.autostash\", &opt_autostash);\n> +\n> +\t\tif (opt_autostash == 0 || opt_autostash == -1)\n> \t\t\tdie_on_unclean_work_tree(prefix);\n\nIf the command line didn't give --[no-]autostash when the control\nreaches this section of the code, get_bool() is used to see if\nrebase.autostash is set. If it is explicitly set to false or unset,\nthe command will check if the work tree is clean and complain.\n\nMakes sense.\n\nThis does not have to be in the scope of your change, by the way,\nbut it may make sense to reduce the use of git_config_get_*()\nfunction when the program already contains a call to git_config().\n\nInstead of calling git_default_config callback from there, introduce\na new callback function and read variables that matter from there,\ni.e.\n\nstatic int opt_autostash = -1;\nstatic int config_autostash = -1;\n...\nstatic int git_pull_config(const char *var, const char *value, void *cb)\n{\n\tif (!strcmp(var, \"rebase.autostash\")) {\n \tconfig_autostash = git_config_bool(var, value);\n\t\treturn 0;\n\t}\n return git_default_config(var, value, cb);\n}\n\nand then you do \"the command line overrides the configured value\"\nwith\n\n\tif (opt_autostash < 0)\n \topt_autostash = config_autostash;\n\tif (opt_autostash < 0)\n \tdie_on_unclean_work_tree(...);\n\nEither way you have to read the config files in full at least once\nanyway, but the program does not have to ask the config API to look\nup the value with a separate git_config_get_bool() call if you did\nso.\n\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index c952d5e..f5d1d31 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '\n> \ttest modified = \"$(git show HEAD:file)\"\n> '\n> \n> +test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '\n\nThis is overly verbose, isn't it?\n\nYou are verifying that \"--no-autostash overrides rebase.autostash\"\nin this test, so perhaps make it:\n\n test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash'\n\nor something?\n\n> +\ttest_config rebase.autostash true &&\n> +\tgit reset --hard before-rebase &&\n> +\techo dirty >new_file &&\n> +\tgit add new_file &&\n> +\ttest_must_fail git pull --rebase --no-autostash . copy\n> +'\n> +\n> +test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '\n\nLikewise. This verifies that \"--autostash overrides rebase.autostash\".\n\n> +\ttest_config rebase.autostash false &&\n> +\tgit reset --hard before-rebase &&\n> +\techo dirty >new_file &&\n> +\tgit add new_file &&\n> +\tgit pull --rebase --autostash . copy &&\n> +\ttest_cmp_rev HEAD^ copy &&\n> +\ttest \"$(cat new_file)\" = dirty &&\n> +\ttest \"$(cat file)\" = \"modified again\"\n> +'\n> +\n\nFor completeness, there are two other cases, i.e. rebase.autostash\nset to false and --no-autostash given from the command line, and the\nvariable set to true and --autostash from the command line.\n\nIt is worth considering if these two need to be tested. There are\n(at least) two things to take into account:\n\n - We know, after seeing the current implementation, that these two\n cases do not have to be tested, because the configuration is not\n even looked at when there is a command line option.\n\n - However, the real reason why we prepare these tests is not to\n demonstrate that the patch that introduces a new feature works as\n expected. It is to prevent _future_ changes by other people, who\n may not be aware of the reason why we chose not to test these two\n cases, from breaking the code. If a careless refactoring of the\n code can break our assumption (i.e. \"the configuration is not\n even looked at when there is a command line option.\" above), such\n a change may introduce a bug that disables autostash when both\n variable and option tell the code to autostash, and having the\n two additional tests would catch such a breakage.\n\n> test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '\n> \ttest_config rebase.autostash true &&\n> \tgit reset --hard before-rebase &&\n\nThese two new tests are inserted in a somewhat strange location, by\nthe way. This existing test checks the basic working of rebase.autostash\nwithout any command line override. Shouldn't the more complex new cases\nthat deal with command line override come after this one?\n\nIs it worth checking the case where autostash kicks in, rebase\nitself is completed successfully, but the final \"stash pop\" fails in\nconflict? I am thinking aloud and just wondering, not suggesting\nto add such a test, or even suggesting that having such a test is\nworthwhile. I didn't even check if there is already an existing test\nto cover that case.\n\nOther than the above, looking better. Thanks.\n"},{"id":"280318","messageId":"CA+DCAeSD60jChusPviF-VNENVEfO0SoFkTZE-B-q+X_YcnuvFw@mail.gmail.com","threadId":"41554","inReplyTo":"xmqqfuw4x3e8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-07T08:23:46Z","receivedAt":"2016-03-07T08:23:46Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Hi Junio,\n\nThanks for the thorough review.\n\nOn Sat, Mar 5, 2016 at 10:34 PM, Junio C Hamano wrote:\n> Is it worth checking the case where autostash kicks in, rebase\n> itself is completed successfully, but the final \"stash pop\" fails in\n> conflict? I am thinking aloud and just wondering, not suggesting\n> to add such a test, or even suggesting that having such a test is\n> worthwhile. I didn't even check if there is already an existing test\n> to cover that case.\n\nI think this case has already been checked in t3420-rebase.autostash.sh:\n\"rebase$type: non-conflicting rebase, conflicting stash\". Though in that\ntest rebase.autoStash has been used to trigger git-stash.\n\nThanks,\nMehul\n"},{"id":"280324","messageId":"1457354245-26871-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v5] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-07T12:37:25Z","receivedAt":"2016-03-07T12:37:25Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is set, there is no way to\noverride it for \"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no-]autostash command line flag which\noverrides the current value of rebase.autoStash, if set. As \"git rebase\"\nunderstands the --[no-]autostash option, it's just a matter of passing\nthe option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nAlso introduce a callback function to read config variables.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\nPrevious patches: $gname287709\n\nChanges: \n* Documentation modified\n* Introduced a callback function to read config variables.\n* Introduce two new tests\n* Modified previous tests because:\n\t\n\tif worktree is dirty and --no-autostash is passed then git-pull\n\tshould die before git-fetch is called, hence the error message \n\tmust be: \n\t\n\t\"Cannot pull with rebase: Your index contains uncommitted changes.\"\n\n\tWhereas if git-fetch is invoked and --no-autostash is passed to\n\tgit-rebase then error message is:\n\t\n\t\"Cannot rebase: Your index contains uncommitted changes.\"\n\n\tThis test will prevent_future_changes by other people to break the\n\tcode which may cause git-fetch being called even if worktree is dirty\n\twith --no-autostash option.\n\n Documentation/git-pull.txt | 9 +++++++++\n builtin/pull.c | 34 ++++++++++++++++++++++++++++------\n t/t5520-pull.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 77 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..393ac7d 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tBefore starting rebase, stash local modifications away (see\n+\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n+\tdone (this option is only valid when '--rebase' is used).\n++\n+'--no-autostash' is useful to override the 'rebase.autoStash'\n+configuration variable (see linkgit:git-config[1]).\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..b25c53d 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -85,6 +85,8 @@ static char *opt_squash;\n static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n+static int opt_autostash = -1;\n+static int config_autostash = -1;\n static char *opt_verify_signatures;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -146,6 +148,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n \t\tN_(\"abort if fast-forward is not possible\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n@@ -306,6 +310,18 @@ static enum rebase_type config_get_rebase(void)\n }\n \n /**\n+ * Read config variables.\n+ */\n+static int git_pull_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var,\"rebase.autostash\")) {\n+\t\tconfig_autostash = git_config_bool(var,value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+/**\n * Returns 1 if there are unstaged changes, 0 otherwise.\n */\n static int has_unstaged_changes(const char *prefix)\n@@ -789,6 +805,10 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n+\tif (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n+\telse if (opt_autostash == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -823,7 +843,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase();\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_pull_config, NULL);\n \n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"Pull\");\n@@ -835,18 +855,20 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n-\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n+\n+\t\tif (opt_autostash == 0 || opt_autostash == -1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n-\t}\n+\t} else\n+\t\tif (opt_autostash != -1)\n+\t\t\tdie(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..cc6a481 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -256,6 +256,46 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n \ttest \"$(cat file)\" = \"modified again\"\n '\n \n+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n \ttest_config pull.rebase true &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280340","messageId":"xmqqy49tnb8x.fsf@gitster.mtv.corp.google.com","threadId":"41554","inReplyTo":"1457354245-26871-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v5] pull --rebase: add --[no-]autostash flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-07T23:01:34Z","receivedAt":"2016-03-07T23:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mehul Jain writes:\n\n> If rebase.autoStash configuration variable is set, there is no way to\n> override it for \"git pull --rebase\" from the command line.\n>\n> Teach \"git pull --rebase\" the --[no-]autostash command line flag which\n> overrides the current value of rebase.autoStash, if set. As \"git rebase\"\n> understands the --[no-]autostash option, it's just a matter of passing\n> the option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n>\n> Also introduce a callback function to read config variables.\n\nI was expecting you not to do this (it does not have to be in the\nscope of this topic), but if you do so, it would be preferrable to\ndo it as a preparatory clean-up patch--the existing code already\nmakes a separate get_bool() call to read a single config while it\nuses git_config(git_default_config) to read the other variables, and\nintroduction of git_pull_config() is a clean-up that is independent\nof your new feature.\n\nAnd then on top of that cleaned-up codebase, add this new feature.\n\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index a62a2a6..393ac7d 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n> --no-rebase::\n> \tOverride earlier --rebase.\n> \n> +--autostash::\n> +--no-autostash::\n> +\tBefore starting rebase, stash local modifications away (see\n> +\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n> +\tdone (this option is only valid when '--rebase' is used).\n> ++\n> +'--no-autostash' is useful to override the 'rebase.autoStash'\n> +configuration variable (see linkgit:git-config[1]).\n\nDon't we use `typewriter font` when referring to literal command,\noption and variable names in the documentation? The description\nfor `--rebase` that comes immediately before this hunk seems to\nsupport that observation.\n\n> @@ -306,6 +310,18 @@ static enum rebase_type config_get_rebase(void)\n> }\n> \n> /**\n> + * Read config variables.\n> + */\n> +static int git_pull_config(const char *var, const char *value, void *cb)\n> +{\n> +\tif (!strcmp(var,\"rebase.autostash\")) {\n\nStyle: SP after a comma. There may be the same problem elsewhere on\nthe new lines, but I won't point out all of them.\n\nOther than that, looking better.\n\nThanks.\n"},{"id":"280379","messageId":"1457461148-8109-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v6 1/2] git-pull.c: introduce git_pull_config()","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-08T18:19:07Z","receivedAt":"2016-03-08T18:19:07Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"git-pull makes a seperate call to git_config_get_bool() to read the value\nof \"rebase.autostash\", this can be reduced as a call to git_config() is\nalready there in the code.\n\nIntroduce a callback function git_pull_config() to read \"rebase.autostash\"\nalong with other variables.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Mehul Jain \n---\nprevious patches: $gname287709\n\nThis is a clean-up patch to add --[no-]autostash option to \n\"git pull --rebase\".\n \n builtin/pull.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..8a318e9 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int config_autostash = -1;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n static char *opt_gpg_sign;\n@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)\n \n \treturn REBASE_FALSE;\n }\n+/**\n+ * Read config variables.\n+ */\n+static int git_pull_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"rebase.autostash\")) {\n+\t\tconfig_autostash = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n \n /**\n * Returns 1 if there are unstaged changes, 0 otherwise.\n@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase();\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_pull_config, NULL);\n \n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"Pull\");\n@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n \n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (config_autostash != 1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280380","messageId":"1457461148-8109-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1457461148-8109-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v6 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-08T18:19:08Z","receivedAt":"2016-03-08T18:19:08Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is set, there is no way to\noverride it for \"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no-]autostash command line flag which\noverrides the current value of rebase.autoStash, if set. As \"git rebase\"\nunderstands the --[no-]autostash option, it's just a matter of passing\nthe option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\nPrevious patches: $gname287709\n\nChanges:\n\t- Slight change is documentation.\n\n Documentation/git-pull.txt | 9 +++++++++\n builtin/pull.c | 16 ++++++++++++++--\n t/t5520-pull.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..da89be6 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tBefore starting rebase, stash local modifications away (see\n+\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n+\tdone (this option is only valid when \"--rebase\" is used).\n++\n+'--no-autostash' is useful to override the 'rebase.autoStash'\n+configuration variable (see linkgit:git-config[1]).\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 8a318e9..a01058a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int opt_autostash = -1;\n static int config_autostash = -1;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -150,6 +151,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU_ARGV('s', \"strategy\", &opt_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"),\n \t\t0),\n@@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n+\tif (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n+\telse if (opt_autostash == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (config_autostash != 1)\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n+\n+\t\tif (opt_autostash != 1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n-\t}\n+\t} else\n+\t\tif (opt_autostash != -1)\n+\t\t\t die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..85d9bea 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n \ttest \"$(cat new_file)\" = dirty &&\n \ttest \"$(cat file)\" = \"modified again\"\n '\n+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n \n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280407","messageId":"1457497100-13045-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v7 1/2] git-pull.c: introduce git_pull_config()","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-09T04:18:19Z","receivedAt":"2016-03-09T04:18:19Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"git-pull makes a seperate call to git_config_get_bool() to read the value\nof \"rebase.autostash\", this can be reduced as a call to git_config() is\nalready there in the code.\n\nIntroduce a callback function git_pull_config() to read \"rebase.autostash\"\nalong with other variables.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Mehul Jain \n---\nprevious patches: $gname287709\n\nThis is a clean-up patch to add --[no-]autostash option to \n\"git pull --rebase\".\n \n builtin/pull.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..8a318e9 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int config_autostash = -1;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n static char *opt_gpg_sign;\n@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)\n \n \treturn REBASE_FALSE;\n }\n+/**\n+ * Read config variables.\n+ */\n+static int git_pull_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"rebase.autostash\")) {\n+\t\tconfig_autostash = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n \n /**\n * Returns 1 if there are unstaged changes, 0 otherwise.\n@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase();\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_pull_config, NULL);\n \n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"Pull\");\n@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n \n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (config_autostash != 1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280408","messageId":"1457497100-13045-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1457497100-13045-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-09T04:18:20Z","receivedAt":"2016-03-09T04:18:20Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is set, there is no way to\noverride it for \"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no-]autostash command line flag which\noverrides the current value of rebase.autoStash, if set. As \"git rebase\"\nunderstands the --[no-]autostash option, it's just a matter of passing\nthe option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\nPrevious patches: $gname287709\n\nChanges:\n\t- Slight change is documentation.\n\n Documentation/git-pull.txt | 9 +++++++++\n builtin/pull.c | 16 ++++++++++++++--\n t/t5520-pull.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..da89be6 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tBefore starting rebase, stash local modifications away (see\n+\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n+\tdone (this option is only valid when \"--rebase\" is used).\n++\n+`--no-autostash` is useful to override the `rebase.autoStash`\n+configuration variable (see linkgit:git-config[1]).\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 8a318e9..a01058a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int opt_autostash = -1;\n static int config_autostash = -1;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -150,6 +151,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU_ARGV('s', \"strategy\", &opt_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"),\n \t\t0),\n@@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n+\tif (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n+\telse if (opt_autostash == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (config_autostash != 1)\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n+\n+\t\tif (opt_autostash != 1)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n-\t}\n+\t} else\n+\t\tif (opt_autostash != -1)\n+\t\t\t die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..85d9bea 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n \ttest \"$(cat new_file)\" = dirty &&\n \ttest \"$(cat file)\" = \"modified again\"\n '\n+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n \n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280605","messageId":"CACRoPnRESpiNL_=rEgw7kRdKjek8Z=qVjPH=BEWsWeYn22YryQ@mail.gmail.com","threadId":"41554","inReplyTo":"1457497100-13045-2-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2016-03-11T04:51:51Z","receivedAt":"2016-03-11T04:51:51Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Mar 9, 2016 at 12:18 PM, Mehul Jain wrote:\n> If rebase.autoStash configuration variable is set, there is no way to\n> override it for \"git pull --rebase\" from the command line.\n>\n> Teach \"git pull --rebase\" the --[no-]autostash command line flag which\n> overrides the current value of rebase.autoStash, if set. As \"git rebase\"\n> understands the --[no-]autostash option, it's just a matter of passing\n> the option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n>\n> Helped-by: Matthieu Moy \n> Helped-by: Junio C Hamano \n> Helped-by: Paul Tan \n> Helped-by: Eric Sunshine \n> Signed-off-by: Mehul Jain \n> ---\n> Previous patches: $gname287709\n>\n> Changes:\n> - Slight change is documentation.\n>\n> Documentation/git-pull.txt | 9 +++++++++\n> builtin/pull.c | 16 ++++++++++++++--\n> t/t5520-pull.sh | 39 +++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 62 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index a62a2a6..da89be6 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n> --no-rebase::\n> Override earlier --rebase.\n>\n> +--autostash::\n> +--no-autostash::\n> + Before starting rebase, stash local modifications away (see\n> + linkgit:git-stash.txt[1]) if needed, and apply the stash when\n> + done (this option is only valid when \"--rebase\" is used).\n> ++\n> +`--no-autostash` is useful to override the `rebase.autoStash`\n> +configuration variable (see linkgit:git-config[1]).\n> +\n> Options related to fetching\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 8a318e9..a01058a 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -86,6 +86,7 @@ static char *opt_commit;\n> static char *opt_edit;\n> static char *opt_ff;\n> static char *opt_verify_signatures;\n> +static int opt_autostash = -1;\n> static int config_autostash = -1;\n\nHmm, why can't config_autostash just default to 0?\n\n> static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n> static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n> @@ -150,6 +151,8 @@ static struct option pull_options[] = {\n> OPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n> N_(\"verify that the named commit has a valid GPG signature\"),\n> PARSE_OPT_NOARG),\n> + OPT_BOOL(0, \"autostash\", &opt_autostash,\n> + N_(\"automatically stash/stash pop before and after rebase\")),\n> OPT_PASSTHRU_ARGV('s', \"strategy\", &opt_strategies, N_(\"strategy\"),\n> N_(\"merge strategy to use\"),\n> 0),\n> @@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,\n> argv_array_pushv(&args, opt_strategy_opts.argv);\n> if (opt_gpg_sign)\n> argv_array_push(&args, opt_gpg_sign);\n> + if (opt_autostash == 1)\n> + argv_array_push(&args, \"--autostash\");\n> + else if (opt_autostash == 0)\n> + argv_array_push(&args, \"--no-autostash\");\n\nThe precise testing for specific values of -1, 0 and 1 throughout the\ncode makes me uncomfortable. Ordinarily, I would expect a simple\n\n argv_array_push(&args, opt_autostash ? \"--autostash\" : \"--no-autostash\");\n\nStepping back a bit, the only reason why we introduced opt_autostash =\n-1 as a possible value is because we need to detect if\n--[no-]autostash is being used with git-merge. I consider that a\nkludge, because if git-merge supports --autostash as well (possibly in\nthe future), then we will not need this -1 value.\n\nSo, from that point of view, a -1 value is okay as a workaround, but\nkludges, and hence the -1 value, should be gotten rid off as soon as\npossible.\n\n>\n> argv_array_push(&args, \"--onto\");\n> argv_array_push(&args, sha1_to_hex(merge_head));\n> @@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>\n> - if (config_autostash != 1)\n> + if (opt_autostash == -1)\n> + opt_autostash = config_autostash;\n\nSo, if config_autostash defaults to zero, we can be certain that\nopt_autostash will be either true or false.\n\n> +\n> + if (opt_autostash != 1)\n\nAnd then we can do if (!opt_autostash) here, instead of making readers\nwonder why we are testing for the value \"1\" specifically.\n\n> die_on_unclean_work_tree(prefix);\n>\n> if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n> hashclr(rebase_fork_point);\n> - }\n> + } else\n> + if (opt_autostash != -1)\n> + die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n>\n> if (run_fetch(repo, refspecs))\n> return 1;\n\nThanks,\nPaul\n"},{"id":"280611","messageId":"CA+DCAeSvjyy6WO=n-wdo3_yHGSWmUC0Epbt-OYLF_fbuczeeKA@mail.gmail.com","threadId":"41554","inReplyTo":"CACRoPnRESpiNL_=rEgw7kRdKjek8Z=qVjPH=BEWsWeYn22YryQ@mail.gmail.com","subject":"Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-11T13:15:01Z","receivedAt":"2016-03-11T13:15:01Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan wrote:\n>> static int config_autostash = -1;\n>\n> Hmm, why can't config_autostash just default to 0?\n\nPreviously Junio recommended not to explicitly initialize a\nstatic to 0 (or NULL).\nhttp://thread.gmane.org/gmane.comp.version-control.git/287709/focus=287726\n\nDefaulting config_autostash = 0 will work too as you explained.\n\n>> + if (opt_autostash == 1)\n>> + argv_array_push(&args, \"--autostash\");\n>> + else if (opt_autostash == 0)\n>> + argv_array_push(&args, \"--no-autostash\");\n>\n> The precise testing for specific values of -1, 0 and 1 throughout the\n> code makes me uncomfortable. Ordinarily, I would expect a simple\n>\n> argv_array_push(&args, opt_autostash ? \"--autostash\" : \"--no-autostash\");\n\nThis looks good. I will change accordingly.\n\n> Stepping back a bit, the only reason why we introduced opt_autostash =\n> -1 as a possible value is because we need to detect if\n> --[no-]autostash is being used with git-merge. I consider that a\n> kludge, because if git-merge supports --autostash as well (possibly in\n> the future), then we will not need this -1 value.\n>\n> So, from that point of view, a -1 value is okay as a workaround, but\n> kludges, and hence the -1 value, should be gotten rid off as soon as\n> possible.\n\nThat right! But until git-merge doesn't support --autostash, it's necessary to\nhave opt_autostash = -1 as default.\n\nI wonder if it will be a good thing to add the following line to the\ncommit message\n\"Changes needed to be introduced:\nChange opt_autostash = 0 as default as soon as git-merge supports\n--autostash option, also display no error when \"git pull --autostash\"\nis called.\"\n\nPossibly it would be better to add gmane link of your review in the\ncommit message\nfor further clarification.\n\nThanks,\nMehul\n"},{"id":"280618","messageId":"vpq8u1p16rv.fsf@anie.imag.fr","threadId":"41554","inReplyTo":"CA+DCAeSvjyy6WO=n-wdo3_yHGSWmUC0Epbt-OYLF_fbuczeeKA@mail.gmail.com","subject":"Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-11T13:30:28Z","receivedAt":"2016-03-11T13:30:28Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain writes:\n\n> On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan wrote:\n>>> static int config_autostash = -1;\n>>\n>> Hmm, why can't config_autostash just default to 0?\n>\n> Previously Junio recommended not to explicitly initialize a\n> static to 0 (or NULL).\n> http://thread.gmane.org/gmane.comp.version-control.git/287709/focus=287726\n\nWhat Junio says is that you don't need to write\n\nstatic int config_autostash = 0;\n\nsince it is equivalent to\n\nstatic int config_autostash;\n\nBut there's nothing wrong with having a static variable defaulting to 0.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"280620","messageId":"CA+DCAeTRavhVb05sXsi-P5NKGw7xRAZLnc2-XC+8f6ZWZgsJhA@mail.gmail.com","threadId":"41554","inReplyTo":"vpq8u1p16rv.fsf@anie.imag.fr","subject":"Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-11T14:38:23Z","receivedAt":"2016-03-11T14:38:23Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 11, 2016 at 7:00 PM, Matthieu Moy\n wrote:\n> What Junio says is that you don't need to write\n>\n> static int config_autostash = 0;\n>\n> since it is equivalent to\n>\n> static int config_autostash;\n>\n> But there's nothing wrong with having a static variable defaulting to 0.\n\nMy bad. I should have read Junio's comment more carefully.\n\nconfig_autostash can be default to 0. And thus\n if (opt_autostash != 1)\n die_on_unclean_work_tree(prefix);\n\ncan be replaced by\n if (!opt_autostash)\n die_on_unclean_work_tree(prefix);\n\nand thus opt_autostash will be either 0 or 1 and we don't\nhave to worry about it being -1 (whenever --rebase is passed).\n\nI will make the changes accordingly.\n\nThanks,\nMehul\n"},{"id":"280657","messageId":"CA+DCAeSF5UJ-CVe0bFrvx=iixs-wjZ8T_OALCvMA9oN+SfYWoA@mail.gmail.com","threadId":"41554","inReplyTo":"CACRoPnRESpiNL_=rEgw7kRdKjek8Z=qVjPH=BEWsWeYn22YryQ@mail.gmail.com","subject":"Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-12T06:49:13Z","receivedAt":"2016-03-12T06:49:13Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan wrote:\n> Stepping back a bit, the only reason why we introduced opt_autostash =\n> -1 as a possible value is because we need to detect if\n> --[no-]autostash is being used with git-merge. I consider that a\n> kludge, because if git-merge supports --autostash as well (possibly in\n> the future), then we will not need this -1 value.\n\nNo, there is one more reason for which opt_autostash = -1 is required.\nWhen user calls \"git pull --rebase\" then config_autostash value will\nbe used to perform --[no-]autostash task but if user calls \"git pull\n--rebase --[no-]autostash\" then config_autostash value should not be\nread at all as this option is supposed to override config_autostash\nvalue. So if opt_autostash defaults to 0 then how will the code\nunderstand if \"--[no-]autostash\" flag is passed or not?\n\nAs per the current patch, the value opt_autostash = 0 or 1 tells us\nthat the user has explicitly asked for --no-autostash or --autostash\nrespectively, and -1 value tells us that user has not specified\nanything and thus we should read config_autostash value to perform\n--[no-]autostash.\n\nOne way to do this was to read rebase.autoStash before parse_options(),\nbut now as we have introduced a callback function git_pull_config(),\nreading this config variable before parse_option() will now require\ncalling git_config(git_pull_config, NULL) before parse_option() and\ndoing opt_autostash = config_autostash there only.This may lead to\nsome problems (I'm not sure of that), as git_config() reads many other\nconfig variables too.\n\nThanks,\nMehul\n"},{"id":"280811","messageId":"1458061904-26516-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 1/2] git-pull.c: introduce git_pull_config()","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-15T17:11:43Z","receivedAt":"2016-03-15T17:11:43Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"git-pull makes a seperate call to git_config_get_bool() to read the value\nof \"rebase.autostash\", this can be reduced as a call to git_config() is\nalready there in the code.\n\nIntroduce a callback function git_pull_config() to read \"rebase.autostash\"\nalong with other variables.\n\nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nSigned-off-by: Mehul Jain \n---\nPrevious patches: http://thread.gmane.org/gmane.comp.version-control.git/287709\n\nChange: config_autostash initialized with 0 instead of -1\n\n builtin/pull.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..43353f9 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int config_autostash;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n static char *opt_gpg_sign;\n@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)\n \n \treturn REBASE_FALSE;\n }\n+/**\n+ * Read config variables.\n+ */\n+static int git_pull_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"rebase.autostash\")) {\n+\t\tconfig_autostash = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\treturn git_default_config(var, value, cb);\n+}\n \n /**\n * Returns 1 if there are unstaged changes, 0 otherwise.\n@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase();\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_pull_config, NULL);\n \n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"Pull\");\n@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\thashclr(orig_head);\n \n \tif (opt_rebase) {\n-\t\tint autostash = 0;\n \n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tgit_config_get_bool(\"rebase.autostash\", &autostash);\n-\t\tif (!autostash)\n+\t\tif (config_autostash)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280812","messageId":"1458061904-26516-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41554","inReplyTo":"1458061904-26516-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-15T17:11:44Z","receivedAt":"2016-03-15T17:11:44Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"If rebase.autoStash configuration variable is set, there is no way to\noverride it for \"git pull --rebase\" from the command line.\n\nTeach \"git pull --rebase\" the --[no-]autostash command line flag which\noverrides the current value of rebase.autoStash, if set. As \"git rebase\"\nunderstands the --[no-]autostash option, it's just a matter of passing\nthe option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nHelped-by: Matthieu Moy \nHelped-by: Junio C Hamano \nHelped-by: Paul Tan \nHelped-by: Eric Sunshine \nSigned-off-by: Mehul Jain \n---\nprevious patches: http://thread.gmane.org/gmane.comp.version-control.git/287709\n\n Documentation/git-pull.txt | 9 +++++++++\n builtin/pull.c | 13 +++++++++++--\n t/t5520-pull.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..a070ec9 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n --no-rebase::\n \tOverride earlier --rebase.\n \n+--autostash::\n+--no-autostash::\n+\tBefore starting rebase, stash local modifications away (see\n+\tlinkgit:git-stash.txt[1]) if needed, and apply the stash when\n+\tdone (this option is only valid when \"--rebase\" is used).\n++\n+`--no-autostash` is useful to override the `rebase.autoStash`\n+configuration variable (see linkgit:git-config[1]).\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 43353f9..c48e28a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int opt_autostash = -1;\n static int config_autostash;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -150,6 +151,8 @@ static struct option pull_options[] = {\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n+\tOPT_BOOL(0, \"autostash\", &opt_autostash,\n+\t\tN_(\"automatically stash/stash pop before and after rebase\")),\n \tOPT_PASSTHRU_ARGV('s', \"strategy\", &opt_strategies, N_(\"strategy\"),\n \t\tN_(\"merge strategy to use\"),\n \t\t0),\n@@ -801,6 +804,7 @@ static int run_rebase(const unsigned char *curr_head,\n \targv_array_pushv(&args, opt_strategy_opts.argv);\n \tif (opt_gpg_sign)\n \t\targv_array_push(&args, opt_gpg_sign);\n+\targv_array_push(&args, opt_autostash ? \"--autostash\" : \"--no-autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (is_null_sha1(orig_head) && !is_cache_unborn())\n \t\t\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n \n-\t\tif (config_autostash)\n+\t\tif (opt_autostash == -1)\n+\t\t\topt_autostash = config_autostash;\n+\n+\t\tif (!opt_autostash)\n \t\t\tdie_on_unclean_work_tree(prefix);\n \n \t\tif (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n \t\t\thashclr(rebase_fork_point);\n-\t}\n+\t} else\n+\t\tif (opt_autostash != -1)\n+\t\t\t die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \n \tif (run_fetch(repo, refspecs))\n \t\treturn 1;\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..85d9bea 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n \ttest \"$(cat new_file)\" = dirty &&\n \ttest \"$(cat file)\" = \"modified again\"\n '\n+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\tgit pull --rebase --autostash . copy &&\n+\ttest_cmp_rev HEAD^ copy &&\n+\ttest \"$(cat new_file)\" = dirty &&\n+\ttest \"$(cat file)\" = \"modified again\"\n+'\n+\n+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\n+\ttest_config rebase.autostash true &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n+\n+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '\n+\ttest_config rebase.autostash false &&\n+\tgit reset --hard before-rebase &&\n+\techo dirty >new_file &&\n+\tgit add new_file &&\n+\ttest_must_fail git pull --rebase --no-autostash . copy 2>err &&\n+\ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n+'\n \n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"280846","messageId":"CAPig+cSnp+NsBAMib4pExKCLB5ocGsHWyO7qMU0E91WqE6a5_g@mail.gmail.com","threadId":"41554","inReplyTo":"1458061904-26516-2-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-15T21:43:27Z","receivedAt":"2016-03-15T21:43:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 15, 2016 at 1:11 PM, Mehul Jain wrote:\n> If rebase.autoStash configuration variable is set, there is no way to\n> override it for \"git pull --rebase\" from the command line.\n>\n> Teach \"git pull --rebase\" the --[no-]autostash command line flag which\n> overrides the current value of rebase.autoStash, if set. As \"git rebase\"\n> understands the --[no-]autostash option, it's just a matter of passing\n> the option to underlying \"git rebase\" when \"git pull --rebase\" is called.\n\nThe below comments may or may not be worth a re-roll (you decide)...\n\n> Signed-off-by: Mehul Jain \n> ---\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n> +--autostash::\n> +--no-autostash::\n> + Before starting rebase, stash local modifications away (see\n> + linkgit:git-stash.txt[1]) if needed, and apply the stash when\n> + done (this option is only valid when \"--rebase\" is used).\n> ++\n> +`--no-autostash` is useful to override the `rebase.autoStash`\n> +configuration variable (see linkgit:git-config[1]).\n\nThe last couple sentences seem reversed. It feels odd to have the bit\nabout --rebase dropped dead in the middle of the description of\n--autostash and --no-autostash. I'd have expected to see --autostash\nand --no-autostash discussed together, and then, as a follow-on,\nmention them being valid only with --rebase.\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> @@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>\n> - if (config_autostash)\n> + if (opt_autostash == -1)\n\nIn patch 1/2, this changed from 'if (autostash)' to 'if\n(config_autostash)'; it's a bit sad that patch 2/2 then has to touch\nthe same code to change it yet again, this time to 'if\n(opt_autostash)'. Have you tried just keeping the original 'autostash'\nvariable and modifying its value based upon config_autostash and\nopt_autostash instead? (Not saying that this would be better, but\ninterested in knowing if the result is as clean or cleaner or worse.)\n\n> + opt_autostash = config_autostash;\n> +\n> + if (!opt_autostash)\n> die_on_unclean_work_tree(prefix);\n>\n> if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n> hashclr(rebase_fork_point);\n> - }\n> + } else\n> + if (opt_autostash != -1)\n> + die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n\nHow about formatting this as a normal 'else if'?\n\n } else if (opt_autostash != -1)\n\nOn the other hand, this error case hanging off the 'rebase'\nconditional is somewhat more difficult to reason about than perhaps it\nought to be. It might be easier to see what's going on if you get the\nerror case out of the way early, and then handle the normal case. That\nis, something like this:\n\n if (!opt_rebase && opt_autostash != -1)\n die(_(\"... is only valid with --rebase\"));\n\n if (opt_rebase) {\n ...\n }\n\n> if (run_fetch(repo, refspecs))\n> return 1;\n"},{"id":"280874","messageId":"CA+DCAeRWRiY+qCZ440NbU+DMg0OQ6C1bSpYfsHEqjiOvhX69Eg@mail.gmail.com","threadId":"41554","inReplyTo":"CAPig+cSnp+NsBAMib4pExKCLB5ocGsHWyO7qMU0E91WqE6a5_g@mail.gmail.com","subject":"Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-16T05:00:38Z","receivedAt":"2016-03-16T05:00:38Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine wrote:\n>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n>> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.\n>> +--autostash::\n>> +--no-autostash::\n>> + Before starting rebase, stash local modifications away (see\n>> + linkgit:git-stash.txt[1]) if needed, and apply the stash when\n>> + done (this option is only valid when \"--rebase\" is used).\n>> ++\n>> +`--no-autostash` is useful to override the `rebase.autoStash`\n>> +configuration variable (see linkgit:git-config[1]).\n>\n> The last couple sentences seem reversed. It feels odd to have the bit\n> about --rebase dropped dead in the middle of the description of\n> --autostash and --no-autostash. I'd have expected to see --autostash\n> and --no-autostash discussed together, and then, as a follow-on,\n> mention them being valid only with --rebase.\n\nSo you are suggesting something like this:\n\n--autostash::\n--no-autostash::\n Before starting rebase, stash local modifications away (see\n linkgit:git-stash.txt[1]) if needed, and apply the stash when\n done. `--no-autostash` is useful to override the `rebase.autoStash`\n configuration variable (see linkgit:git-config[1]).\n+\nThis option is only valid when \"--rebase\" is used.\n\nCan be done and it make more sense to talk about the validity of the\noption in a seperate line.\n\n>> diff --git a/builtin/pull.c b/builtin/pull.c\n>> @@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>> if (is_null_sha1(orig_head) && !is_cache_unborn())\n>> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>>\n>> - if (config_autostash)\n>> + if (opt_autostash == -1)\n>\n> In patch 1/2, this changed from 'if (autostash)' to 'if\n> (config_autostash)'; it's a bit sad that patch 2/2 then has to touch\n> the same code to change it yet again, this time to 'if\n> (opt_autostash)'. Have you tried just keeping the original 'autostash'\n> variable and modifying its value based upon config_autostash and\n> opt_autostash instead? (Not saying that this would be better, but\n> interested in knowing if the result is as clean or cleaner or worse.)\n\nYes, I tried that. Things looked something like this:\n\nIn patch 1/2\n...\n\n- int autostash = 0;\n+ int autostash = config_autoshash;\n\n if (is_null_sha1(orig_head) && !is_cache_unborn())\n die(_(\"Updating ...\"));\n\n- git_config_get_bool(\"rebase.autostash\", &autostash);\n if (!autostash)\n die_on_unclean_work_tree(prefix);\n\n...\n\nIn patch 2/2\n...\n int autostash = config_autoshash;\n\n if (is_null_sha1(orig_head) && !is_cache_unborn())\n die(_(\"Updating ...\"));\n\n+ if (opt_autostash != -1)\n+ autostash = opt_autostash;\n\n if (!autostash)\n die_on_unclean_work_tree(prefix);\n...\n\nThis implementation looks much more cleaner but we are using some\nextra space (autostash) to do the task. If everyone is fine with this\ntrade off then I can re-roll a new patch with this method. Comments please.\n\n>> + opt_autostash = config_autostash;\n>> +\n>> + if (!opt_autostash)\n>> die_on_unclean_work_tree(prefix);\n>>\n>> if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))\n>> hashclr(rebase_fork_point);\n>> - }\n>> + } else\n>> + if (opt_autostash != -1)\n>> + die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n>\n> How about formatting this as a normal 'else if'?\n>\n> } else if (opt_autostash != -1)\n\nI thought of it earlier but voted against it as it may reduce the readability of\nthe code.\n\n> On the other hand, this error case hanging off the 'rebase'\n> conditional is somewhat more difficult to reason about than perhaps it\n> ought to be. It might be easier to see what's going on if you get the\n> error case out of the way early, and then handle the normal case. That\n> is, something like this:\n>\n> if (!opt_rebase && opt_autostash != -1)\n> die(_(\"... is only valid with --rebase\"));\n>\n> if (opt_rebase) {\n> ...\n> }\n\nThis is good. I'll make the changes accordingly.\n\nThanks for the comments.\n\nMehul\n"},{"id":"281015","messageId":"CA+DCAeSoyKyO+SVYuY7oYF-2YK4pgvQEKpe7FpU6=5Ag+KeT-Q@mail.gmail.com","threadId":"41554","inReplyTo":"CAPig+cSnp+NsBAMib4pExKCLB5ocGsHWyO7qMU0E91WqE6a5_g@mail.gmail.com","subject":"Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-17T08:17:44Z","receivedAt":"2016-03-17T08:17:44Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Hello Eric,\n\nI tried out this approach and here's the result.\n\n---\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex b5b0255..0ce007d 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -86,6 +86,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *opt_ff;\n static char *opt_verify_signatures;\n+static int opt_autostash = -1;\n static int config_autostash;\n static struct argv_array opt_strategies = ARGV_ARRAY_INIT;\n static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;\n@@ -150,6 +151,8 @@ static struct option pull_options[] = {\n OPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n N_(\"verify that the named commit has a valid GPG signature\"),\n PARSE_OPT_NOARG),\n+ OPT_BOOL(0, \"autostash\", &opt_autostash,\n+ N_(\"automatically stash/stash pop before and after rebase\")),\n OPT_PASSTHRU_ARGV('s', \"strategy\", &opt_strategies, N_(\"strategy\"),\n N_(\"merge strategy to use\"),\n 0),\n@@ -801,6 +804,7 @@ static int run_rebase(const unsigned char *curr_head,\n argv_array_pushv(&args, opt_strategy_opts.argv);\n if (opt_gpg_sign)\n argv_array_push(&args, opt_gpg_sign);\n+ argv_array_push(&args, opt_autostash ? \"--autostash\" :\n\"--no-autostash\");\n\n argv_array_push(&args, \"--onto\");\n argv_array_push(&args, sha1_to_hex(merge_head));\n@@ -846,12 +850,20 @@ int cmd_pull(int argc, const char **argv, const\nchar *prefix)\n if (get_sha1(\"HEAD\", orig_head))\n hashclr(orig_head);\n\n+ if(!opt_rebase && opt_autostash != -1)\n+ die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n+\n if (opt_rebase) {\n int autostash = config_autostash;\n\n if (is_null_sha1(orig_head) && !is_cache_unborn())\n die(_(\"Updating an unborn branch with changes\nadded to the index.\"));\n\n+ if (opt_autostash != -1)\n+ autostash = opt_autostash;\n+ else\n+ opt_autostash = config_autostash;\n+\n if (!autostash)\n die_on_unclean_work_tree(prefix);\n---\n\nThis way of implementation looks a bit less clean to me than\nthe previous one because we are using \"opt_autostash\" to pass\nthe \"--[no-]autostash\" flag to git-rebase, thus if user does not\nspecify anything about stashing in command line then config_autostash\nvalue has to be used ( i.e. opt_autostash = config_autostash).\nTo do this an \"else\" case has to be introduced in the code. This\nmight effect the readability of the code because the reader might\nwonder why \"opt_autostash\" is used to assign value to \"autostash\"\nin one case, and opt_autostash = config_autostash in other case.\n\nBut I agree that this way I won't be touching the changes I made\nin patch 1/2.\n\nI would like to know your view on above mentioned issue.\n\nAlso I made a mistake in patch 1/2 which I will correct in the next\nversion along with other changes suggested by you.\n\nThanks,\nMehul\n"},{"id":"281105","messageId":"CAPig+cQyAJhaxhQvbtRyNrCq083q58K4yJ20BRoVw2WPkrK8VQ@mail.gmail.com","threadId":"41554","inReplyTo":"CA+DCAeRWRiY+qCZ440NbU+DMg0OQ6C1bSpYfsHEqjiOvhX69Eg@mail.gmail.com","subject":"Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T03:32:23Z","receivedAt":"2016-03-18T03:32:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 16, 2016 at 1:00 AM, Mehul Jain wrote:\n> On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine wrote:\n>>> +--autostash::\n>>> +--no-autostash::\n>>> + Before starting rebase, stash local modifications away (see\n>>> + linkgit:git-stash.txt[1]) if needed, and apply the stash when\n>>> + done (this option is only valid when \"--rebase\" is used).\n>>> ++\n>>> +`--no-autostash` is useful to override the `rebase.autoStash`\n>>> +configuration variable (see linkgit:git-config[1]).\n>>\n>> The last couple sentences seem reversed. It feels odd to have the bit\n>> about --rebase dropped dead in the middle of the description of\n>> --autostash and --no-autostash. I'd have expected to see --autostash\n>> and --no-autostash discussed together, and then, as a follow-on,\n>> mention them being valid only with --rebase.\n>\n> So you are suggesting something like this:\n>\n> --autostash::\n> --no-autostash::\n> Before starting rebase, stash local modifications away (see\n> linkgit:git-stash.txt[1]) if needed, and apply the stash when\n> done. `--no-autostash` is useful to override the `rebase.autoStash`\n> configuration variable (see linkgit:git-config[1]).\n> +\n> This option is only valid when \"--rebase\" is used.\n>\n> Can be done and it make more sense to talk about the validity of the\n> option in a seperate line.\n\nYes, like that.\n\n>>> diff --git a/builtin/pull.c b/builtin/pull.c\n>>> @@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>>> if (is_null_sha1(orig_head) && !is_cache_unborn())\n>>> die(_(\"Updating an unborn branch with changes added to the index.\"));\n>>>\n>>> - if (config_autostash)\n>>> + if (opt_autostash == -1)\n>>\n>> In patch 1/2, this changed from 'if (autostash)' to 'if\n>> (config_autostash)'; it's a bit sad that patch 2/2 then has to touch\n>> the same code to change it yet again, this time to 'if\n>> (opt_autostash)'. Have you tried just keeping the original 'autostash'\n>> variable and modifying its value based upon config_autostash and\n>> opt_autostash instead? (Not saying that this would be better, but\n>> interested in knowing if the result is as clean or cleaner or worse.)\n>\n> Yes, I tried that. Things looked something like this:\n>\n> In patch 1/2\n> ...\n>\n> - int autostash = 0;\n> + int autostash = config_autoshash;\n>\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating ...\"));\n>\n> - git_config_get_bool(\"rebase.autostash\", &autostash);\n> if (!autostash)\n> die_on_unclean_work_tree(prefix);\n\nThe individual diffs look nicer and are less noisy, thus a bit easier to review.\n\n> In patch 2/2\n> ...\n> int autostash = config_autoshash;\n>\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating ...\"));\n>\n> + if (opt_autostash != -1)\n> + autostash = opt_autostash;\n\nI'd probably have placed this conditional just below the line where\n'autostash' is declared so that the logic for computing the value of\n'autostash' is all in one place.\n\n> if (!autostash)\n> die_on_unclean_work_tree(prefix);\n> ...\n>\n> This implementation looks much more cleaner but we are using some\n> extra space (autostash) to do the task. If everyone is fine with this\n> trade off then I can re-roll a new patch with this method. Comments please.\n\nUsing an extra variable isn't a big deal and would be a good idea if\nit helped clarify the logic. In this case, the logic isn't\nparticularly difficult, so I don't feel too strongly about it one way\nor the other.\n"},{"id":"281106","messageId":"CAPig+cRmA5qQQxrHhE=3b7QPKWtSQq6ks2qPRc5v0yrE20pRsg@mail.gmail.com","threadId":"41554","inReplyTo":"CA+DCAeSoyKyO+SVYuY7oYF-2YK4pgvQEKpe7FpU6=5Ag+KeT-Q@mail.gmail.com","subject":"Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T03:53:16Z","receivedAt":"2016-03-18T03:53:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain wrote:\n> I tried out this approach and here's the result.\n>\n> + if(!opt_rebase && opt_autostash != -1)\n> + die(_(\"--[no-]autostash option is only valid with --rebase.\"));\n> +\n> if (opt_rebase) {\n> int autostash = config_autostash;\n>\n> if (is_null_sha1(orig_head) && !is_cache_unborn())\n> die(_(\"Updating an unborn branch with changes\n> added to the index.\"));\n>\n> + if (opt_autostash != -1)\n> + autostash = opt_autostash;\n> + else\n> + opt_autostash = config_autostash;\n> if (!autostash)\n> die_on_unclean_work_tree(prefix);\n>\n> This way of implementation looks a bit less clean to me than\n> the previous one because we are using \"opt_autostash\" to pass\n> the \"--[no-]autostash\" flag to git-rebase, thus if user does not\n> specify anything about stashing in command line then config_autostash\n> value has to be used ( i.e. opt_autostash = config_autostash).\n> To do this an \"else\" case has to be introduced in the code. This\n> might effect the readability of the code because the reader might\n> wonder why \"opt_autostash\" is used to assign value to \"autostash\"\n> in one case, and opt_autostash = config_autostash in other case.\n\nThat's pretty ugly. Since cmd_pull() is the only caller of\nrun_rebase(), an alternative would be to pass 'autostash' as an\nargument to run_rebase(). However, since run_rebase() is already\naccessing other 'opt_foo' globals, it wouldn't make sense to make an\nexception of 'autostash' by passing it as an argument. So, in the end,\nthe original approach is indeed probably cleaner.\n\n> Also I made a mistake in patch 1/2 which I will correct in the next\n> version along with other changes suggested by you.\n\nWhich mistake would that be?\n"}]}