[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

Subsystems: the rest

STALE3724d

24 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH v2 1/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:31

git pull --rebase understands --[no-]autostash flag.

This flag overrides config variable "rebase.autoStash" 
if set (default is false).

When calling "git pull --rebase" with "--autostash",
pull passes the "--autostash" option to rebase, 
which then runs rebase on a dirty worktree.

With "--no-autostash" option, the command will die
if the worktree is dirty, before calling rebase.

Signed-off-by: Mehul Jain <redacted>
---

Thanks to Paul and Matthieu for comments on previous round.
Changes:
 	- --autostash flag added
	- OPT_COLOR_FLAG replaced by OPT_BOOL
	- Default value of opt_rebase changed
	- Few changes in code
	- Commit message improvements
	- Documentation added
	- Few tests removed as suggested by Paul
	- Added test for --autostash flag
All tests passed: https://travis-ci.org/mehul2029/git 

 builtin/pull.c          | 13 ++++++++-----
 t/t5520-pull.sh         | 19 +++++++++++++++++++
 t/t5521-pull-options.sh | 16 ++++++++++++++++
 3 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..60b320e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -85,6 +85,7 @@ static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
+static int opt_autostash = 0;
 static char *opt_verify_signatures;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -146,6 +147,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_BOOL(0,"autostash",&opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
-
+	if(opt_autostash)
+		argv_array_push(&args,"--autostash");
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -813,6 +817,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (!getenv("GIT_REFLOG_ACTION"))
 		set_reflog_message(argc, argv);
 
+	git_config_get_bool("rebase.autostash",&opt_autostash);
+
 	argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);

@@ -835,13 +841,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
-
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (!opt_autostash)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..d80b6cc 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '
 	test modified = "$(git show HEAD:file)"
 '
 
+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set false' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy
+'
+
+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
 	test_config rebase.autostash true &&
 	git reset --hard before-rebase &&

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 18372ca..3930e45 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh

@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
+test_expect_success 'git pull --rebase --autostash' '
+	mkdir clonedrbas &&
+	(cd clonedrbas  && git init &&
+	git pull --rebase --autostash "../parent" >out 2>err &&
+	test -s err &&
+	test_must_be_empty out)
+'
+
+test_expect_success 'git pull --rebase --no-autostash' '
+	mkdir clonedrbnas &&
+	(cd clonedrbnas  && git init &&
+	git pull --rebase --no-autostash "../parent" >out 2>err &&
+	test -s err &&
+	test_must_be_empty out)
+'
+
 test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&

-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 2/2] Documentation/git-pull.txt: teach 'git pull --rebase' the --[no-]autostash option.

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:31

git pull --rebase understands --[no-]autostash flag.

This flag overrides config variable "rebase.autoStash" 
if set (default is false).

When calling "git pull --rebase" with "--autostash",
pull passes the "--autostash" option to rebase, 
which then runs rebase on a dirty worktree.

With "--no-autostash" option, the command will die
if the worktree is dirty, before calling rebase.
	
Signed-off-by: Mehul Jain <redacted>
---
 Documentation/git-pull.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..ce22c52 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,22 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+	Automatically create a temporary stash before the operation
+	begins, and apply it after the operation ends. This means
+	that you can run rebase on a dirty worktree.
++
+This option is only valid when '--rebase' option is used.
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
+--no-autostash::
+	If the '--autostash' option is enabled by default using the
+	configuration variable `rebase.autoStash`, this option can be
+	used to override this setting.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
2.7.1.340.g69eb491.dirty

[PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:36

If rebase.autoStash configuration variable is 
set, there is no way to override it for 
"git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no]autostash
command line flag which overrides the current
value of rebase.autostash, if set. As "git rebase"
understands the --[no]autostash option, it's 
just a matter of passing the option to underlying 
"git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Sorry for a late follow up. I had my mid-semester
examination going on.

Previous patch: $gname287709

Changes:
	* error message is produced when "git pull
	 --[no]autostash" is called.
  	* opt_autostash intialized with -1.

 builtin/pull.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..b338b83 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -85,6 +85,7 @@ static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
+static int opt_autostash = -1;
 static char *opt_verify_signatures;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -146,6 +147,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
-
+	if (opt_autostash)
+		argv_array_push(&args, "--autostash");
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -835,18 +839,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
-
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (opt_autostash == -1)
+			git_config_get_bool("rebase.autostash", &opt_autostash);
+
+		if (opt_autostash == 0 || opt_autostash == -1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
 	}
+	else {
+		/* If --[no-]autostash option is called without --rebase */
+		if (opt_autostash == 0)
+			die(_("--no-autostash option is only valid with --rebase."));
+		else if (opt_autostash == 1)
+			die(_("--autostash option is only valid with --rebase."));
+	}
 
 	if (run_fetch(repo, refspecs))
 		return 1;

-- 
2.7.1.340.g69eb491.dirty

[PATCH v3 2/3] test: add test for --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:36

Signed-off-by: Mehul Jain <redacted>
---
 t/t5520-pull.sh         | 19 +++++++++++++++++++
 t/t5521-pull-options.sh | 16 ++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..f5d1d31 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '
 	test modified = "$(git show HEAD:file)"
 '
 
+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy
+'
+
+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
 	test_config rebase.autostash true &&
 	git reset --hard before-rebase &&

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 18372ca..3930e45 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh

@@ -62,6 +62,22 @@ test_expect_success 'git pull -v --rebase' '
 	test_must_be_empty out)
 '
 
