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

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

[Mehul Jain]

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 <mehul.jain2029@gmail.com>
---

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.

[Mehul Jain]

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 <mehul.jain2029@gmail.com>
---
 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

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

[Junio C Hamano]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> git pull --rebase understands --[no-]autostash flag.
>
> This flag overrides config variable "rebase.autoStash" 
> if set (default is false).

Is that a statement of a fact?  If so, is it true before this patch
is applied, or after?

Each project has local convention for log messages, and we do too.
A long log message typically start by explaining how the world is,
why that is not desirable, present a description of a possible world
that is better, and then give commands to somebody who is updating
the code telling him what to do to make that better world a reality
(and optionally how).

So perhaps (I am totally making this up; you need to fact check and
adjust):

    If you enable rebase.autoStash option in your repository, there
    is no way to override it for "git pull --rebase" from the
    command line.

    Teach "git pull" a new "--[no-]autostash" option so that a
    rebase.autoStash configuration can be overridden.  As "git
    rebase" already knows "--[no-]autostash" option, it is just the
    matter of passing one when we spawn the command as necessary.

or something.  The first one gives the readers how the current world
works, and why it is not ideal.  The proposed better world in this
case is too simple--the first paragraph complained that "we cannot
do X" and X is something reader would likely to agree is a good
thing to do, so it can be left unsaid that a better world is one in
which X can be done.

> 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.

These two paragraphs are too obvious and probably are better left
unsaid.  Especially the latter--you are changing "pull" and do not
control what "rebase" would do in the future.  It could be that a
better rebase in the future may be able to do its job in a dirty
worktree without doing an autostash.

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>
> 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;

Do not explicitly initialize a static to 0 (or NULL).

>  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)

Style: control keywords are followed by a single SP before the next '('.

> +		argv_array_push(&args,"--autostash");

Style: a single SP after a comma.

How would --no-autostash defeat a configured rebase.autostash with this?

By the way, how would this affect "git pull --autostash" that is run
without "--rebase"?  If this is an option to "git pull", shouldn't
the stashing done even when you are not doing a rebase but making a
merge?

>  	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);
> +

Why is this change even necessary?

>  	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);

I would have expected that

 * a global opt_autostash is initialized to -1 (unspecified);

 * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;

 * existing "rebase.autostash" configuration check inside "git pull"
   code  gets removed;

 * and the code that builds "git rebase" invocation command line
   will do

   	if (opt_autostash < 0)
        	; /* do nothing */
	else if (opt_autostash == 0)
        	argv_array_push(&args, "--no-autostash");
	else
        	argv_array_push(&args, "--autostash");
        	
Then when "git pull --rebase" is run without "--[no-]autostash", the
underlying "git rebase" would be run without that option, and does its
usual thing, including reading rebase.autostash and deciding to do
"git stash".  And when "git pull" is run with "--[no-]autostash",
the underlying "git rebase" would be given the same option, and
would do what it was told to do, ignoring rebase.autostash setting.

So why does "git pull" still need to look at rebase.autostash
configuration after this change?

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

[Mehul Jain]

On Sun, Feb 28, 2016 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> git pull --rebase understands --[no-]autostash flag.
>>
>> This flag overrides config variable "rebase.autoStash"
>> if set (default is false).
>
> Is that a statement of a fact?  If so, is it true before this patch
> is applied, or after?
>
> Each project has local convention for log messages, and we do too.
> A long log message typically start by explaining how the world is,
> why that is not desirable, present a description of a possible world
> that is better, and then give commands to somebody who is updating
> the code telling him what to do to make that better world a reality
> (and optionally how).
>
> So perhaps (I am totally making this up; you need to fact check and
> adjust):
>
>     If you enable rebase.autoStash option in your repository, there
>     is no way to override it for "git pull --rebase" from the
>     command line.
>
>     Teach "git pull" a new "--[no-]autostash" option so that a
>     rebase.autoStash configuration can be overridden.  As "git
>     rebase" already knows "--[no-]autostash" option, it is just the
>     matter of passing one when we spawn the command as necessary.
>
> or something.  The first one gives the readers how the current world
> works, and why it is not ideal.  The proposed better world in this
> case is too simple--the first paragraph complained that "we cannot
> do X" and X is something reader would likely to agree is a good
> thing to do, so it can be left unsaid that a better world is one in
> which X can be done.
>
>> 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.
>
> These two paragraphs are too obvious and probably are better left
> unsaid.  Especially the latter--you are changing "pull" and do not
> control what "rebase" would do in the future.  It could be that a
> better rebase in the future may be able to do its job in a dirty
> worktree without doing an autostash.

