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

On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine [off-list ref] wrote:

On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain [off-list ref] wrote:

quoted

@@ -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");

At this point, we know that opt_autostash can't be -1 (thus
incorrectly triggering use of --autostash) because the conditional in
cmd_pull() set it to the value of config_autostash (either 0 or 1) if
the user did not specify it on the command-line. Okay. Makes sense.

Actually, this is going to pass --autostash or --no-autostash to
git-rebase unconditionally won't it? This seems kind of undesirable
due to the unnecessarily tight coupling it creates between the two
commands. I wasn't paying close attention to the earlier discussion,
but wasn't the idea that you should pass one of these two options
along to git-rebase only if the user explicitly asked to do by saying
so on the command line?

This is interesting. I checked out git-rebase.sh and found that it reads
rebase.autoStash if nothing is specified by user. So if user is not
specifying anything about stashing then it is the job of git-rebase
to decide whether or not to do stashing by reading rebase.autoStash.

Similarly if user doesn't specify the --[no-]autostash option to git-pull
then neither of --autostash and --no-autstash should be passed to the
git-rebase as it will decide on his own about what needs to be done.
Agreed. I made a unnecessary tight coupling between git-pull and
git-rebase. Instead of that the following changes can be done to
remove it.

...
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
-       argv_array_push(&args, opt_autostash ? "--autostash" :
"--no-autostash");
+       if (opt_autostash == 0)
+               argv_array_push(&args, "--no-autostash");
+       else if (opt_autostash == 1)
+               argv_array_push(&args, "--autostash");

...

...
        if (opt_rebase) {
+               int autostash = config_autostash;
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("... with changes added to the index."));

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

-               if (!opt_autostash)
+               if (!autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
        }
...

Note that the above changes are suggest with respect to my patch, not
the current
code base.

This way there's no need to remove "autostash" from the current code
base and instead use it to write a much cleaner patch.  Something like
this (this is w.r.t. current code base)

...
        if (opt_gpg_sign)
                argv_array_push(&args, opt_gpg_sign);
+        if (opt_autostash == 0)
+               argv_array_push(&args, "--no-autostash");
+       else if (opt_autostash == 1)
+               argv_array_push(&args, "--autostash");

...

...

if (opt_rebase) {
                 int autostash = config_autostash;
+               if (opt_autostash != -1)
+                       autostash = opt_autostash;

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

                if (!autostash)
                        die_on_unclean_work_tree(prefix);

                if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
                        hashclr(rebase_fork_point);
        }
...


What are your views on this? Also this way I will not touch changes
introduced in patch 1/2.

In other words:

* invoke "git-rebase --autostash" only if the user typed "git pull
--rebase --autostash"

* invoke "git-rebase --no-autostash" only if the user typed "git pull
--rebase --no-autostash"

* invoke "git rebase" if the user typed bare "git pull --rebase"

Thanks,
Mehul