On Fri, Mar 18, 2016 at 10:09 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, Mar 18, 2016 at 12:24 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Thu, Mar 17, 2016 at 12:49 PM, Mehul Jain <mehul.jain2029@xxxxxxxxx> wrote:
>>> @@ -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
--
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