Mehul Jain [off-list ref] 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.
quoted hunk
--- 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/
On Thu, Mar 3, 2016 at 12:24 PM, Matthieu Moy
[off-list ref] wrote:
Mehul Jain [off-list ref] writes:
quoted
+ 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."));
On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
[off-list ref] wrote:
Mehul Jain [off-list ref] writes:
quoted
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.
quoted
+ 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