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

On Wed, Mar 16, 2016 at 1:00 AM, Mehul Jain [off-list ref] wrote:

On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine [off-list ref] wrote:

quoted

quoted

+--autostash::
+--no-autostash::
+       Before starting rebase, stash local modifications away (see
+       linkgit:git-stash.txt[1]) if needed, and apply the stash when
+       done (this option is only valid when "--rebase" is used).
++
+`--no-autostash` is useful to override the `rebase.autoStash`
+configuration variable (see linkgit:git-config[1]).

The last couple sentences seem reversed. It feels odd to have the bit
about --rebase dropped dead in the middle of the description of
--autostash and --no-autostash. I'd have expected to see --autostash
and --no-autostash discussed together, and then, as a follow-on,
mention them being valid only with --rebase.

So you are suggesting something like this:

--autostash::
--no-autostash::
    Before starting rebase, stash local modifications away (see
    linkgit:git-stash.txt[1]) if needed, and apply the stash when
    done. `--no-autostash` is useful to override the `rebase.autoStash`
    configuration variable (see linkgit:git-config[1]).
+
This option is only valid when "--rebase" is used.

Can be done and it make more sense to talk about the validity of the
option in a seperate line.

Yes, like that.

quoted

quoted

diff --git a/builtin/pull.c b/builtin/pull.c

@@ -851,12 +855,17 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
                if (is_null_sha1(orig_head) && !is_cache_unborn())
                        die(_("Updating an unborn branch with changes added to the index."));

-               if (config_autostash)
+               if (opt_autostash == -1)

In patch 1/2, this changed from 'if (autostash)' to 'if
(config_autostash)'; it's a bit sad that patch 2/2 then has to touch
the same code to change it yet again, this time to 'if
(opt_autostash)'. Have you tried just keeping the original 'autostash'
variable and modifying its value based upon config_autostash and
opt_autostash instead? (Not saying that this would be better, but
interested in knowing if the result is as clean or cleaner or worse.)

Yes, I tried that. Things looked something like this:

In patch 1/2
...

-    int autostash = 0;
+    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

-    git_config_get_bool("rebase.autostash", &autostash);
    if (!autostash)
            die_on_unclean_work_tree(prefix);

The individual diffs look nicer and are less noisy, thus a bit easier to review.

In patch 2/2
...
    int autostash = config_autoshash;

    if (is_null_sha1(orig_head) && !is_cache_unborn())
            die(_("Updating ..."));

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

I'd probably have placed this conditional just below the line where
'autostash' is declared so that the logic for computing the value of
'autostash' is all in one place.

    if (!autostash)
        die_on_unclean_work_tree(prefix);
...

This implementation looks much more cleaner but we are using some
extra space (autostash) to do the task. If everyone is fine with this
trade off then I can re-roll a new patch with this method. Comments please.

Using an extra variable isn't a big deal and would be a good idea if
it helped clarify the logic. In this case, the logic isn't
particularly difficult, so I don't feel too strongly about it one way
or the other.