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

[Junio C Hamano]

Paul Tan  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
> Mehul Jain  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", );
>> - 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(, "--no-autostash");
> else
> argv_array_push(, "--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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matthieu Moy]

Mehul Jain  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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
> Mehul Jain  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 
>> ---
>>
>> 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", _ff, NULL,
>>   N_("abort if fast-forward is not possible"),
>>   PARSE_OPT_NOARG | PARSE_OPT_NONEG),
>> + OPT_BOOL(0,"autostash",_autostash,
>> + N_("automatically stash/stash pop before and after rebase")),
>>   OPT_PASSTHRU(0, "verify-signatures", _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(, opt_strategy_opts.argv);
>>   if (opt_gpg_sign)
>>   argv_array_push(, opt_gpg_sign);
>> -
>> + if(opt_autostash)
>
> Style: control keywords are followed by a single SP before the next '('.
>
>> + argv_array_push(,"--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:

 

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

[Junio C Hamano]

Mehul Jain  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 
> ---
>
> 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", _ff, NULL,
>   N_("abort if fast-forward is not possible"),
>   PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> + OPT_BOOL(0,"autostash",_autostash,
> + N_("automatically stash/stash pop before and after rebase")),
>   OPT_PASSTHRU(0, "verify-signatures", _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(, opt_strategy_opts.argv);
>   if (opt_gpg_sign)
>   argv_array_push(, opt_gpg_sign);
> -
> + if(opt_autostash)

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

> + argv_array_push(,"--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(, "--onto");
>   argv_array_push(, 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",_autostash);
> +

Why is this change even necessary?

>   argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>  
>   parse_repo_refspecs(argc, argv, , );
> @@ -835,13 +841,10 @@ int