OK. I will follow up with a better commit message.

>> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
>> ---
>>
>> 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;
>
> Do not explicitly initialize a static to 0 (or NULL).
>
>>  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)
>
> Style: control keywords are followed by a single SP before the next '('.
>
>> +             argv_array_push(&args,"--autostash");
>
> Style: a single SP after a comma.
>
> How would --no-autostash defeat a configured rebase.autostash with this?
>
> By the way, how would this affect "git pull --autostash" that is run
> without "--rebase"?  If this is an option to "git pull", shouldn't
> the stashing done even when you are not doing a rebase but making a
> merge?

As "git rebase" takes "--[no-]autostash" as an option whereas "git
merge" does not,
hence "--[no-]autostash" option is only valid when "--rebase" option is used in
"git pull".

If user uses "git pull --[no-]autostash" then two possible things can be done:

           * Either "git pull" ignores "--[no-]autostash" and calls
underlying "git merge",
             as merge stashes the untracked files by itself. Thus
ignoring --no-autostash
             flag given by user.

           * Or "git pull" dies with the following error:

                    "--[no-]autostash is only valid when --rebase is used.
                     Example: git pull --rebase --[no-]autostash"

I suggest that the latter option should be used in this case as user should know
that stashing is performed by "git merge" anyway and "--no-autostash"
flag is not
a way to tell "git merge" to not to do stashing. Also this error will
fit perfectly with the
documentation of "--[no-]autostash" option given in the [PATCH v2 2/2].

Please suggest what are your views on this.

>>       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);
>> +
>
> Why is this change even necessary?
>
>>       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);
>
> I would have expected that
>
>  * a global opt_autostash is initialized to -1 (unspecified);
>
>  * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;
>
>  * existing "rebase.autostash" configuration check inside "git pull"
>    code  gets removed;
>
>  * and the code that builds "git rebase" invocation command line
>    will do
>
>         if (opt_autostash < 0)
>                 ; /* do nothing */
>         else if (opt_autostash == 0)
>                 argv_array_push(&args, "--no-autostash");
>         else
>                 argv_array_push(&args, "--autostash");
>
> Then when "git pull --rebase" is run without "--[no-]autostash", the
> underlying "git rebase" would be run without that option, and does its
> usual thing, including reading rebase.autostash and deciding to do
> "git stash".  And when "git pull" is run with "--[no-]autostash",
> the underlying "git rebase" would be given the same option, and
> would do what it was told to do, ignoring rebase.autostash setting.
>
> So why does "git pull" still need to look at rebase.autostash
> configuration after this change?

I agree with your point that future rebase might be able to do it's job on
dirty working tree without  autostash option, so it's always better to
let rebase
check for dirty working tree and do the task of looking at rebase.autostash.

Thanks for a thorough review.

Cheers,
Mehul Jain

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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> If user uses "git pull --[no-]autostash" then two possible things can be done:
>
>            * Either "git pull" ignores "--[no-]autostash" and calls
>              underlying "git merge",
>              as merge stashes the untracked files by itself. Thus
>              ignoring --no-autostash
>              flag given by user.
>
>            * Or "git pull" dies with the following error:
>
>                     "--[no-]autostash is only valid when --rebase is used.
>                      Example: git pull --rebase --[no-]autostash"
>
> I suggest that the latter option should be used in this case as user should know
> that stashing is performed by "git merge" anyway

Not exactly. "git merge" doesn't do a stash, but it accepts to run if
local changes do not touch the same files as the merge. So, --autostash
is usually not needed for merge.

But I agree that erroring out is better. Silently accepting the option
and doing nothing with it would be confusing for users. This would mean
that "git pull --autostash" would work, but still sometimes show the error:

  error: Your local changes to the following files would be overwritten by merge:

The situation is different for command-line options and for config
variables: the config variable wasn't explicitly specified for each run
of "git pull", hence it's acceptable to ignore its value when we have
nothing to do with it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Paul Tan]

Hi Junio,