+test_expect_success 'git pull --rebase --autostash' '
+	mkdir clonedrbas &&
+	(cd clonedrbas  && git init &&
+	git pull --rebase --autostash "../parent" >out 2>err &&
+	test -s err &&
+	test_must_be_empty out)
+'
+
+test_expect_success 'git pull --rebase --no-autostash' '
+	mkdir clonedrbnas &&
+	(cd clonedrbnas  && git init &&
+	git pull --rebase --no-autostash "../parent" >out 2>err &&
+	test -s err &&
+	test_must_be_empty out)
+'
+
 test_expect_success 'git pull -v -q' '
 	mkdir clonedvq &&
 	(cd clonedvq && git init &&

-- 
2.7.1.340.g69eb491.dirty

[PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:36

Signed-off-by: Mehul Jain <redacted>
---
 Documentation/git-pull.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..a593972 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Automatically create a temporary stash before the operation
+	begins, and apply it after the operation ends. This means
+	that you can run rebase on a dirty worktree.
++
+This option is only valid when '--rebase' option is used.
++
+The default is --no-autostash, unless rebase.autoStash configuration 
+is set.
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH v3 1/3] pull --rebase: add --[no-]autostash flag

From: Paul Tan <hidden>
Date: 2016-06-15 23:08:37

On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain [off-list ref] wrote:

quoted hunk

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..b338b83 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -85,6 +85,7 @@ static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
+static int opt_autostash = -1;
 static char *opt_verify_signatures;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -146,6 +147,8 @@ static struct option pull_options[] = {
        OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
                N_("abort if fast-forward is not possible"),
                PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+       OPT_BOOL(0, "autostash", &opt_autostash,
+               N_("automatically stash/stash pop before and after rebase")),
        OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
                N_("verify that the named commit has a valid GPG signature"),
                PARSE_OPT_NOARG),

@@ -789,7 +792,8 @@ static int run_rebase(const unsigned char *curr_head,
        argv_array_pushv(&args, opt_strategy_opts.argv);
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
-

Minor nit: but when I wrote the code for run_rebase() I separated the
options for "Shared options" and "Options passed to git-rebase" into
different code block groups from the other code, and I would like it
if it remained that way :-(

+       if (opt_autostash)
+               argv_array_push(&args, "--autostash");

Hmm, interesting. If rebase.autostash=true !opt_autostash, we don't
need to pass --no-autostash to git-rebase because it will only stash
if the worktree is dirty, but a dirty worktree will be caught by
git-pull's die_on_unclean_worktree() anyway.

Still, it may be a problem because the worktree may become dirty
in-between our "worktree is clean" check and when git-rebase is run
(during the git-fetch), and the user may be surprised if git-rebase
attempted to stash in that case.

So we may wish to pass "--no-autostash" to git-rebase as well.

quoted hunk

        argv_array_push(&args, "--onto");
        argv_array_push(&args, sha1_to_hex(merge_head));

@@ -835,18 +839,25 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                hashclr(orig_head);

        if (opt_rebase) {
-               int autostash = 0;
-
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               git_config_get_bool("rebase.autostash", &autostash);
-               if (!autostash)
+               if (opt_autostash == -1)
+                       git_config_get_bool("rebase.autostash", &opt_autostash);
+
+               if (opt_autostash == 0 || opt_autostash == -1)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
        }
+       else {

Git code style puts the else on the same line, not on a new one.

+               /* If --[no-]autostash option is called without --rebase */

Yeah, I agree with Eric that this comment should be dropped,

+               if (opt_autostash == 0)
+                       die(_("--no-autostash option is only valid with --rebase."));
+               else if (opt_autostash == 1)
+                       die(_("--autostash option is only valid with --rebase."));
+       }

and these error messages combined.

        if (run_fetch(repo, refspecs))
                return 1;
--
2.7.1.340.g69eb491.dirty

Regards,
Paul

Re: [PATCH v3 3/3] Documentation/git-pull: document --[no-]autostash option

From: Paul Tan <hidden>
Date: 2016-06-15 23:08:37

On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain [off-list ref] wrote:

quoted hunk

Signed-off-by: Mehul Jain <redacted>
---
 Documentation/git-pull.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..a593972 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
        Override earlier --rebase.

+--autostash::
+--no-autostash::
+       Automatically create a temporary stash before the operation
+       begins, and apply it after the operation ends. This means
+       that you can run rebase on a dirty worktree.
++
+This option is only valid when '--rebase' option is used.
++
+The default is --no-autostash, unless rebase.autoStash configuration

By the way, there is a trailing space on this line.

+is set.
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

--
2.7.1.340.g69eb491.dirty

Regards,
Paul

[PATCH v4] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:38

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---

Previous patches: $gname287709

Changes:
	* --no-autostash is passed to git-rebase (suggested by Paul)
	
	* Error message changed when "git pull --[no-]autostash" is called.

	* If rebase.autoStash is unset and user don't pass --[no-]autostash
	  flag then nothing is passed to git-rebase (i.e. if rebase.autoStash = -1),
	  as it should be left to git-rebase to decide what to do.

 Documentation/git-pull.txt | 15 +++++++++++++++
 builtin/pull.c             | 18 ++++++++++++++----
 t/t5520-pull.sh            | 19 +++++++++++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..06e5ddd 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,21 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Before starting rebase, stash local modifications away (see
+	linkgit:git-stash.txt[1]) if needed, and apply the stash when
+	done.
++
+This option is only valid when '--rebase' is used.
++
+'--no-autostash' is useful to override the 'rebase.autoStash'
+configuration variable (see linkgit:git-config[1]).
++
+[NOTE]
+Use with care: the final stash application after a successful
+rebase might result in non-trivial conflicts.
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..e3f5fbf 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -85,6 +85,7 @@ static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
+static int opt_autostash = -1;
 static char *opt_verify_signatures;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -146,6 +147,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -789,6 +792,10 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	if (opt_autostash == 1)
+		argv_array_push(&args, "--autostash");
+	else if (opt_autostash == 0)
+		argv_array_push(&args, "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -835,17 +842,20 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
-
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (opt_autostash == -1)
+			git_config_get_bool("rebase.autostash", &opt_autostash);
+
+		if (opt_autostash == 0 || opt_autostash == -1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
+	} else {
+		if (opt_autostash != -1)
+			die(_("--[no-]autostash option is only valid with --rebase."));
 	}
 
 	if (run_fetch(repo, refspecs))

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..f5d1d31 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -245,6 +245,25 @@ test_expect_success '--rebase fails with multiple branches' '
 	test modified = "$(git show HEAD:file)"
 '
 
+test_expect_success 'pull --rebase --no-autostash fails with dirty working directory and rebase.autstash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy
+'
+
+test_expect_success 'pull --rebase --autostash succeeds with dirty working directory and rebase.autstash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
 	test_config rebase.autostash true &&
 	git reset --hard before-rebase &&

-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH v4] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:38

On Sat, Mar 5, 2016 at 3:22 PM, Mehul Jain [off-list ref] wrote:

Changes:
        * --no-autostash is passed to git-rebase (suggested by Paul)

        * Error message changed when "git pull --[no-]autostash" is called.

        * If rebase.autoStash is unset and user don't pass --[no-]autostash
          flag then nothing is passed to git-rebase (i.e. if rebase.autoStash = -1),
          as it should be left to git-rebase to decide what to do.

I'm sorry. Here instead of " ... (i.e. if rebase.autoStash = -1), ...
" it should be " ... (i.e. if opt_autostash = -1), ... " .

Thanks,
Mehul

[PATCH v5] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:39

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Also introduce a callback function to read config variables.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Previous patches: $gname287709

Changes: 
* Documentation modified
* Introduced a callback function to read config variables.
* Introduce two new tests
* Modified previous tests because:
	
	if worktree is dirty and --no-autostash is passed then git-pull
	should die before git-fetch is called, hence the error message 
	must be: 
	
	"Cannot pull with rebase: Your index contains uncommitted changes."

	Whereas if git-fetch is invoked and --no-autostash is passed to
	git-rebase then error message is:
	
	"Cannot rebase: Your index contains uncommitted changes."

	This test will prevent_future_changes by other people to break the
	code which may cause git-fetch being called even if worktree is dirty
	with --no-autostash option.

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 34 ++++++++++++++++++++++++++++------
 t/t5520-pull.sh            | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..393ac7d 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Before starting rebase, stash local modifications away (see
+	linkgit:git-stash.txt[1]) if needed, and apply the stash when
+	done (this option is only valid when '--rebase' is used).
++
+'--no-autostash' is useful to override the 'rebase.autoStash'
+configuration variable (see linkgit:git-config[1]).
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..b25c53d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -85,6 +85,8 @@ static char *opt_squash;
 static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
+static int opt_autostash = -1;
+static int config_autostash = -1;
 static char *opt_verify_signatures;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -146,6 +148,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
 		N_("abort if fast-forward is not possible"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),

@@ -306,6 +310,18 @@ static enum rebase_type config_get_rebase(void)
 }
 
 /**
+ * Read config variables.
+ */
+static int git_pull_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var,"rebase.autostash")) {
+		config_autostash = git_config_bool(var,value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
+
+/**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
 static int has_unstaged_changes(const char *prefix)

@@ -789,6 +805,10 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	if (opt_autostash == 1)
+		argv_array_push(&args, "--autostash");
+	else if (opt_autostash == 0)
+		argv_array_push(&args, "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -823,7 +843,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase();
 
-	git_config(git_default_config, NULL);
+	git_config(git_pull_config, NULL);
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("Pull");

@@ -835,18 +855,20 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
-
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
+
+		if (opt_autostash == 0 || opt_autostash == -1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-	}
+	} else
+		if (opt_autostash != -1)
+			die(_("--[no-]autostash option is only valid with --rebase."));
 
 	if (run_fetch(repo, refspecs))
 		return 1;

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..cc6a481 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -256,6 +256,46 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat file)" = "modified again"
 '
 
+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&

-- 
2.7.1.340.g69eb491.dirty

[PATCH v6 1/2] git-pull.c: introduce git_pull_config()

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:39

git-pull makes a seperate call to git_config_get_bool() to read the value
of "rebase.autostash", this can be reduced as a call to git_config() is
already there in the code.

Introduce a callback function git_pull_config() to read "rebase.autostash"
along with other variables.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
previous patches: $gname287709

This is a clean-up patch to add --[no-]autostash option to 
"git pull --rebase".
 
 builtin/pull.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..8a318e9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int config_autostash = -1;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
 static char *opt_gpg_sign;

@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)
 
 	return REBASE_FALSE;
 }
+/**
+ * Read config variables.
+ */
+static int git_pull_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "rebase.autostash")) {
+		config_autostash = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
 
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.

@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase();
 
-	git_config(git_default_config, NULL);
+	git_config(git_pull_config, NULL);
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("Pull");

@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
 
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (config_autostash != 1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

-- 
2.7.1.340.g69eb491.dirty

[PATCH v6 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:39

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Previous patches: $gname287709

Changes:
	- Slight change is documentation.

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 16 ++++++++++++++--
 t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..da89be6 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Before starting rebase, stash local modifications away (see
+	linkgit:git-stash.txt[1]) if needed, and apply the stash when
+	done (this option is only valid when "--rebase" is used).
++
+'--no-autostash' is useful to override the 'rebase.autoStash'
+configuration variable (see linkgit:git-config[1]).
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 8a318e9..a01058a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash = -1;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
 		N_("merge strategy to use"),
 		0),

@@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	if (opt_autostash == 1)
+		argv_array_push(&args, "--autostash");
+	else if (opt_autostash == 0)
+		argv_array_push(&args, "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		if (config_autostash != 1)
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
+
+		if (opt_autostash != 1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-	}
+	} else
+		if (opt_autostash != -1)
+			 die(_("--[no-]autostash option is only valid with --rebase."));
 
 	if (run_fetch(repo, refspecs))
 		return 1;

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..85d9bea 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat new_file)" = dirty &&
 	test "$(cat file)" = "modified again"
 '
+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&

-- 
2.7.1.340.g69eb491.dirty

[PATCH v7 1/2] git-pull.c: introduce git_pull_config()

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:40

git-pull makes a seperate call to git_config_get_bool() to read the value
of "rebase.autostash", this can be reduced as a call to git_config() is
already there in the code.

Introduce a callback function git_pull_config() to read "rebase.autostash"
along with other variables.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
previous patches: $gname287709

This is a clean-up patch to add --[no-]autostash option to 
"git pull --rebase".
 
 builtin/pull.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..8a318e9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int config_autostash = -1;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
 static char *opt_gpg_sign;

@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)
 
 	return REBASE_FALSE;
 }
