Re: [PATCH v2 3/8] checkout: validate stage and merge option compatibility in checkout_paths()
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Sep 2, 2026, 11:20 UTC
- Message-ID
- <CAOLa=ZSQs5umaTxT6RKQJdnnAEbK+AHgj0n5yiTM0jsbZcyiig@mail.gmail.com>
- In-Reply-To
- <xmqqld9ksw26.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> @@ -591,6 +591,10 @@ static int checkout_paths(const struct checkout_opts *opts,
>>> die(_("'%s', '%s', or '%s' cannot be used when checking out of a tree"),
>>> "--merge", "--ours", "--theirs");
>>>
>>> + if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
>>> + die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
>>> + "checking out of the index."));
>>> +
>>
>> Doesn't checkout_paths() also get triggered when using '--patch',
>> shouldn't this go below...
>>
>>> if (opts->patch_mode) {
>>> enum add_p_mode patch_mode;
>>> struct interactive_options interactive_opts = {
>>>
>>
>> ...this block? As the original checked for `opts->patchspec.nr`
>
> Sorry, I do not quite understand your question.
>
> The "if (opts->patch_mode)" block indeed appears in checkout_paths()
> and when "--patch" is given the control comes here. Are you worried
> about
>
> git restore --patch --merge --theirs
>
> triggering the option compatibility check, instead of ignoring it
> and directly jumping into the patch mode?
>Sorry if I was unclear. I was stating that the condition udner which the check runs is now difference. Previously we checked `opts->pathspec.nr`, but now `checkout_paths()` is also entered for '--patch' without any path spec.
Having a closer look, it seems to be okay. Because in patch mode, each of the combinations is already rejected before in `checkout_paths()`.
It still might be good to have a sentence in a the commit message, since the guard did change and the reasoning why that is safe is not so obvious.
This does however change the precedence of the error messages:
$ git.master restore --staged --theirs --merge foo fatal: git checkout: --ours/--theirs, --force and --merge are incompatible when checking out of the index.
$ git.patch restore --staged --theirs --merge foo fatal: '--ours' or '--theirs' cannot be used with --staged
This seems okay to me, not sure if we have tests to validate this behavior.
Show 15 quoted lines
>>> @@ -2063,11 +2067,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>>>
>>> opts->pathspec.recursive = 1;
>>>
>>> - if (opts->pathspec.nr) {
>>> - if (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)
>>> - die(_("git checkout: --ours/--theirs, --force and --merge are incompatible when\n"
>>> - "checking out of the index."));
>>> - } else {
>>> + if (!opts->pathspec.nr) {
>>> if (opts->accept_pathspec && !opts->empty_pathspec_ok &&
>>> !opts->patch_mode) /* patch mode is special */
>>> die(_("you must specify path(s) to restore"));
>>> --
>>> 2.55.0-884-g76cf8659c2