On Sun, Feb 28, 2016 at 3:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>> @@ -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);
>
> I would have expected that
>
>  * a global opt_autostash is initialized to -1 (unspecified);
>
>  * opt_bool() would flip it to either 0 or 1 with --[no-]autostash;
>
>  * existing "rebase.autostash" configuration check inside "git pull"
>    code  gets removed;

Removing the "rebase.autostash" configuration check would bring back
the problem which 53c76dc (pull: allow dirty tree when
rebase.autostash enabled, 2015-07-04) fixed.

>  * and the code that builds "git rebase" invocation command line
>    will do
>
>         if (opt_autostash < 0)
>                 ; /* do nothing */
>         else if (opt_autostash == 0)
>                 argv_array_push(&args, "--no-autostash");
>         else
>                 argv_array_push(&args, "--autostash");
>
> Then when "git pull --rebase" is run without "--[no-]autostash", the
> underlying "git rebase" would be run without that option, and does its
> usual thing, including reading rebase.autostash and deciding to do
> "git stash".  And when "git pull" is run with "--[no-]autostash",
> the underlying "git rebase" would be given the same option, and
> would do what it was told to do, ignoring rebase.autostash setting.
>
> So why does "git pull" still need to look at rebase.autostash
> configuration after this change?

Ultimately, git-pull needs to be aware of whether autostash is active
or not (and this means rebase.autostash needs to be looked at as well)
because if autostash is disabled, git-pull needs to perform the
"worktree is clean" check. And this "worktree is clean" check needs to
be done *before* git-fetch and git-rebase is run.

See f9189cf (pull --rebase: exit early when the working directory is
dirty, 2008-05-21).

Regards,
Paul

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Ultimately, git-pull needs to be aware of whether autostash is active
> or not (and this means rebase.autostash needs to be looked at as well)
> because if autostash is disabled, git-pull needs to perform the
> "worktree is clean" check. And this "worktree is clean" check needs to
> be done *before* git-fetch and git-rebase is run.

Ahh, right. I forgot about that discussion.

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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Mehul Jain]

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 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

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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  t/t5520-pull.sh         | 19 +++++++++++++++++++
>  t/t5521-pull-options.sh | 16 ++++++++++++++++

There's no need to split code/test/doc into separate patches, except if
your patches are getting really huge. As a reviewer, I like having tests
and doc in the same patch because they describe the intention of the
programmer.

We try to have a history where each commit is equally good, and with
your version there are two commits where --autostash exists and is
undocumented (which is not catastrophic, though).

> +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"
> +'

Sounds good.

> --- 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)
> +'

Not sure these tests are needed if you have the ones in t/t5520-pull.sh.
More tests means more time to run so testing twice the same thing has a
cost.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Mehul Jain]

On Thu, Mar 3, 2016 at 11:01 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> There's no need to split code/test/doc into separate patches, except if
> your patches are getting really huge. As a reviewer, I like having tests
> and doc in the same patch because they describe the intention of the
> programmer.
>
> We try to have a history where each commit is equally good, and with
> your version there are two commits where --autostash exists and is
> undocumented (which is not catastrophic, though).

Alright, I will make a single patch for the next version.

>> --- 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)
>> +'
>
> Not sure these tests are needed if you have the ones in t/t5520-pull.sh.
> More tests means more time to run so testing twice the same thing has a
> cost.

I agree with you. This test may not be needed as t/t5520-pull.sh already test
this option.

Thanks,
Mehul

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

[Mehul Jain]

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 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 3/3] Documentation/git-pull: document --[no-]autostash option

[Junio C Hamano]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  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.
> +

Should this entry this verbose?

 - Is there a non-temporary stash?

 - I think "This means that ..." is totally unnecessary.

 - It probably makes sense to have "This option is only valid..." as
   a separate second paragraph as you did.

 - "The default is..." is misleading.  Even if rebase.autostash is
   set to false, we won't autostash, but that is different from the
   default being "--no-autostash".

   Think of "--[no-]autostash" option as *ONE* way to affect the
   auto-stashing behaviour, and treat "options" and "behaviours" two
   different things.

There is no default "option" for this.  It is that "autostash"
behaviour defaults to what is given to rebase.autostash if
exists, and can be explicitly set by --[no-]autostash if given.

But that is the norm for any configuration and option that overrides
the configuration, so it probably is a better use of the ink to say
something like this perhaps?

        --autostash::
        --no-autostash::
                Before starting "pull --rebase", create a stash to save
                local modifications, 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]).

