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

[Paul Tan]

On Fri, Mar 4, 2016 at 12:13 AM, Mehul Jain  wrote:
> 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", _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);
> -

Minor nit: but when I wrote the code for run_rebase() I separated the
options for "Shared options" and "Options passed to git-rebase" into
different code block groups from the other code, and I would like it
if it remained that way :-(

> +   if (opt_autostash)
> +   argv_array_push(, "--autostash");

Hmm, interesting. If rebase.autostash=true !opt_autostash, we don't
need to pass --no-autostash to git-rebase because it will only stash
if the worktree is dirty, but a dirty worktree will be caught by
git-pull's die_on_unclean_worktree() anyway.

Still, it may be a problem because the worktree may become dirty
in-between our "worktree is clean" check and when git-rebase is run
(during the git-fetch), and the user may be surprised if git-rebase
attempted to stash in that case.

So we may wish to pass "--no-autostash" to git-rebase as well.

> argv_array_push(, "--onto");
> argv_array_push(, 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", );
> -   if (!autostash)
> +   if (opt_autostash == -1)
> +   git_config_get_bool("rebase.autostash", 
> _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 {

Git code style puts the else on the same line, not on a new one.

> +   /* If --[no-]autostash option is called without --rebase */

Yeah, I agree with Eric that this comment should be dropped,

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

and these error messages combined.

>
> if (run_fetch(repo, refspecs))
> return 1;
> --
> 2.7.1.340.g69eb491.dirty

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 v3 1/3] pull --rebase: add --[no-]autostash flag

[Matthieu Moy]

Mehul Jain  writes:

> On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
>  wrote:
>> Mehul Jain  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.

The last "else" keyword. Not the "else" branch. It would just work like
this, and be a bit more pleasing to my eyes:

if (...)
die(...);
if (...)
die(...);

-- 
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 v3 1/3] pull --rebase: add --[no-]autostash flag

[Matthieu Moy]

Eric Sunshine  writes:

> It would be reasonable to combine the two cases into one:
>
> if (opt_autostash != -1)
> die(_("--[no]-autostash option is only valid with --rebase."));

Nit (in case Mehul copy/paste this): that would be --[no-], not --[no]-.

-- 
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 v3 1/3] pull --rebase: add --[no-]autostash flag

[Mehul Jain]

On Thu, Mar 3, 2016 at 10:54 PM, Matthieu Moy
 wrote:
> Mehul Jain  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.

OK. I will change it.

>> + 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
--
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 v3 1/3] pull --rebase: add --[no-]autostash flag

[Eric Sunshine]

On Thu, Mar 3, 2016 at 12:24 PM, Matthieu Moy
 wrote:
> Mehul Jain  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.

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."));
--
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 v3 1/3] pull --rebase: add --[no-]autostash flag

[Matthieu Moy]

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

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

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

[Mehul Jain]

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 
Helped-by: Junio C Hamano 
Helped-by: Paul Tan 
Signed-off-by: Mehul Jain 
---
Sorry for a late follow up. I had my mid-semester
examination going on.

Previous patch: $gname287709

Changes:
* error message is produced when "git pull
 --[no]autostash" is called.
* opt_autostash intialized with -1.

 builtin/pull.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

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", _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)
+   argv_array_push(, "--autostash");
argv_array_push(, "--onto");
argv_array_push(, 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", );
-   if (!autostash)
+   if (opt_autostash == -1)
+   git_config_get_bool("rebase.autostash", _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 --[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)
+   die(_("--autostash option is only valid with 
--rebase."));
+   }
 
if (run_fetch(repo, refspecs))
return 1;
-- 
2.7.1.340.g69eb491.dirty

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