+/**
+ * Read config variables.
+ */
+static int git_pull_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "rebase.autostash")) {
+		config_autostash = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
 
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.

@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase();
 
-	git_config(git_default_config, NULL);
+	git_config(git_pull_config, NULL);
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("Pull");

@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
 
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (config_autostash != 1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

-- 
2.7.1.340.g69eb491.dirty

[PATCH v7 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:40

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Previous patches: $gname287709

Changes:
	- Slight change is documentation.

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 16 ++++++++++++++--
 t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..da89be6 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Before starting rebase, stash local modifications away (see
+	linkgit:git-stash.txt[1]) if needed, and apply the stash when
+	done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 8a318e9..a01058a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash = -1;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
 		N_("merge strategy to use"),
 		0),

@@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	if (opt_autostash == 1)
+		argv_array_push(&args, "--autostash");
+	else if (opt_autostash == 0)
+		argv_array_push(&args, "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		if (config_autostash != 1)
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
+
+		if (opt_autostash != 1)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-	}
+	} else
+		if (opt_autostash != -1)
+			 die(_("--[no-]autostash option is only valid with --rebase."));
 
 	if (run_fetch(repo, refspecs))
 		return 1;

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..85d9bea 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat new_file)" = dirty &&
 	test "$(cat file)" = "modified again"
 '
