Re: [PATCH v2 3/8] checkout: validate stage and merge option compatibility in checkout_paths()

Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

quoted

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

quoted

 	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.

quoted

quoted

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