From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 02 Sep 2026 11:20:53 GMT
Subject: Re: [PATCH v2 3/8] checkout: validate stage and merge option compatibility in checkout_paths()
Message-ID: <CAOLa=ZSQs5umaTxT6RKQJdnnAEbK+AHgj0n5yiTM0jsbZcyiig@mail.gmail.com>
In-Reply-To: <xmqqld9ksw26.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> 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.

>>> @@ -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