On Wed, Mar 16, 2016 at 1:00 AM, Mehul Jain <[email protected]> wrote:
> On Wed, Mar 16, 2016 at 3:13 AM, Eric Sunshine <[email protected]>
> wrote:
>>> +--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.
>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