By the way, some other patches in this series say --noautostash
without a dash after --no, which you would want to correct.

Thanks.

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

[Mehul Jain]

On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Should this entry this verbose?
>
>  - Is there a non-temporary stash?
>
>  - I think "This means that ..." is totally unnecessary.
>
>  - It probably makes sense to have "This option is only valid..." as
>    a separate second paragraph as you did.
>
>  - "The default is..." is misleading.  Even if rebase.autostash is
>    set to false, we won't autostash, but that is different from the
>    default being "--no-autostash".
>
>    Think of "--[no-]autostash" option as *ONE* way to affect the
>    auto-stashing behaviour, and treat "options" and "behaviours" two
>    different things.
>
> There is no default "option" for this.  It is that "autostash"
> behaviour defaults to what is given to rebase.autostash if
> exists, and can be explicitly set by --[no-]autostash if given.
>
> But that is the norm for any configuration and option that overrides
> the configuration, so it probably is a better use of the ink to say
> something like this perhaps?
>
>         --autostash::
>         --no-autostash::
>                 Before starting "pull --rebase", create a stash to save
>                 local modifications, and apply the stash when done (this
>                 option is only valid when "--rebase" is used).

OK, but according to the definition of --[no-]autostash given in
git-rebase documentation (https://git-scm.com/docs/git-rebase),
temporary stash is created. So shouldn't we be consistent with this
definition by writing "... , create a temporary stash ..." instead of
"... , create a stash ...".

>         +
>         '--no-autostash' is useful to override the 'rebase.autoStash'
>         configuration variable (see linkgit:git-config[1]).

Thanks,
Mehul

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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> On Thu, Mar 3, 2016 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Should this entry this verbose?
>>
>>  - Is there a non-temporary stash?
>>
>>  - I think "This means that ..." is totally unnecessary.
>>
>>  - It probably makes sense to have "This option is only valid..." as
>>    a separate second paragraph as you did.
>>
>>  - "The default is..." is misleading.  Even if rebase.autostash is
>>    set to false, we won't autostash, but that is different from the
>>    default being "--no-autostash".
>>
>>    Think of "--[no-]autostash" option as *ONE* way to affect the
>>    auto-stashing behaviour, and treat "options" and "behaviours" two
>>    different things.
>>
>> There is no default "option" for this.  It is that "autostash"
>> behaviour defaults to what is given to rebase.autostash if
>> exists, and can be explicitly set by --[no-]autostash if given.
>>
>> But that is the norm for any configuration and option that overrides
>> the configuration, so it probably is a better use of the ink to say
>> something like this perhaps?
>>
>>         --autostash::
>>         --no-autostash::
>>                 Before starting "pull --rebase", create a stash to save
>>                 local modifications, and apply the stash when done (this
>>                 option is only valid when "--rebase" is used).
>
> OK, but according to the definition of --[no-]autostash given in
> git-rebase documentation (https://git-scm.com/docs/git-rebase),
> temporary stash is created.

You shouldn't justify your change by copy-paste from another place. If
the other place is wrong, then replicating it is the worse thing to do
because this would mean we end up with two suboptimal pieces of
documentation instead of one (keep in mind: maintaining stuff is usually
harder than writing it in the first place).

I agree with Junio that "temporary stash" is pleonasm. OTOH, for someone
not familiar with "git stash", the explanation makes no sense anyway.
So, I'd drop the "temporary", and instead add a link to the doc of git
stash. Also, this is not really before starting "pull --rebase" but
after the user calls "pull" and before the actual rebase starst. I'd
write something like this:

	Before starting rebase, stash local modifications away (see
	linkgit:git-stash.txt[1]) if needed, and apply the stash when
	done

(I added "if needed" which makes sense technically since we don't create
an empty stash if not needed)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Paul Tan]

On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  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

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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> 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.

We normally wrap text with a bit less than 80 columns. Yours is wrappet
at 50 columns which makes it look weird.

> --- 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;

Instead of going through this 3-valued "true/false/unset", I would have
let opt_autostash = 0 by default, and read the configuration before the
call to parse_options (the usual way to apply precedence: read from low
precedence to high precedence).

But this is a bit less easy than it seems, since the code currently
checks the configuration variable only when --rebase is given, so my
version would do a useless call to git_config_get_bool() when --rebase
is not given. So I think your version is OK.

> +	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)

