v2, 2 partspull --rebase: add --[no-]autostash flag

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

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

> 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

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

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

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

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

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

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

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

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

> 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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

>> 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) {
>         ...
>     }

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

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

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