On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
>> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
>> +--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.
>> 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);
...
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;
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.
>> + opt_autostash = config_autostash;
>> +
>> + if (!opt_autostash)
>> die_on_unclean_work_tree(prefix);
>>
>> if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>> hashclr(rebase_fork_point);
>> - }
>> + } else
>> + if (opt_autostash != -1)
>> + die(_("--[no-]autostash option is only valid with --rebase."));
>
> How about formatting this as a normal 'else if'?
>
> } else if (opt_autostash != -1)
I thought of it earlier but voted against it as it may reduce the readability of
the code.
> On the other hand, this error case hanging off the 'rebase'
> conditional is somewhat more difficult to reason about than perhaps it
> ought to be. It might be easier to see what's going on if you get the
> error case out of the way early, and then handle the normal case. That
> is, something like this:
>
> if (!opt_rebase && opt_autostash != -1)
> die(_("... is only valid with --rebase"));
>
> if (opt_rebase) {
> ...
> }
This is good. I'll make the changes accordingly.
Thanks for the comments.
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html