The else is not needed since the other branch dies.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Eric Sunshine]

On Thu, Mar 3, 2016 at 12:24 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>> +     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)
>
> The else is not needed since the other branch dies.

A couple other minor comments (to be considered or ignored):

The comment "/* If --[no-]autostash ... */" merely repeats what the
code itself already says, thus is not really helpful and can be
dropped.

It would be reasonable to combine the two cases into one:

    if (opt_autostash != -1)
        die(_("--[no]-autostash option is only valid with --rebase."));

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

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> It would be reasonable to combine the two cases into one:
>
>     if (opt_autostash != -1)
>         die(_("--[no]-autostash option is only valid with --rebase."));

Nit (in case Mehul copy/paste this): that would be --[no-], not --[no]-.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Mehul Jain]

On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> 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.
>
> We normally wrap text with a bit less than 80 columns. Yours is wrappet
> at 50 columns which makes it look weird.

OK. I will change it.

>> +     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)
>
> The else is not needed since the other branch dies.

I'm bit confused here. Which "else" you are talking about. I think both the
"else" and "else if" are needed here because:

- for the first "else", it is necessary that the case is only executed
when --rebase option is not given. If "else" is removed then in some case
where user calls "git pull --rebase --autostash" will lead to the execution of
"else if (opt_autostash == 1)"  case.

- Also removal of  "else if (opt_autostash == 1)" is not the right thing. As
the possibility of opt_autostash = -1 is there and this change may lead to
the execution of "die(_("--no-autostash ... "));" in case user calls "git pull".

Though I agree with Eric on combining the "if and else if" cases.

Thanks,
Mehul

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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>> +     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)
>>
>> The else is not needed since the other branch dies.
>
> I'm bit confused here. Which "else" you are talking about.

The last "else" keyword. Not the "else" branch. It would just work like
this, and be a bit more pleasing to my eyes:

if (...)
	die(...);
if (...)
	die(...);

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Paul Tan]

On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> 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.

>         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

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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---

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

[Mehul Jain]

On Sat, Mar 5, 2016 at 3:22 PM, Mehul Jain <mehul.jain2029@gmail.com> 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

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

[Junio C Hamano]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> +--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.
> +

Should this entry be this verbose and have four separate paragraphs?

I think "This option is..." can and should be a part of the first
paragraph, and *NOTE* paragraph does not have to be there.  The
readers have already been referred to rebase.autoStash that warns
them to be prepared to deal with possible conflicts when replaying
the stashed change.

> -		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 the command line didn't give --[no-]autostash when the control
reaches this section of the code, get_bool() is used to see if
rebase.autostash is set.  If it is explicitly set to false or unset,
the command will check if the work tree is clean and complain.

Makes sense.

This does not have to be in the scope of your change, by the way,
but it may make sense to reduce the use of git_config_get_*()
function when the program already contains a call to git_config().

Instead of calling git_default_config callback from there, introduce
a new callback function and read variables that matter from there,
i.e.

static int opt_autostash = -1;
static int config_autostash = -1;
...
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);
}

and then you do "the command line overrides the configured value"
with

	if (opt_autostash < 0)
        	opt_autostash = config_autostash;
	if (opt_autostash < 0)
        	die_on_unclean_work_tree(...);

Either way you have to read the config files in full at least once
anyway, but the program does not have to ask the config API to look
up the value with a separate git_config_get_bool() call if you did
so.

> 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' '

This is overly verbose, isn't it?

You are verifying that "--no-autostash overrides rebase.autostash"
in this test, so perhaps make it:

    test_expect_success 'pull --rebase: --no-autostash overrides rebase.autostash'

or something?

> +	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' '

Likewise.  This verifies that "--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"
> +'
> +

For completeness, there are two other cases, i.e. rebase.autostash
set to false and --no-autostash given from the command line, and the
variable set to true and --autostash from the command line.