+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&

-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag

From: Paul Tan <hidden>
Date: 2016-06-15 23:08:42

On Wed, Mar 9, 2016 at 12:18 PM, Mehul Jain [off-list ref] wrote:

quoted hunk

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Previous patches: $gname287709

Changes:
        - Slight change is documentation.

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 16 ++++++++++++++--
 t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..da89be6 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
        Override earlier --rebase.

+--autostash::
+--no-autostash::
+       Before starting rebase, stash local modifications away (see
+       linkgit:git-stash.txt[1]) if needed, and apply the stash when
+       done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 8a318e9..a01058a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash = -1;

Hmm, why can't config_autostash just default to 0?

quoted hunk

 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
        OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
                N_("verify that the named commit has a valid GPG signature"),
                PARSE_OPT_NOARG),
+       OPT_BOOL(0, "autostash", &opt_autostash,
+               N_("automatically stash/stash pop before and after rebase")),
        OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
                N_("merge strategy to use"),
                0),
@@ -801,6 +804,10 @@ static int run_rebase(const unsigned char *curr_head,
        argv_array_pushv(&args, opt_strategy_opts.argv);
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
+       if (opt_autostash == 1)
+               argv_array_push(&args, "--autostash");
+       else if (opt_autostash == 0)
+               argv_array_push(&args, "--no-autostash");

The precise testing for specific values of -1, 0 and 1 throughout the
code makes me uncomfortable. Ordinarily, I would expect a simple

    argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");

Stepping back a bit, the only reason why we introduced opt_autostash =
-1 as a possible value is because we need to detect if
--[no-]autostash is being used with git-merge. I consider that a
kludge, because if git-merge supports --autostash as well (possibly in
the future), then we will not need this -1 value.

So, from that point of view, a -1 value is okay as a workaround, but
kludges, and hence the -1 value, should be gotten rid off as soon as
possible.

quoted hunk

        argv_array_push(&args, "--onto");
        argv_array_push(&args, sha1_to_hex(merge_head));

@@ -851,12 +858,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               if (config_autostash != 1)
+               if (opt_autostash == -1)
+                       opt_autostash = config_autostash;

So, if config_autostash defaults to zero, we can be certain that
opt_autostash will be either true or false.

+
+               if (opt_autostash != 1)

And then we can do if (!opt_autostash) here, instead of making readers
wonder why we are testing for the value "1" specifically.

                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
-       }
+       } else
+               if (opt_autostash != -1)
+                        die(_("--[no-]autostash option is only valid with --rebase."));

        if (run_fetch(repo, refspecs))
                return 1;

