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

On Thu, Mar 17, 2016 at 4:17 AM, Mehul Jain [off-list ref] wrote:

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

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

That's pretty ugly. Since cmd_pull() is the only caller of
run_rebase(), an alternative would be to pass 'autostash' as an
argument to run_rebase(). However, since run_rebase() is already
accessing other 'opt_foo' globals, it wouldn't make sense to make an
exception of 'autostash' by passing it as an argument. So, in the end,
the original approach is indeed probably cleaner.

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

Which mistake would that be?