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

Hello Eric,

I tried out this approach and here's the result.

---

diff --git a/builtin/pull.c b/builtin/pull.c
index b5b0255..0ce007d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c

@@ -86,6 +86,7 @@ static char *opt_commit;
 static char *opt_edit;
 static char *opt_ff;
 static char *opt_verify_signatures;
+static int opt_autostash = -1;
 static int config_autostash;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;

@@ -150,6 +151,8 @@ static struct option pull_options[] = {
        OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
                N_("verify that the named commit has a valid GPG signature"),
                PARSE_OPT_NOARG),
+       OPT_BOOL(0, "autostash", &opt_autostash,
+               N_("automatically stash/stash pop before and after rebase")),
        OPT_PASSTHRU_ARGV('s', "strategy", &opt_strategies, N_("strategy"),
                N_("merge strategy to use"),
                0),

@@ -801,6 +804,7 @@ 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);
+       argv_array_push(&args, opt_autostash ? "--autostash" :

"--no-autostash");

        argv_array_push(&args, "--onto");
        argv_array_push(&args, sha1_to_hex(merge_head));

@@ -846,12 +850,20 @@ int cmd_pull(int argc, const char **argv, const

char *prefix)
        if (get_sha1("HEAD", orig_head))
                hashclr(orig_head);

+       if(!opt_rebase && opt_autostash != -1)
+               die(_("--[no-]autostash option is only valid with --rebase."));
+
        if (opt_rebase) {
                int autostash = config_autostash;

                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes
added to the index."));

+               if (opt_autostash != -1)
+                       autostash = opt_autostash;
+               else
+                       opt_autostash = config_autostash;
+
                if (!autostash)
                        die_on_unclean_work_tree(prefix);
---

This way of implementation looks a bit less clean to me than
the previous one because we are using "opt_autostash" to pass
the "--[no-]autostash"  flag to git-rebase, thus if user does not
specify anything about stashing in command line then  config_autostash
value has to be used ( i.e. opt_autostash = config_autostash).
To do this an "else" case has to be introduced in the code. This
might effect the readability of the code because the reader might
wonder why "opt_autostash" is used to assign value to "autostash"
in one case, and opt_autostash = config_autostash in other case.

But I agree that this way I won't be touching the changes I made
in patch 1/2.

I would like to know your view on above mentioned issue.

Also I made a mistake in patch 1/2 which I will correct in the next
version along with other changes suggested by you.

Thanks,
Mehul