{"thread":{"id":"41770","subject":"[PATCH v10 0/2] introduce --[no-]autostash command line flag","startedAt":"2016-03-21T18:18:01Z","lastAt":"2016-03-25T22:29:43Z","messageCount":16,"participants":["Mehul Jain","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":10,"patchTotal":2},"messages":[{"id":"281358","messageId":"1458584283-23816-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41770","inReplyTo":null,"subject":"[PATCH v10 0/2] introduce --[no-]autostash command line flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-21T18:18:01Z","receivedAt":"2016-03-21T18:18:01Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"Following series of patches introduce --[no-]autostash command line flag\nfor \"git pull --rebase\".\n\n[PATCH v10 1/2] git-pull.c: introduce git_pull_config()\nIt's a clean-up patch for changes introduced in [PATCH v10 2/2].\n\n[PATCH v10 2/2] pull --rebase: add --[no-]autostash flag\nChanges introduced w.r.t. previous patch:\n\n* Unnecessary tight coupling between git-rebase and git-pull introduced\n  in previous patch has been removed by passing \"--[no-]autostash\" option\n  to git-rebase only when user explicitly tell via command line.\n\n* Patch looks more clearer than before as \"autostash\" variable is used for\n  implementation of logic (thanks to Eric).\n\n* Test titles are modified for better understanding of tests.\n\n* Two new tests are added to cover all the combinations of\n  \"--[no-]autostash\" and rebase.autoStash.\n\n* Two more tests are added to checkout for error when \"git pull\n  --[no-]autostash\" is called. Here I'm forced to use \"test_i18ncmp\"\n  instead of \"test_i18ngrep\" to compare the expected error message with\n  the actual because grep was, unfortunately, reading \"--[no-]autostash\"\n  as an option and thus leading to test failure.\n\nPrevious patch: http://thread.gmane.org/gmane.comp.version-control.git/289127\n\nHere's the interdiff between previous patch and current patch.\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 671179b..d98f481 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -308,6 +308,7 @@ static enum rebase_type config_get_rebase(void)\n \n \treturn REBASE_FALSE;\n }\n+\n /**\n  * Read config variables.\n  */\n@@ -804,7 +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-\targv_array_push(&args, opt_autostash ? \"--autostash\" : \"--no-autostash\");\n+\tif (opt_autostash == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n+\telse if (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -854,13 +858,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--[no-]autostash option is only valid with --rebase.\"));\n \n \tif (opt_rebase) {\n+\t\tint autostash = config_autostash;\n+\t\tif (opt_autostash != -1)\n+\t\t\tautostash = opt_autostash;\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\tif (opt_autostash == -1)\n-\t\t\topt_autostash = config_autostash;\n-\n-\t\tif (!opt_autostash)\n+\t\tif (!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 85d9bea..745e59e 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -255,7 +255,19 @@ 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+\n+test_expect_success 'pull --rebase --autostash & rebase.autostash=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 --autostash & rebase.autoStash=false' '\n \ttest_config rebase.autostash false &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n@@ -266,8 +278,7 @@ test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\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+test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n \tgit add new_file &&\n@@ -277,7 +288,7 @@ test_expect_success 'pull --rebase --autostash works with rebase.autostash set t\n \ttest \"$(cat file)\" = \"modified again\"\n '\n \n-test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\n+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '\n \ttest_config rebase.autostash true &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n@@ -286,7 +297,7 @@ test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '\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+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n \ttest_config rebase.autostash false &&\n \tgit reset --hard before-rebase &&\n \techo dirty >new_file &&\n@@ -295,6 +306,26 @@ test_expect_success 'pull --rebase --no-autostash works with rebase.autostash se\n \ttest_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n '\n \n+test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\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 --autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\n+'\n+\n+test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --no-autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\n+'\n+\n test_expect_success 'pull.rebase' '\n \tgit reset --hard before-rebase &&\n \ttest_config pull.rebase true &&\n\n\nMehul Jain (2):\n  git-pull.c: introduce git_pull_config()\n  pull --rebase: add --[no-]autostash flag\n\n Documentation/git-pull.txt |  9 ++++++\n builtin/pull.c             | 30 ++++++++++++++++++--\n t/t5520-pull.sh            | 70 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 106 insertions(+), 3 deletions(-)\n\n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"281359","messageId":"1458584283-23816-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41770","inReplyTo":"1458584283-23816-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v10 1/2] git-pull.c: introduce git_pull_config()","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-21T18:18:02Z","receivedAt":"2016-03-21T18:18:02Z","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 <gitster@pobox.com>\nHelped-by: Paul Tan <pyokagan@gmail.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n---\n builtin/pull.c | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..c21897d 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@@ -306,6 +307,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@@ -823,7 +836,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,12 +848,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+\t\tint autostash = config_autostash;\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\t\tdie_on_unclean_work_tree(prefix);\n \n-- \n2.7.1.340.g69eb491.dirty\n"},{"id":"281360","messageId":"1458584283-23816-3-git-send-email-mehul.jain2029@gmail.com","threadId":"41770","inReplyTo":"1458584283-23816-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-21T18:18:03Z","receivedAt":"2016-03-21T18:18:03Z","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 <Matthieu.Moy@grenoble-inp.fr>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Paul Tan <pyokagan@gmail.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n---\n Documentation/git-pull.txt |  9 ++++++\n builtin/pull.c             | 12 ++++++++\n t/t5520-pull.sh            | 70 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 91 insertions(+)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..3914507 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. `--no-autostash` is useful to override the `rebase.autoStash`\n+\tconfiguration variable (see linkgit:git-config[1]).\n++\n+This option is only valid when \"--rebase\" is used.\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex c21897d..d98f481 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@@ -802,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 == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n+\telse if (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -847,8 +854,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(\"HEAD\", orig_head))\n \t\thashclr(orig_head);\n \n+\tif (!opt_rebase && opt_autostash != -1)\n+\t\tdie(_(\"--[no-]autostash option is only valid with --rebase.\"));\n+\n \tif (opt_rebase) {\n \t\tint autostash = config_autostash;\n+\t\tif (opt_autostash != -1)\n+\t\t\tautostash = opt_autostash;\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.\"));\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..745e59e 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -256,6 +256,76 @@ 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 & rebase.autostash=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 --autostash & rebase.autoStash=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: --autostash & rebase.autoStash unset' '\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 & rebase.autostash=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 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 & rebase.autostash=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 --no-autostash & rebase.autostash unset' '\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 --autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\n+'\n+\n+test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --no-autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\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":"281370","messageId":"vpq37rjr7y9.fsf@anie.imag.fr","threadId":"41770","inReplyTo":"1458584283-23816-3-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-21T18:39:58Z","receivedAt":"2016-03-21T18:39:58Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain <mehul.jain2029@gmail.com> writes:\n\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\nPlease drop the \".txt\" after linkgit.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"281377","messageId":"1458591170-28079-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41770","inReplyTo":"1458584283-23816-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-21T20:12:50Z","receivedAt":"2016-03-21T20:12:50Z","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 <Matthieu.Moy@grenoble-inp.fr>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Paul Tan <pyokagan@gmail.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n---\n Documentation/git-pull.txt |  9 ++++++\n builtin/pull.c             | 12 ++++++++\n t/t5520-pull.sh            | 70 ++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 91 insertions(+)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex a62a2a6..3914507 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[1]) if needed, and apply the stash when\n+\tdone. `--no-autostash` is useful to override the `rebase.autoStash`\n+\tconfiguration variable (see linkgit:git-config[1]).\n++\n+This option is only valid when \"--rebase\" is used.\n+\n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex c21897d..d98f481 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@@ -802,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 == 0)\n+\t\targv_array_push(&args, \"--no-autostash\");\n+\telse if (opt_autostash == 1)\n+\t\targv_array_push(&args, \"--autostash\");\n \n \targv_array_push(&args, \"--onto\");\n \targv_array_push(&args, sha1_to_hex(merge_head));\n@@ -847,8 +854,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(\"HEAD\", orig_head))\n \t\thashclr(orig_head);\n \n+\tif (!opt_rebase && opt_autostash != -1)\n+\t\tdie(_(\"--[no-]autostash option is only valid with --rebase.\"));\n+\n \tif (opt_rebase) {\n \t\tint autostash = config_autostash;\n+\t\tif (opt_autostash != -1)\n+\t\t\tautostash = opt_autostash;\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.\"));\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex c952d5e..745e59e 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -256,6 +256,76 @@ 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 & rebase.autostash=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 --autostash & rebase.autoStash=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: --autostash & rebase.autoStash unset' '\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 & rebase.autostash=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 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 & rebase.autostash=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 --no-autostash & rebase.autostash unset' '\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 --autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\n+'\n+\n+test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n+\ttest_must_fail git pull --no-autostash . copy 2>actual &&\n+\techo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n+\ttest_i18ncmp actual expect\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":"281783","messageId":"CAPig+cTqnev_YpamaSi1tkvWydZHRadBzo_zLnF1Pd6FyWKiTQ@mail.gmail.com","threadId":"41770","inReplyTo":"1458584283-23816-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v10 0/2] introduce --[no-]autostash command line flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T07:23:51Z","receivedAt":"2016-03-25T07:23:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 2:18 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\n> Changes introduced w.r.t. previous patch:\n> [...]\n> * Two more tests are added to checkout for error when \"git pull\n>   --[no-]autostash\" is called. Here I'm forced to use \"test_i18ncmp\"\n>   instead of \"test_i18ngrep\" to compare the expected error message with\n>   the actual because grep was, unfortunately, reading \"--[no-]autostash\"\n>   as an option and thus leading to test failure.\n\nPass -e to grep to treat the next argument as an expression (even if\nit happens to look like an option):\n\n    test_i18ngrep -e \"--[no-]-autostash ...\"\n\nYou may also need to escape the [ and ] with backslash (\\) to force\ngrep to treat them as literal characters rather than as the character\nset \"[no-]\". Alternately, rather than escaping, also pass the -F flag\nto make it treat all characters as literals.\n"},{"id":"281784","messageId":"CAPig+cSdegoGNCMBMcHyEYiE+LUzixvdk-qu0Q-zbFvatX2=KA@mail.gmail.com","threadId":"41770","inReplyTo":"1458591170-28079-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T08:31:35Z","receivedAt":"2016-03-25T08:31:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 4:12 PM, Mehul Jain <mehul.jain2029@gmail.com> 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\nThis version of the patch (coupled with patch 1/2) is a pleasant\nimprovement over previous versions due to the cleaner structure, less\nnoisy diff, and general simplicity (thus easier to reason about and\nreview).\n\nSee below for a nit and some comments about the tests.\n\n> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n> ---\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> @@ -256,6 +256,76 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb\n>         test \"$(cat file)\" = \"modified again\"\n>  '\n>\n> +test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '\n\nNit: Some of the test titles spell this as \"rebase.autostash\" while\nothers use \"rebase.autoStash\".\n\n> +       test_config rebase.autostash true &&\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       git pull --rebase --autostash . copy &&\n> +       test_cmp_rev HEAD^ copy &&\n> +       test \"$(cat new_file)\" = dirty &&\n> +       test \"$(cat file)\" = \"modified again\"\n> +'\n> +\n> +test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '\n> +       test_config rebase.autostash false &&\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       git pull --rebase --autostash . copy &&\n> +       test_cmp_rev HEAD^ copy &&\n> +       test \"$(cat new_file)\" = dirty &&\n> +       test \"$(cat file)\" = \"modified again\"\n> +'\n> +\n> +test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '\n\nThe title says that this is testing with rebase.autoStash unset,\nhowever, the test itself doesn't take any action to ensure that it is\nindeed unset. As with the two above tests which explicitly set\nrebase.autoStash, this test should explicitly unset rebase.autoStash\nto ensure consistent results even if some future change somehow\npollutes the configuration globally. Therefore:\n\n    test_unconfig rebase.autostash &&\n\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       git pull --rebase --autostash . copy &&\n> +       test_cmp_rev HEAD^ copy &&\n> +       test \"$(cat new_file)\" = dirty &&\n> +       test \"$(cat file)\" = \"modified again\"\n> +'\n\nWith the addition of these three new tests, aside from the\nintroductory 'test_{un}config', this exact sequence of commands is now\nrepeated four times in the script. Such repetition suggests that the\ncommon code should be moved to a function. For instance:\n\n    test_rebase_autostash () {\n        git reset --hard before-rebase &&\n        echo dirty >new_file &&\n        git add new_file &&\n        git pull --rebase . copy &&\n        test_cmp_rev HEAD^ copy &&\n        test \"$(cat new_file)\" = dirty &&\n        test \"$(cat file)\" = \"modified again\"\n    }\n\nAnd, a caller would look like this:\n\n    test_expect_success 'pull ... rebase.autostash=true' '\n        test_config rebase.autostash true &&\n        test_rebase_autostash\n    '\n\nOf course, you'd also update the original test, from which this code\nwas copied, to also call the new function. Factoring out the common\ncode into a function should probably be done as a separate preparatory\npatch.\n\nThis suggestion isn't mandatory and doesn't demand a re-roll, but, if\nyou're feeling ambitious, it would make the code easier to digest and\nreview.\n\n> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '\n> +       test_config rebase.autostash true &&\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n\nI don't care strongly, but many tests consider test_must_fail() alone\nsufficient to verify proper behavior and don't bother being more exact\nby checking the precise error message (since error messages sometimes\nget refined, thus requiring adjustments to the tests). If you do\nretain the error message check, it's often sufficient to check for\njust a fragment of the error string rather than the full message. For\ninstance, it might be fine to grep merely for \"uncommitted changes\".\n\n> +'\n> +\n> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n> +       test_config rebase.autostash false &&\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n> +'\n> +\n> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n\nSame comment as above:\n\n    test_unconfig rebase.autostash &&\n\n> +       git reset --hard before-rebase &&\n> +       echo dirty >new_file &&\n> +       git add new_file &&\n> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n> +'\n\nSame comment as above about the common code shared by these three new\ntest: moving it to a function is suggested.\n\n> +test_expect_success 'pull --autostash (without --rebase) should error out' '\n> +       test_must_fail git pull --autostash . copy 2>actual &&\n> +       echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n> +       test_i18ncmp actual expect\n\nSame comment as above about checking the exact error message (vs. just\ntrusting test_must_fail).\n\nAlso, you mentioned in your cover letter that you couldn't use\ntest_i18ngrep because grep was mistaking \"--[no-]autostash\" in the\nabove expression as a command-line option. If you were using the exact\nstring as above as an argument to test_i18ngrep, then it is more\nlikely that the problem was that grep was seeing \"[no-]\" as a\ncharacter class rather than as a literal pattern to match. You could\nget around this either by escaping the [ and ] with a backslash (\\) or\nby passing -F to test_i18ngrep.\n\nAlternately, as mentioned above, just grep for a fragment of the error\nmessage, such as \"only valid with --rebase\", rather than the full\ndiagnostic.\n\n> +'\n> +\n> +test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n> +       test_must_fail git pull --no-autostash . copy 2>actual &&\n> +       echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n> +       test_i18ncmp actual expect\n> +'\n\nSame comment as above about code common to these two tests. However,\nin this case, it might be easier simply to use a 'for' loop rather\nthan a function:\n\n    for i in --autostash --no-autostash\n    do\n        test_expect_success \"pull $i (without --rebase) is illegal\" \"\n           test_must_fail git pull $i . copy 2>actual &&\n           test_i18ngrep 'only valid with --rebase' actual\n        \"\n    done\n\nTake special note of how use of double (\") and single (') quotes\ndiffer in this case from other tests since $i needs to be interpolated\ninto the test body.\n"},{"id":"281785","messageId":"CAPig+cTH-3PqZFyP_R1FyTPKhRhbbLRDeYfv2TcVq=gq=ZpRcQ@mail.gmail.com","threadId":"41770","inReplyTo":"CAPig+cSdegoGNCMBMcHyEYiE+LUzixvdk-qu0Q-zbFvatX2=KA@mail.gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T08:44:18Z","receivedAt":"2016-03-25T08:44:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>     for i in --autostash --no-autostash\n>     do\n>         test_expect_success \"pull $i (without --rebase) is illegal\" \"\n>            test_must_fail git pull $i . copy 2>actual &&\n>            test_i18ngrep 'only valid with --rebase' actual\n>         \"\n>     done\n>\n> Take special note of how use of double (\") and single (') quotes\n> differ in this case from other tests since $i needs to be interpolated\n> into the test body.\n\nThat's not accurate. Since $i will be visible when the test body is\nactually evaluated, it will work correctly even with the body\nsingle-quoted as usual (like all other tests), so swapping the quotes\naround like this is unnecessary (and Junio would prefer[1] they not be\nswapped).\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/284769\n"},{"id":"281787","messageId":"vpqshzfuduv.fsf@anie.imag.fr","threadId":"41770","inReplyTo":"1458591170-28079-1-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-25T09:05:28Z","receivedAt":"2016-03-25T09:05:28Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain <mehul.jain2029@gmail.com> writes:\n\n> +--autostash::\n> +--no-autostash::\n> +\tBefore starting rebase, stash local modifications away (see\n> +\tlinkgit:git-stash[1]) if needed, and apply the stash when\n> +\tdone. `--no-autostash` is useful to override the `rebase.autoStash`\n> +\tconfiguration variable (see linkgit:git-config[1]).\n> ++\n> +This option is only valid when \"--rebase\" is used.\n\nThis does not have to be added to this series (I don't want to break\neverything at v10 ...), but I think it would be nice to allow \"git pull\n--autostash\" even without --rebase if pull.rebase=true.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"281823","messageId":"CAPig+cTzV5FT=BBFW6kTUqPG8=ZWXf+78Y-HG=pb2x_8h8he1g@mail.gmail.com","threadId":"41770","inReplyTo":"CAPig+cTH-3PqZFyP_R1FyTPKhRhbbLRDeYfv2TcVq=gq=ZpRcQ@mail.gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T16:37:27Z","receivedAt":"2016-03-25T16:37:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 4:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Mar 25, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>     for i in --autostash --no-autostash\n>>     do\n>>         test_expect_success \"pull $i (without --rebase) is illegal\" \"\n>>            test_must_fail git pull $i . copy 2>actual &&\n>>            test_i18ngrep 'only valid with --rebase' actual\n>>         \"\n>>     done\n>>\n>> Take special note of how use of double (\") and single (') quotes\n>> differ in this case from other tests since $i needs to be interpolated\n>> into the test body.\n>\n> That's not accurate. Since $i will be visible when the test body is\n> actually evaluated, it will work correctly even with the body\n> single-quoted as usual (like all other tests), so swapping the quotes\n> around like this is unnecessary (and Junio would prefer[1] they not be\n> swapped).\n\nJunio pointed out to me privately that I forgot to mention explicitly\nthat you would need to use double quotes for the test title to ensure\nthat $i is interpolated, but the test body can continue using single\nquotes, as explained above.\n"},{"id":"281846","messageId":"CA+DCAeT0PW6oCjO5QxcL+nJYneLUGgvieki9_44HQJK4bDHaUQ@mail.gmail.com","threadId":"41770","inReplyTo":"CAPig+cTqnev_YpamaSi1tkvWydZHRadBzo_zLnF1Pd6FyWKiTQ@mail.gmail.com","subject":"Re: [PATCH v10 0/2] introduce --[no-]autostash command line flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-25T18:01:39Z","receivedAt":"2016-03-25T18:01:39Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 25, 2016 at 12:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Mar 21, 2016 at 2:18 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\n>> Changes introduced w.r.t. previous patch:\n>> [...]\n>> * Two more tests are added to checkout for error when \"git pull\n>>   --[no-]autostash\" is called. Here I'm forced to use \"test_i18ncmp\"\n>>   instead of \"test_i18ngrep\" to compare the expected error message with\n>>   the actual because grep was, unfortunately, reading \"--[no-]autostash\"\n>>   as an option and thus leading to test failure.\n>\n> Pass -e to grep to treat the next argument as an expression (even if\n> it happens to look like an option):\n>\n>     test_i18ngrep -e \"--[no-]-autostash ...\"\n>\n> You may also need to escape the [ and ] with backslash (\\) to force\n> grep to treat them as literal characters rather than as the character\n> set \"[no-]\". Alternately, rather than escaping, also pass the -F flag\n> to make it treat all characters as literals.\n\nThanks for this. I tried it out\n\n    test_i18ngrep -F -e \"--[no-]autostash ...\" err\n\nand worked fine.\n\nThanks,\nMehul\n"},{"id":"281848","messageId":"CA+DCAeRbD3S5Ltse3A6vBcvhKwh9t5av=Fnz98fD2ES5pbAN=Q@mail.gmail.com","threadId":"41770","inReplyTo":"CAPig+cSdegoGNCMBMcHyEYiE+LUzixvdk-qu0Q-zbFvatX2=KA@mail.gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-25T18:07:54Z","receivedAt":"2016-03-25T18:07:54Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> +test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '\n>\n> Nit: Some of the test titles spell this as \"rebase.autostash\" while\n> others use \"rebase.autoStash\".\n\nThat's a mistake. All test titles must spell \"rebase.autoStash\".\n\n>> +test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '\n>\n> The title says that this is testing with rebase.autoStash unset,\n> however, the test itself doesn't take any action to ensure that it is\n> indeed unset.\n\nActually test_config unset the config variable once the test is complete.\nThus I felt that test_unconfig might not be needed.\n\n>As with the two above tests which explicitly set\n> rebase.autoStash, this test should explicitly unset rebase.autoStash\n> to ensure consistent results even if some future change somehow\n> pollutes the configuration globally. Therefore:\n>\n>     test_unconfig rebase.autostash &&\n>\n\nBut considering this point, I'm convinced that indeed test_unconfig\nshould have been used.\n\n>> +       git reset --hard before-rebase &&\n>> +       echo dirty >new_file &&\n>> +       git add new_file &&\n>> +       git pull --rebase --autostash . copy &&\n>> +       test_cmp_rev HEAD^ copy &&\n>> +       test \"$(cat new_file)\" = dirty &&\n>> +       test \"$(cat file)\" = \"modified again\"\n>> +'\n>\n> With the addition of these three new tests, aside from the\n> introductory 'test_{un}config', this exact sequence of commands is now\n> repeated four times in the script. Such repetition suggests that the\n> common code should be moved to a function. For instance:\n>\n>     test_rebase_autostash () {\n>         git reset --hard before-rebase &&\n>         echo dirty >new_file &&\n>         git add new_file &&\n>         git pull --rebase . copy &&\n>         test_cmp_rev HEAD^ copy &&\n>         test \"$(cat new_file)\" = dirty &&\n>         test \"$(cat file)\" = \"modified again\"\n>     }\n>\n> And, a caller would look like this:\n>\n>     test_expect_success 'pull ... rebase.autostash=true' '\n>         test_config rebase.autostash true &&\n>         test_rebase_autostash\n>     '\n>\n> Of course, you'd also update the original test, from which this code\n> was copied, to also call the new function. Factoring out the common\n> code into a function should probably be done as a separate preparatory\n> patch.\n>\n> This suggestion isn't mandatory and doesn't demand a re-roll, but, if\n> you're feeling ambitious, it would make the code easier to digest and\n> review.\n\nNice. This will increase fluency of the code and also lead to significant\nreduction in number of new lines introduced by this patch.\n\n>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '\n>> +       test_config rebase.autostash true &&\n>> +       git reset --hard before-rebase &&\n>> +       echo dirty >new_file &&\n>> +       git add new_file &&\n>> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n>> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n>\n> I don't care strongly, but many tests consider test_must_fail() alone\n> sufficient to verify proper behavior and don't bother being more exact\n> by checking the precise error message (since error messages sometimes\n> get refined, thus requiring adjustments to the tests). If you do\n\nMain reason to use test_i18ngrep here and check for this specific\nerror is that in future if some developer make changes which might\ntrigger git-pull not to die at die_on_unclean_work_tree() check (if\nwork tree is dirty) but leads git-pull to die somewhere else then\nbasically he/she will not understand the bug introduced by him/her as\ntest \"pull --rebase --no-autostash & rebase.autostash=true\" might pass.\ntest_i18ngrep will make sure that this does not happen.\n\n> retain the error message check, it's often sufficient to check for\n> just a fragment of the error string rather than the full message. For\n> instance, it might be fine to grep merely for \"uncommitted changes\".\n\nYes, that will work too as no other error messages for git-pull contain these\nwords.\n\n>> +'\n>> +\n>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '\n>> +       test_config rebase.autostash false &&\n>> +       git reset --hard before-rebase &&\n>> +       echo dirty >new_file &&\n>> +       git add new_file &&\n>> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n>> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n>> +'\n>> +\n>> +test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n>\n> Same comment as above:\n>\n>     test_unconfig rebase.autostash &&\n>\n>> +       git reset --hard before-rebase &&\n>> +       echo dirty >new_file &&\n>> +       git add new_file &&\n>> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&\n>> +       test_i18ngrep \"Cannot pull with rebase: Your index contains uncommitted changes.\" err\n>> +'\n>\n> Same comment as above about the common code shared by these three new\n> test: moving it to a function is suggested.\n>\n>> +test_expect_success 'pull --autostash (without --rebase) should error out' '\n>> +       test_must_fail git pull --autostash . copy 2>actual &&\n>> +       echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n>> +       test_i18ncmp actual expect\n>\n> Same comment as above about checking the exact error message (vs. just\n> trusting test_must_fail).\n>\n> Also, you mentioned in your cover letter that you couldn't use\n> test_i18ngrep because grep was mistaking \"--[no-]autostash\" in the\n> above expression as a command-line option. If you were using the exact\n> string as above as an argument to test_i18ngrep, then it is more\n> likely that the problem was that grep was seeing \"[no-]\" as a\n> character class rather than as a literal pattern to match. You could\n> get around this either by escaping the [ and ] with a backslash (\\) or\n> by passing -F to test_i18ngrep.\n>\n> Alternately, as mentioned above, just grep for a fragment of the error\n> message, such as \"only valid with --rebase\", rather than the full\n> diagnostic.\n>\n>> +'\n>> +\n>> +test_expect_success 'pull --no-autostash (without --rebase) should error out' '\n>> +       test_must_fail git pull --no-autostash . copy 2>actual &&\n>> +       echo \"fatal: --[no-]autostash option is only valid with --rebase.\" >expect &&\n>> +       test_i18ncmp actual expect\n>> +'\n>\n> Same comment as above about code common to these two tests. However,\n> in this case, it might be easier simply to use a 'for' loop rather\n> than a function:\n>\n>     for i in --autostash --no-autostash\n>     do\n>         test_expect_success \"pull $i (without --rebase) is illegal\" \"\n>            test_must_fail git pull $i . copy 2>actual &&\n>            test_i18ngrep 'only valid with --rebase' actual\n>         \"\n>     done\n>\n> Take special note of how use of double (\") and single (') quotes\n> differ in this case from other tests since $i needs to be interpolated\n> into the test body.\n\nI agree with all of these comments. I will introduce two new function to\nreduce the code and the above mention loop. Also the work on Matthieu's\ncomment.\n\nI feel that most of your comments are necessary and should be there in\nthe next patch. But I have a doubt regarding the next patch. As Junio has\nmerged v10 of current series in next branch (as noticed from his mail),\nsending a new patch should be based on the current patch (i.e. on next\nbranch) or master branch (i.e. continuing with this series)?\n\nThanks,\nMehul\n"},{"id":"281849","messageId":"CA+DCAeTNv-2RkbGo+ciKP_bfCvThKjGAsJEr=xuBYBFgrTvGtg@mail.gmail.com","threadId":"41770","inReplyTo":"vpqshzfuduv.fsf@anie.imag.fr","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-25T18:10:04Z","receivedAt":"2016-03-25T18:10:04Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 25, 2016 at 2:35 PM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Mehul Jain <mehul.jain2029@gmail.com> writes:\n>\n>> +--autostash::\n>> +--no-autostash::\n>> +     Before starting rebase, stash local modifications away (see\n>> +     linkgit:git-stash[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> This does not have to be added to this series (I don't want to break\n> everything at v10 ...), but I think it would be nice to allow \"git pull\n> --autostash\" even without --rebase if pull.rebase=true.\n\nThis is a nice observation. As current patch allow \"git pull --autostash\"\nto be run without --rebase if pull.rebase=true, hence correct\ndocumentation should be something like this\n\n    This option is only valid when \"--rebase\" is used or pull.rebase=true.\n\nBut OTOH users who knows about pull.rebase understands that\npull.rebase=true means \"git pull --rebase ...\" will be executed whenever\n\"git pull ...\" is called, thus for those users it might be easy to deduce that\nneed of \"--rebase\" for validity of \"--autostash\" is not necessary if\npull.rebase=true.\n\nI will correct it in the re-roll.\n\nThanks,\nMehul\n"},{"id":"281860","messageId":"vpq7fgql7zh.fsf@anie.imag.fr","threadId":"41770","inReplyTo":"CA+DCAeTNv-2RkbGo+ciKP_bfCvThKjGAsJEr=xuBYBFgrTvGtg@mail.gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-03-25T18:37:06Z","receivedAt":"2016-03-25T18:37:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mehul Jain <mehul.jain2029@gmail.com> writes:\n\n> On Fri, Mar 25, 2016 at 2:35 PM, Matthieu Moy\n> <Matthieu.Moy@grenoble-inp.fr> wrote:\n>> Mehul Jain <mehul.jain2029@gmail.com> writes:\n>>\n>>> +--autostash::\n>>> +--no-autostash::\n>>> +     Before starting rebase, stash local modifications away (see\n>>> +     linkgit:git-stash[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>> This does not have to be added to this series (I don't want to break\n>> everything at v10 ...), but I think it would be nice to allow \"git pull\n>> --autostash\" even without --rebase if pull.rebase=true.\n>\n> This is a nice observation. As current patch allow \"git pull --autostash\"\n> to be run without --rebase if pull.rebase=true,\n\nOK, I misread the patch assuming that opt_rebase was only reflecting the\noptions, but it is also set by the config:\n\n\tif (opt_rebase < 0)\n\t\topt_rebase = config_get_rebase();\n\n> hence correct documentation should be something like this\n>\n>     This option is only valid when \"--rebase\" is used or pull.rebase=true.\n\n... or just \"when pull is used in rebase mode\", which is shorter and\nstill technically accurate. I don't think you need to be exhaustive in\nthis kind of documentation, the user will notice anyway if he tries to\nuse --autostash in a forbidden situation.\n\n> But OTOH users who knows about pull.rebase understands that\n> pull.rebase=true means \"git pull --rebase ...\" will be executed whenever\n> \"git pull ...\" is called, thus for those users it might be easy to deduce that\n> need of \"--rebase\" for validity of \"--autostash\" is not necessary if\n> pull.rebase=true.\n\nI'd rather have something technically correct.\n\nI think you should also change one of the tests to use pull.resbase=true\nso that this behavior is properly tested.\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"281869","messageId":"CA+DCAeQZjH+vhGYc3PSSt+mgtVi=nJbjbpMBBbTuX-eL9diE9w@mail.gmail.com","threadId":"41770","inReplyTo":"vpq7fgql7zh.fsf@anie.imag.fr","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-25T19:07:15Z","receivedAt":"2016-03-25T19:07:15Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Sat, Mar 26, 2016 at 12:07 AM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> I think you should also change one of the tests to use pull.resbase=true\n> so that this behavior is properly tested.\n\nSure. I will add this test in the re-roll.\n\nThanks,\nMehul\n"},{"id":"281881","messageId":"CAPig+cT=UZdueU+sRa1K637nb6FVYhR2z=-SrUsJKnoG+-+Odw@mail.gmail.com","threadId":"41770","inReplyTo":"CA+DCAeRbD3S5Ltse3A6vBcvhKwh9t5av=Fnz98fD2ES5pbAN=Q@mail.gmail.com","subject":"Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T22:29:43Z","receivedAt":"2016-03-25T22:29:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 2:07 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\n> On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> Nit: Some of the test titles spell this as \"rebase.autostash\" while\n>> others use \"rebase.autoStash\".\n>> [...]\n>> The title says that this is testing with rebase.autoStash unset,\n>> however, the test itself doesn't take any action to ensure that it is\n>> indeed unset. As with the two above tests which explicitly set\n>> rebase.autoStash, this test should explicitly unset rebase.autoStash\n>> to ensure consistent results even if some future change somehow\n>> pollutes the configuration globally. Therefore:\n>> [...]\n>> With the addition of these three new tests, aside from the\n>> introductory 'test_{un}config', this exact sequence of commands is now\n>> repeated four times in the script. Such repetition suggests that the\n>> common code should be moved to a function. For instance:\n>\n> I agree with all of these comments. I will introduce two new function to\n> reduce the code and the above mention loop. Also the work on Matthieu's\n> comment.\n>\n> I feel that most of your comments are necessary and should be there in\n> the next patch. But I have a doubt regarding the next patch. As Junio has\n> merged v10 of current series in next branch (as noticed from his mail),\n> sending a new patch should be based on the current patch (i.e. on next\n> branch) or master branch (i.e. continuing with this series)?\n\nI hadn't noticed that v10 was already in 'next'. In this case, the\nsuggested changes should be a new patch series which makes incremental\nchanges to what is already in 'next'. Be sure to mention in the cover\nletter that the new series should be applied atop\nmj/pull-rebase-autostash.\n"}]}