Thanks,
Paul

Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:42

On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan [off-list ref] wrote:

quoted

 static int config_autostash = -1;

Hmm, why can't config_autostash just default to 0?

Previously Junio recommended not to explicitly initialize a
static to 0 (or NULL).
http://thread.gmane.org/gmane.comp.version-control.git/287709/focus=287726

Defaulting config_autostash = 0 will work too as you explained.

quoted

+       if (opt_autostash == 1)
+               argv_array_push(&args, "--autostash");
+       else if (opt_autostash == 0)
+               argv_array_push(&args, "--no-autostash");

The precise testing for specific values of -1, 0 and 1 throughout the
code makes me uncomfortable. Ordinarily, I would expect a simple

    argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");

This looks good. I will change accordingly.

Stepping back a bit, the only reason why we introduced opt_autostash =
-1 as a possible value is because we need to detect if
--[no-]autostash is being used with git-merge. I consider that a
kludge, because if git-merge supports --autostash as well (possibly in
the future), then we will not need this -1 value.

So, from that point of view, a -1 value is okay as a workaround, but
kludges, and hence the -1 value, should be gotten rid off as soon as
possible.

That right! But until git-merge doesn't support --autostash, it's necessary to
have opt_autostash = -1 as default.

I wonder if it will be a good thing to add the following line to the
commit message
"Changes needed to be introduced:
Change opt_autostash = 0 as default as soon as git-merge supports
--autostash option, also display no error when "git pull --autostash"
is called."

Possibly it would be better to add gmane link of your review in the
commit message
for further clarification.

Thanks,
Mehul

Re: [PATCH v7 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:43

On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan [off-list ref] wrote:

Stepping back a bit, the only reason why we introduced opt_autostash =
-1 as a possible value is because we need to detect if
--[no-]autostash is being used with git-merge. I consider that a
kludge, because if git-merge supports --autostash as well (possibly in
the future), then we will not need this -1 value.

No, there is one more reason for which opt_autostash = -1 is required.
When user calls "git pull --rebase" then config_autostash value will
be used to perform --[no-]autostash task but if user calls "git pull
--rebase --[no-]autostash" then config_autostash value should not be
read at all as this option is supposed to override config_autostash
value. So if opt_autostash defaults to 0 then how will the code
understand if "--[no-]autostash" flag is passed or not?

As per the current patch, the value opt_autostash = 0 or 1  tells us
that the user has explicitly asked for --no-autostash or --autostash
respectively, and -1 value tells us that user has not specified
anything and thus we should read config_autostash value to perform
--[no-]autostash.

One way to do this was to read rebase.autoStash before parse_options(),
but now  as we have introduced a callback function git_pull_config(),
reading this config variable before parse_option() will now require
calling git_config(git_pull_config, NULL) before parse_option() and
doing opt_autostash = config_autostash there only.This may lead to
some problems (I'm not sure of that), as git_config() reads many other
config variables too.

Thanks,
Mehul

[PATCH 1/2] git-pull.c: introduce git_pull_config()

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:44

git-pull makes a seperate call to git_config_get_bool() to read the value
of "rebase.autostash", this can be reduced as a call to git_config() is
already there in the code.

Introduce a callback function git_pull_config() to read "rebase.autostash"
along with other variables.

Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
Previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709

Change: config_autostash initialized with 0 instead of -1

 builtin/pull.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..43353f9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int config_autostash;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
 static char *opt_gpg_sign;

@@ -304,6 +305,17 @@ static enum rebase_type config_get_rebase(void)
 
 	return REBASE_FALSE;
 }
+/**
+ * Read config variables.
+ */
+static int git_pull_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "rebase.autostash")) {
+		config_autostash = git_config_bool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value, cb);
+}
 
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.