It is worth considering if these two need to be tested.  There are
(at least) two things to take into account:

 - We know, after seeing the current implementation, that these two
   cases do not have to be tested, because the configuration is not
   even looked at when there is a command line option.

 - However, the real reason why we prepare these tests is not to
   demonstrate that the patch that introduces a new feature works as
   expected.  It is to prevent _future_ changes by other people, who
   may not be aware of the reason why we chose not to test these two
   cases, from breaking the code.  If a careless refactoring of the
   code can break our assumption (i.e. "the configuration is not
   even looked at when there is a command line option." above), such
   a change may introduce a bug that disables autostash when both
   variable and option tell the code to autostash, and having the
   two additional tests would catch such a breakage.

>  test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
>  	test_config rebase.autostash true &&
>  	git reset --hard before-rebase &&

These two new tests are inserted in a somewhat strange location, by
the way.  This existing test checks the basic working of rebase.autostash
without any command line override.  Shouldn't the more complex new cases
that deal with command line override come after this one?

Is it worth checking the case where autostash kicks in, rebase
itself is completed successfully, but the final "stash pop" fails in
conflict?  I am thinking aloud and just wondering, not suggesting
to add such a test, or even suggesting that having such a test is
worthwhile. I didn't even check if there is already an existing test
to cover that case.

Other than the above, looking better.  Thanks.

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

[Mehul Jain]

Hi Junio,

Thanks for the thorough review.

On Sat, Mar 5, 2016 at 10:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Is it worth checking the case where autostash kicks in, rebase
> itself is completed successfully, but the final "stash pop" fails in
> conflict?  I am thinking aloud and just wondering, not suggesting
> to add such a test, or even suggesting that having such a test is
> worthwhile. I didn't even check if there is already an existing test
> to cover that case.

I think this case has already been checked in t3420-rebase.autostash.sh:
"rebase$type: non-conflicting rebase, conflicting stash". Though in that
test rebase.autoStash has been used to trigger git-stash.

Thanks,
Mehul

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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

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

[Junio C Hamano]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> 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.

I was expecting you not to do this (it does not have to be in the
scope of this topic), but if you do so, it would be preferrable to
do it as a preparatory clean-up patch--the existing code already
makes a separate get_bool() call to read a single config while it
uses git_config(git_default_config) to read the other variables, and
introduction of git_pull_config() is a clean-up that is independent
of your new feature.

And then on top of that cleaned-up codebase, add this new feature.

> 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]).

Don't we use `typewriter font` when referring to literal command,
option and variable names in the documentation?  The description
for `--rebase` that comes immediately before this hunk seems to
support that observation.

> @@ -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")) {

Style: SP after a comma.  There may be the same problem elsewhere on
the new lines, but I won't point out all of them.

Other than that, looking better.

Thanks.

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

[Mehul Jain]

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 <gitster@pobox.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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()

[Mehul Jain]

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 <gitster@pobox.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Paul Tan]

On Wed, Mar 9, 2016 at 12:18 PM, Mehul Jain <mehul.jain2029@gmail.com> 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.
>
> Helped-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Paul Tan <pyokagan@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> 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?

>  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.

>
>         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

[Mehul Jain]

On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>  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.

>> +       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

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>>  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

What Junio says is that you don't need to write

static int config_autostash = 0;

since it is equivalent to

static int config_autostash;

But there's nothing wrong with having a static variable defaulting to 0.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Mehul Jain]

On Fri, Mar 11, 2016 at 7:00 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> What Junio says is that you don't need to write
>
> static int config_autostash = 0;
>
> since it is equivalent to
>
> static int config_autostash;
>
> But there's nothing wrong with having a static variable defaulting to 0.

My bad. I should have read Junio's comment more carefully.

config_autostash can be default to 0. And thus
             if (opt_autostash != 1)
                    die_on_unclean_work_tree(prefix);

can be replaced by
           if (!opt_autostash)
                    die_on_unclean_work_tree(prefix);

and thus opt_autostash will be either 0 or 1 and we don't
have to worry about it being -1 (whenever --rebase is passed).

I will make the changes accordingly.

Thanks,
Mehul

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

[Mehul Jain]

On Fri, Mar 11, 2016 at 10:21 AM, Paul Tan <pyokagan@gmail.com> 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()

[Mehul Jain]

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 <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Mehul Jain]

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 <Matthieu.Moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Paul Tan <pyokagan@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
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

[Eric Sunshine]

On Tue, Mar 15, 2016 at 1:11 PM, Mehul Jain <mehul.jain2029@gmail.com> 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)...

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> 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.

> 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

[Mehul Jain]

On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> 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.

>> 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.

>> +                       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

[Eric Sunshine]

On Wed, Mar 16, 2016 at 1:00 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> +--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.

>>> 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

[Mehul Jain]

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

[Eric Sunshine]

On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain <mehul.jain2029@gmail.com> 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?