{"thread":{"id":"41728","subject":"[PATCH v9 1/2] git-pull.c: introduce git_pull_config()","startedAt":"2016-03-17T16:49:29Z","lastAt":"2016-03-20T02:14:08Z","messageCount":8,"participants":["Mehul Jain","Eric Sunshine"],"isPatch":true,"patchVersion":9,"patchTotal":2},"messages":[{"id":"281053","messageId":"1458233370-8343-1-git-send-email-mehul.jain2029@gmail.com","threadId":"41728","inReplyTo":null,"subject":"[PATCH v9 1/2] git-pull.c: introduce git_pull_config()","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-17T16:49:29Z","receivedAt":"2016-03-17T16:49:29Z","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>\nSigned-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n---\nprevious patches: http://thread.gmane.org/gmane.comp.version-control.git/287709\n builtin/pull.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 10eff03..72f4475 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,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 (!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":"281054","messageId":"1458233370-8343-2-git-send-email-mehul.jain2029@gmail.com","threadId":"41728","inReplyTo":"1458233370-8343-1-git-send-email-mehul.jain2029@gmail.com","subject":"[PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-17T16:49:30Z","receivedAt":"2016-03-17T16:49:30Z","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---\nprevious patches: http://thread.gmane.org/gmane.comp.version-control.git/287709\n\nChanges:\n\t* Modified documentation\n\t* \"git pull --[no-]autostash\" case is handled bit early then before\n\n Documentation/git-pull.txt |  9 +++++++++\n builtin/pull.c             | 12 +++++++++++-\n t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+), 1 deletion(-)\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 72f4475..671179b 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@@ -846,11 +850,17 @@ 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\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))\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":"281107","messageId":"CAPig+cQHbGAB=XCVWtC1MuubWUFc0tT-+ZGD+hW18whJP_U0_Q@mail.gmail.com","threadId":"41728","inReplyTo":"1458233370-8343-2-git-send-email-mehul.jain2029@gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T04:24:13Z","receivedAt":"2016-03-18T04:24:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 17, 2016 at 12:49 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>\n> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>\n> ---\n> previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709\n>\n> Changes:\n>         * Modified documentation\n\nThis would be more helpful for reviewers if it went into a bit more\ndetail; \"modified\" alone doesn't say much. For instance, \"... to keep\nthe description of --autostash and --no-autostash together rather than\nsplitting them apart with a tangential comment\" or something.\n\n>         * \"git pull --[no-]autostash\" case is handled bit early then before\n\nLikewise, explaining why it's handled a bit earlier would help\nreviewers. For instance, \"... since getting the error handling case\nout of the way early makes the following code easier to reason about\"\nor something.\n\nSince this is now a patch series rather than a single patch, another\nway to help reviewers is to use a cover letter (see git-format-patch\n--cover-letter) where you'd explain the changes, and, importantly,\ninclude an interdiff between the previous and current versions.\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> @@ -86,6 +86,7 @@ static char *opt_commit;\n> +static int opt_autostash = -1;\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\" : \"--no-autostash\");\n\nAt this point, we know that opt_autostash can't be -1 (thus\nincorrectly triggering use of --autostash) because the conditional in\ncmd_pull() set it to the value of config_autostash (either 0 or 1) if\nthe user did not specify it on the command-line. Okay. Makes sense.\n\nWould an assert(opt_autostash != -1) to document this be desirable? (I\ndon't feel strongly about it, and it's certainly not worth a re-roll.)\n\n>         argv_array_push(&args, \"--onto\");\n>         argv_array_push(&args, sha1_to_hex(merge_head));\n> @@ -846,11 +850,17 @@ int cmd_pull(int argc, const char **argv, const char *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>                 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> +                       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> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 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>         test \"$(cat new_file)\" = dirty &&\n>         test \"$(cat file)\" = \"modified again\"\n>  '\n> +test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n\nWhy do titles of some of the new test titles have a \":\" after \"rebase\"\nwhile other don't?\n\nAlso, how about normalizing the titles so that the reader knows in\nwhich tests rebase.autostash is 'true' and in which it is 'false'?\nPresently, it's difficult to decipher what's being tested based only\non the titles.\n\nFinally, shouldn't you also be testing --autostash and --no-autostash\nwhen rebase.autostash is not set?\n\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 works with rebase.autostash set true' '\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: --no-autostash overrides rebase.autostash' '\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> +\n> +test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set 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"},{"id":"281110","messageId":"CAPig+cQg2gYbr26X6PPQ6bobkxbufSSNtA1T94kdyEKQwHZXBw@mail.gmail.com","threadId":"41728","inReplyTo":"CAPig+cQHbGAB=XCVWtC1MuubWUFc0tT-+ZGD+hW18whJP_U0_Q@mail.gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-18T04:39:54Z","receivedAt":"2016-03-18T04:39:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\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\" : \"--no-autostash\");\n>\n> At this point, we know that opt_autostash can't be -1 (thus\n> incorrectly triggering use of --autostash) because the conditional in\n> cmd_pull() set it to the value of config_autostash (either 0 or 1) if\n> the user did not specify it on the command-line. Okay. Makes sense.\n\nActually, this is going to pass --autostash or --no-autostash to\ngit-rebase unconditionally won't it? This seems kind of undesirable\ndue to the unnecessarily tight coupling it creates between the two\ncommands. I wasn't paying close attention to the earlier discussion,\nbut wasn't the idea that you should pass one of these two options\nalong to git-rebase only if the user explicitly asked to do by saying\nso on the command line?\n\nIn other words:\n\n* invoke \"git-rebase --autostash\" only if the user typed \"git pull\n--rebase --autostash\"\n\n* invoke \"git-rebase --no-autostash\" only if the user typed \"git pull\n--rebase --no-autostash\"\n\n* invoke \"git rebase\" if the user typed bare \"git pull --rebase\"\n"},{"id":"281147","messageId":"CA+DCAeRe4ak1Ni7CADpJdd-=+dvKTTepHY6dYNfQV3ADpz5f6Q@mail.gmail.com","threadId":"41728","inReplyTo":"CAPig+cQHbGAB=XCVWtC1MuubWUFc0tT-+ZGD+hW18whJP_U0_Q@mail.gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-18T15:17:32Z","receivedAt":"2016-03-18T15:17:32Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Since this is now a patch series rather than a single patch, another\n> way to help reviewers is to use a cover letter (see git-format-patch\n> --cover-letter) where you'd explain the changes, and, importantly,\n> include an interdiff between the previous and current versions.\n\nPoint noted for the future patches.\n\n>> +test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n>\n> Why do titles of some of the new test titles have a \":\" after \"rebase\"\n> while other don't?\n>\n> Also, how about normalizing the titles so that the reader knows in\n> which tests rebase.autostash is 'true' and in which it is 'false'?\n> Presently, it's difficult to decipher what's being tested based only\n> on the titles.\n\nIf it's so then how about the tests titles to be the following:\n\n* pull --rebase: --autostash works with rebase.autoStash set true\n\n* pull --rebase: --autostash works with rebase.autoStash set false\n\n* pull --rebase: --no-autostash works with rebase.autoStash set true\n\n* pull --rebase: --no-autostash works with rebase.autoStash set false\n\nEarlier I tried to keep it as less verbose as possible (and probably\nmade it hard to decipher). Does the above titles seems short and\ninformative to you? If so then I will use them instead of earlier ones.\n\n> Finally, shouldn't you also be testing --autostash and --no-autostash\n> when rebase.autostash is not set?\n\nIf rebase.autoStash is not set then config.autostash will remain zero\nthrough out the process. What I want to point out is that rebase.autoStash\n, if not set, is equivalent to being set false. So adding tests regarding\n\"--[no-]autostash with rebase.autoStash unset\" seems equivalent to tests\n\" pull --rebase: --autostash works with rebase.autoStash set false\" and\n\"pull --rebase: --no-autostash works with rebase.autoStash set false\".\n\nThanks,\nMehul\n"},{"id":"281148","messageId":"CA+DCAeTnvEZjcGaS=Ed95bGYHRqV-svtTY-hX8gEbXstbkYe5A@mail.gmail.com","threadId":"41728","inReplyTo":"CAPig+cQg2gYbr26X6PPQ6bobkxbufSSNtA1T94kdyEKQwHZXBw@mail.gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Mehul Jain","fromEmail":"mehul.jain2029@gmail.com","sentAt":"2016-03-18T15:17:42Z","receivedAt":"2016-03-18T15:17:42Z","isPatch":true,"sender":{"key":"mehul.jain2029@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14936539?v=4"},"body":"On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\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\" : \"--no-autostash\");\n>>\n>> At this point, we know that opt_autostash can't be -1 (thus\n>> incorrectly triggering use of --autostash) because the conditional in\n>> cmd_pull() set it to the value of config_autostash (either 0 or 1) if\n>> the user did not specify it on the command-line. Okay. Makes sense.\n>\n> Actually, this is going to pass --autostash or --no-autostash to\n> git-rebase unconditionally won't it? This seems kind of undesirable\n> due to the unnecessarily tight coupling it creates between the two\n> commands. I wasn't paying close attention to the earlier discussion,\n> but wasn't the idea that you should pass one of these two options\n> along to git-rebase only if the user explicitly asked to do by saying\n> so on the command line?\n\nThis is interesting. I checked out git-rebase.sh and found that it reads\nrebase.autoStash if nothing is specified by user. So if user is not\nspecifying anything about stashing then it is the job of git-rebase\nto decide whether or not to do stashing by reading rebase.autoStash.\n\nSimilarly if user doesn't specify the --[no-]autostash option to git-pull\nthen neither of --autostash and --no-autstash should be passed to the\ngit-rebase as it will decide on his own about what needs to be done.\nAgreed. I made a unnecessary tight coupling between git-pull and\ngit-rebase. Instead of that the following changes can be done to\nremove it.\n\n...\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+       if (opt_autostash == 0)\n+               argv_array_push(&args, \"--no-autostash\");\n+       else if (opt_autostash == 1)\n+               argv_array_push(&args, \"--autostash\");\n\n...\n\n...\n        if (opt_rebase) {\n+               int autostash = config_autostash;\n                if (is_null_sha1(orig_head) && !is_cache_unborn())\n                        die(_(\"... with changes added to the index.\"));\n\n-               if (opt_autostash == -1)\n-                       opt_autostash = config_autostash;\n+               if (opt_autostash != -1)\n+                       autostash = opt_autostash;\n\n-               if (!opt_autostash)\n+               if (!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...\n\nNote that the above changes are suggest with respect to my patch, not\nthe current\ncode base.\n\nThis way there's no need to remove \"autostash\" from the current code\nbase and instead use it to write a much cleaner patch.  Something like\nthis (this is w.r.t. current code base)\n\n...\n        if (opt_gpg_sign)\n                argv_array_push(&args, opt_gpg_sign);\n+        if (opt_autostash == 0)\n+               argv_array_push(&args, \"--no-autostash\");\n+       else if (opt_autostash == 1)\n+               argv_array_push(&args, \"--autostash\");\n\n...\n\n...\n\nif (opt_rebase) {\n                 int autostash = config_autostash;\n+               if (opt_autostash != -1)\n+                       autostash = opt_autostash;\n\n                if (is_null_sha1(orig_head) && !is_cache_unborn())\n                        die(_(\"... with changes added to the index.\"));\n\n                if (!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...\n\n\nWhat are your views on this? Also this way I will not touch changes\nintroduced in patch 1/2.\n\n> In other words:\n>\n> * invoke \"git-rebase --autostash\" only if the user typed \"git pull\n> --rebase --autostash\"\n>\n> * invoke \"git-rebase --no-autostash\" only if the user typed \"git pull\n> --rebase --no-autostash\"\n>\n> * invoke \"git rebase\" if the user typed bare \"git pull --rebase\"\n\n\nThanks,\nMehul\n"},{"id":"281251","messageId":"CAPig+cQOHAWLnDi0LmfJH2vb7mv9CABcx=dyU8DezDunZbfpVQ@mail.gmail.com","threadId":"41728","inReplyTo":"CA+DCAeRe4ak1Ni7CADpJdd-=+dvKTTepHY6dYNfQV3ADpz5f6Q@mail.gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-20T02:01:31Z","receivedAt":"2016-03-20T02:01:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\n> On Fri, Mar 18, 2016 at 9:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> +test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '\n>>\n>> Why do titles of some of the new test titles have a \":\" after \"rebase\"\n>> while other don't?\n>>\n>> Also, how about normalizing the titles so that the reader knows in\n>> which tests rebase.autostash is 'true' and in which it is 'false'?\n>> Presently, it's difficult to decipher what's being tested based only\n>> on the titles.\n>\n> If it's so then how about the tests titles to be the following:\n>\n> * pull --rebase: --autostash works with rebase.autoStash set true\n>\n> * pull --rebase: --autostash works with rebase.autoStash set false\n>\n> * pull --rebase: --no-autostash works with rebase.autoStash set true\n>\n> * pull --rebase: --no-autostash works with rebase.autoStash set false\n>\n> Earlier I tried to keep it as less verbose as possible (and probably\n> made it hard to decipher). Does the above titles seems short and\n> informative to you? If so then I will use them instead of earlier ones.\n\nThose are better. If I was doing it, I'd probably drop the unnecessary\n\":\", \"works with\", and \"set\", so:\n\n    pull --rebase --autostash & rebase.autoStash=true\n    pull --rebase --autostash & rebase.autoStash=false\n    pull --rebase --no-autostash & rebase.autoStash=true\n    pull --rebase --no-autostash & rebase.autoStash=false\n\nor something, but that's a very minor point.\n\n>> Finally, shouldn't you also be testing --autostash and --no-autostash\n>> when rebase.autostash is not set?\n>\n> If rebase.autoStash is not set then config.autostash will remain zero\n> through out the process. What I want to point out is that rebase.autoStash\n> , if not set, is equivalent to being set false. So adding tests regarding\n> \"--[no-]autostash with rebase.autoStash unset\" seems equivalent to tests\n> \" pull --rebase: --autostash works with rebase.autoStash set false\" and\n> \"pull --rebase: --no-autostash works with rebase.autoStash set false\".\n\nYes, but what you've described is how the current *implementation*\nworks, whereas the tests should be checking expected *behavior*. So,\nwhile we both know that under the current implementation, checking:\n\n    pull --rebase --autostash & rebase.autoStash unset\n\nis the same as checking:\n\n    pull --rebase--autostash & rebase.autoStash=false\n\nsome future change to the implementation could (accidentally) break\nthis equivalence, and we want to protect against such breakage by\nchecking behavior, not implementation.\n\n\nAlso, I forgot to mention a couple other missing tests you should add:\n\n    * pull --autostash (without --rebase) should error out\n    * pull --no-autostash (without --rebase) should error out\n"},{"id":"281252","messageId":"CAPig+cQvpGgWzT9rXSx_9TR8fg0TrUf+dSs91iRinbbdwo39cg@mail.gmail.com","threadId":"41728","inReplyTo":"CA+DCAeTnvEZjcGaS=Ed95bGYHRqV-svtTY-hX8gEbXstbkYe5A@mail.gmail.com","subject":"Re: [PATCH v9 2/2] pull --rebase: add --[no-]autostash flag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-20T02:14:08Z","receivedAt":"2016-03-20T02:14:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 11:17 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:\n> On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> Actually, this is going to pass --autostash or --no-autostash to\n>> git-rebase unconditionally won't it? This seems kind of undesirable\n>> due to the unnecessarily tight coupling it creates between the two\n>> commands. I wasn't paying close attention to the earlier discussion,\n>> but wasn't the idea that you should pass one of these two options\n>> along to git-rebase only if the user explicitly asked to do by saying\n>> so on the command line?\n>\n> This is interesting. I checked out git-rebase.sh and found that it reads\n> rebase.autoStash if nothing is specified by user. So if user is not\n> specifying anything about stashing then it is the job of git-rebase\n> to decide whether or not to do stashing by reading rebase.autoStash.\n>\n> Similarly if user doesn't specify the --[no-]autostash option to git-pull\n> then neither of --autostash and --no-autstash should be passed to the\n> git-rebase as it will decide on his own about what needs to be done.\n> Agreed. I made a unnecessary tight coupling between git-pull and\n> git-rebase. Instead of that the following changes can be done to\n> remove it.\n>\n> This way there's no need to remove \"autostash\" from the current code\n> base and instead use it to write a much cleaner patch.  Something like\n> this (this is w.r.t. current code base)\n> [...]\n>\n> What are your views on this?\n\nI think this makes the patches cleaner and the final code nicer, and\nit eliminates the too-tight coupling between the two commands, so it\nseems to be a win overall.\n"}]}