@@ -823,7 +835,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase();
 
-	git_config(git_default_config, NULL);
+	git_config(git_pull_config, NULL);
 
 	if (read_cache_unmerged())
 		die_resolve_conflict("Pull");

@@ -835,13 +847,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		hashclr(orig_head);
 
 	if (opt_rebase) {
-		int autostash = 0;
 
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		git_config_get_bool("rebase.autostash", &autostash);
-		if (!autostash)
+		if (config_autostash)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))

-- 
2.7.1.340.g69eb491.dirty

[PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:44

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

Helped-by: Matthieu Moy [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Paul Tan [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Mehul Jain <redacted>
---
previous patches: http://thread.gmane.org/gmane.comp.version-control.git/287709

 Documentation/git-pull.txt |  9 +++++++++
 builtin/pull.c             | 13 +++++++++++--
 t/t5520-pull.sh            | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index a62a2a6..a070ec9 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
 --no-rebase::
 	Override earlier --rebase.
 
+--autostash::
+--no-autostash::
+	Before starting rebase, stash local modifications away (see
+	linkgit:git-stash.txt[1]) if needed, and apply the stash when
+	done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).
+
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/builtin/pull.c b/builtin/pull.c
index 43353f9..c48e28a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
 		N_("verify that the named commit has a valid GPG signature"),
 		PARSE_OPT_NOARG),
+	OPT_BOOL(0, "autostash", &opt_autostash,
+		N_("automatically stash/stash pop before and after rebase")),
 	OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
 		N_("merge strategy to use"),
 		0),

@@ -801,6 +804,7 @@ static int run_rebase(const unsigned char *curr_head,
 	argv_array_pushv(&args, opt_strategy_opts.argv);
 	if (opt_gpg_sign)
 		argv_array_push(&args, opt_gpg_sign);
+	argv_array_push(&args, opt_autostash ? "--autostash" : "--no-autostash");
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));

@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (is_null_sha1(orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		if (config_autostash)
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
+
+		if (!opt_autostash)
 			die_on_unclean_work_tree(prefix);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-	}
+	} else
+		if (opt_autostash != -1)
+			 die(_("--[no-]autostash option is only valid with --rebase."));
 
 	if (run_fetch(repo, refspecs))
 		return 1;

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c952d5e..85d9bea 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -255,6 +255,45 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat new_file)" = dirty &&
 	test "$(cat file)" = "modified again"
 '
+test_expect_success 'pull --rebase: --autostash overrides rebase.autostash' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase --autostash works with rebase.autostash set true' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull --rebase --autostash . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+'
+
+test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash' '
+	test_config rebase.autostash true &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
+
+test_expect_success 'pull --rebase --no-autostash works with rebase.autostash set false' '
+	test_config rebase.autostash false &&
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
+	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+'
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&

-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:45

On Tue, Mar 15, 2016 at 1:11 PM, Mehul Jain [off-list ref] wrote:

If rebase.autoStash configuration variable is set, there is no way to
override it for "git pull --rebase" from the command line.

Teach "git pull --rebase" the --[no-]autostash command line flag which
overrides the current value of rebase.autoStash, if set. As "git rebase"
understands the --[no-]autostash option, it's just a matter of passing
the option to underlying "git rebase" when "git pull --rebase" is called.

The below comments may or may not be worth a re-roll (you decide)...

quoted hunk

Signed-off-by: Mehul Jain <redacted>
---

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
+--autostash::
+--no-autostash::
+       Before starting rebase, stash local modifications away (see
+       linkgit:git-stash.txt[1]) if needed, and apply the stash when
+       done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).

The last couple sentences seem reversed. It feels odd to have the bit
about --rebase dropped dead in the middle of the description of
--autostash and --no-autostash. I'd have expected to see --autostash
and --no-autostash discussed together, and then, as a follow-on,
mention them being valid only with --rebase.

quoted hunk

diff --git a/builtin/pull.c b/builtin/pull.c

@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               if (config_autostash)
+               if (opt_autostash == -1)

In patch 1/2, this changed from 'if (autostash)' to 'if
(config_autostash)'; it's a bit sad that patch 2/2 then has to touch
the same code to change it yet again, this time to 'if
(opt_autostash)'. Have you tried just keeping the original 'autostash'
variable and modifying its value based upon config_autostash and
opt_autostash instead? (Not saying that this would be better, but
interested in knowing if the result is as clean or cleaner or worse.)

