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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mar 3, 2016, 17:24 UTC
Message-ID: <vpqd1rbiifi.fsf@anie.imag.fr>
In-Reply-To: <1457021601-9099-1-git-send-email-mehul.jain2029@gmail.com>

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

Show 10 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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