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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2016, 19:26 UTC
Message-ID: <xmqqk2lqeyzv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1456594902-21182-1-git-send-email-mehul.jain2029@gmail.com>

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.

Show 6 quoted lines

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

Show 29 quoted lines

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

Show 18 quoted lines

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

Show 9 quoted lines

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

Show 16 quoted lines

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