+                       opt_autostash = config_autostash;
+
+               if (!opt_autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
-       }
+       } else
+               if (opt_autostash != -1)
+                        die(_("--[no-]autostash option is only valid with --rebase."));

How about formatting this as a normal 'else if'?

    } else if (opt_autostash != -1)

On the other hand, this error case hanging off the 'rebase'
conditional is somewhat more difficult to reason about than perhaps it
ought to be. It might be easier to see what's going on if you get the
error case out of the way early, and then handle the normal case. That
is, something like this:

    if (!opt_rebase && opt_autostash != -1)
        die(_("... is only valid with --rebase"));

    if (opt_rebase) {
        ...
    }

        if (run_fetch(repo, refspecs))
                return 1;

Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:45

On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine [off-list ref] wrote:

quoted

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt

@@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
+--autostash::
+--no-autostash::
+       Before starting rebase, stash local modifications away (see
+       linkgit:git-stash.txt[1]) if needed, and apply the stash when
+       done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).

The last couple sentences seem reversed. It feels odd to have the bit
about --rebase dropped dead in the middle of the description of
--autostash and --no-autostash. I'd have expected to see --autostash
and --no-autostash discussed together, and then, as a follow-on,
mention them being valid only with --rebase.

So you are suggesting something like this:

--autostash::
--no-autostash::
    Before starting rebase, stash local modifications away (see
    linkgit:git-stash.txt[1]) if needed, and apply the stash when
    done. `--no-autostash` is useful to override the `rebase.autoStash`
    configuration variable (see linkgit:git-config[1]).
+
This option is only valid when "--rebase" is used.

Can be done and it make more sense to talk about the validity of the
option in a seperate line.

quoted

diff --git a/builtin/pull.c b/builtin/pull.c

@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               if (config_autostash)
+               if (opt_autostash == -1)

In patch 1/2, this changed from 'if (autostash)' to 'if
(config_autostash)'; it's a bit sad that patch 2/2 then has to touch
the same code to change it yet again, this time to 'if
(opt_autostash)'. Have you tried just keeping the original 'autostash'
variable and modifying its value based upon config_autostash and
opt_autostash instead? (Not saying that this would be better, but
interested in knowing if the result is as clean or cleaner or worse.)

Yes, I tried that. Things looked something like this:

In patch 1/2
...

-    int autostash = 0;
+    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

-    git_config_get_bool("rebase.autostash", &autostash);
    if (!autostash)
            die_on_unclean_work_tree(prefix);

...

In patch 2/2
...
    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

+    if (opt_autostash != -1)
+        autostash = opt_autostash;

    if (!autostash)
        die_on_unclean_work_tree(prefix);
...

This implementation looks much more cleaner but we are using some
extra space (autostash) to do the task. If everyone is fine with this
trade off then I can re-roll a new patch with this method. Comments please.

quoted

+                       opt_autostash = config_autostash;
+
+               if (!opt_autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
-       }
+       } else
+               if (opt_autostash != -1)
+                        die(_("--[no-]autostash option is only valid with --rebase."));

How about formatting this as a normal 'else if'?

    } else if (opt_autostash != -1)

I thought of it earlier but voted against it as it may reduce the readability of
the code.

On the other hand, this error case hanging off the 'rebase'
conditional is somewhat more difficult to reason about than perhaps it
ought to be. It might be easier to see what's going on if you get the
error case out of the way early, and then handle the normal case. That
is, something like this:

    if (!opt_rebase && opt_autostash != -1)
        die(_("... is only valid with --rebase"));

    if (opt_rebase) {
        ...
    }

This is good. I'll make the changes accordingly.

Thanks for the comments.

Mehul

Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Mehul Jain <hidden>
Date: 2016-06-15 23:08:47

Hello Eric,

I tried out this approach and here's the result.

---

diff --git a/builtin/pull.c b/builtin/pull.c
index b5b0255..0ce007d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
        OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
                N_("verify that the named commit has a valid GPG signature"),
                PARSE_OPT_NOARG),
+       OPT_BOOL(0, "autostash", &opt_autostash,
+               N_("automatically stash/stash pop before and after rebase")),
        OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
                N_("merge strategy to use"),
                0),

@@ -801,6 +804,7 @@ static int run_rebase(const unsigned char *curr_head,
        argv_array_pushv(&args, opt_strategy_opts.argv);
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
+       argv_array_push(&args, opt_autostash ? "--autostash" :

"--no-autostash");

        argv_array_push(&args, "--onto");
        argv_array_push(&args, sha1_to_hex(merge_head));

@@ -846,12 +850,20 @@ int cmd_pull(int argc, const char **argv, const

char *prefix)
        if (get_sha1("HEAD", orig_head))
                hashclr(orig_head);

+       if(!opt_rebase && opt_autostash != -1)
+               die(_("--[no-]autostash option is only valid with --rebase."));
+
        if (opt_rebase) {
                int autostash = config_autostash;

                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes
added to the index."));

+               if (opt_autostash != -1)
+                       autostash = opt_autostash;
+               else
+                       opt_autostash = config_autostash;
+
                if (!autostash)
                        die_on_unclean_work_tree(prefix);
---

This way of implementation looks a bit less clean to me than
the previous one because we are using "opt_autostash" to pass
the "--[no-]autostash"  flag to git-rebase, thus if user does not
specify anything about stashing in command line then  config_autostash
value has to be used ( i.e. opt_autostash = config_autostash).
To do this an "else" case has to be introduced in the code. This
might effect the readability of the code because the reader might
wonder why "opt_autostash" is used to assign value to "autostash"
in one case, and opt_autostash = config_autostash in other case.

But I agree that this way I won't be touching the changes I made
in patch 1/2.

I would like to know your view on above mentioned issue.

Also I made a mistake in patch 1/2 which I will correct in the next
version along with other changes suggested by you.

Thanks,
Mehul

Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:48

On Wed, Mar 16, 2016 at 1:00 AM, Mehul Jain [off-list ref] wrote:

On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine [off-list ref] wrote:

quoted

+--autostash::
+--no-autostash::
+       Before starting rebase, stash local modifications away (see
+       linkgit:git-stash.txt[1]) if needed, and apply the stash when
+       done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).

The last couple sentences seem reversed. It feels odd to have the bit
about --rebase dropped dead in the middle of the description of
--autostash and --no-autostash. I'd have expected to see --autostash
and --no-autostash discussed together, and then, as a follow-on,
mention them being valid only with --rebase.

So you are suggesting something like this:

--autostash::
--no-autostash::
    Before starting rebase, stash local modifications away (see
    linkgit:git-stash.txt[1]) if needed, and apply the stash when
    done. `--no-autostash` is useful to override the `rebase.autoStash`
    configuration variable (see linkgit:git-config[1]).
+
This option is only valid when "--rebase" is used.

Can be done and it make more sense to talk about the validity of the
option in a seperate line.

Yes, like that.

quoted

diff --git a/builtin/pull.c b/builtin/pull.c

@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               if (config_autostash)
+               if (opt_autostash == -1)

In patch 1/2, this changed from 'if (autostash)' to 'if
(config_autostash)'; it's a bit sad that patch 2/2 then has to touch
the same code to change it yet again, this time to 'if
(opt_autostash)'. Have you tried just keeping the original 'autostash'
variable and modifying its value based upon config_autostash and
opt_autostash instead? (Not saying that this would be better, but
interested in knowing if the result is as clean or cleaner or worse.)

Yes, I tried that. Things looked something like this:

In patch 1/2
...

-    int autostash = 0;
+    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

-    git_config_get_bool("rebase.autostash", &autostash);
    if (!autostash)
            die_on_unclean_work_tree(prefix);

The individual diffs look nicer and are less noisy, thus a bit easier to review.

In patch 2/2
...
    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

+    if (opt_autostash != -1)
+        autostash = opt_autostash;

I'd probably have placed this conditional just below the line where
'autostash' is declared so that the logic for computing the value of
'autostash' is all in one place.

    if (!autostash)
        die_on_unclean_work_tree(prefix);
...

This implementation looks much more cleaner but we are using some
extra space (autostash) to do the task. If everyone is fine with this
trade off then I can re-roll a new patch with this method. Comments please.

Using an extra variable isn't a big deal and would be a good idea if
it helped clarify the logic. In this case, the logic isn't
particularly difficult, so I don't feel too strongly about it one way
or the other.

Re: [PATCH 2/2] pull --rebase: add --[no-]autostash flag

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:48

On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain [off-list ref] wrote:

I tried out this approach and here's the result.

+       if(!opt_rebase && opt_autostash != -1)
+               die(_("--[no-]autostash option is only valid with --rebase."));
+
        if (opt_rebase) {
                int autostash = config_autostash;

                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes
added to the index."));

+               if (opt_autostash != -1)
+                       autostash = opt_autostash;
+               else
+                       opt_autostash = config_autostash;
                if (!autostash)
                        die_on_unclean_work_tree(prefix);

This way of implementation looks a bit less clean to me than
the previous one because we are using "opt_autostash" to pass
the "--[no-]autostash"  flag to git-rebase, thus if user does not
specify anything about stashing in command line then  config_autostash
value has to be used ( i.e. opt_autostash = config_autostash).
To do this an "else" case has to be introduced in the code. This
might effect the readability of the code because the reader might
wonder why "opt_autostash" is used to assign value to "autostash"
in one case, and opt_autostash = config_autostash in other case.

That's pretty ugly. Since cmd_pull() is the only caller of
run_rebase(), an alternative would be to pass 'autostash' as an
argument to run_rebase(). However, since run_rebase() is already
accessing other 'opt_foo' globals, it wouldn't make sense to make an
exception of 'autostash' by passing it as an argument. So, in the end,
the original approach is indeed probably cleaner.

Also I made a mistake in patch 1/2 which I will correct in the next
version along with other changes suggested by you.

Which mistake would that be?

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help