Re: [PATCH] restore: fault --staged --worktree with merge opts

[Junio C Hamano <gitster@pobox.com>]

Andy Koppe <andy.koppe@gmail.com> writes:

> The 'restore' command already rejects the --merge, --conflict, --ours
> and --theirs options when combined with --staged, but accepts them when
> --worktree is added as well.
>
> Unfortunately that doesn't appear to do anything useful. The --ours and
> --theirs options seem to be ignored when both --staged and --worktree
> are given, whereas with --merge or --conflict, the command has the same
> effect as if the --staged option wasn't present.

I think "--ours" and "--theirs" should not have any effect unless
you are checking out from the index to the working tree.  And
"--worktree --staged" (i.e. update both working tree and the index
[*]) is clearly outside that use case.  It is understandable that
these options are not "honored", simply because there is no sane way
to "honor" them [*], but it may give us a nicer end-user experience
if we noticed such incompatible combinations of options and errored
out, instead of silently ignored them.

	Side note: "--staged" here is a bit of misnomer, but it
        unfortunately is way too late to fix.  When an option
        affects only the index, "--cached" is how we spell it (and
        "--index" is an option that makes the command affect both
        the index and the working tree).

	Side note 2: it is conceivable that --worktree --staged
	--ours may want to (1) resolve the conflicted path to stage
	#2 in the index and (2) check out the result in the working
	tree.  But until such an improved behaviour gets
	implemented, it is probably better to error it out for now.
	It is much easier to allow what has been forbidden later,
	than changing the behaviour of a command to work
	differently.

> So reject those options with '--staged --worktree' as well, using
> opts->accept_ref to distinguish restore from checkout.

OK.  This probably deserves in-code comment, if the patch is
introducing behaviour that is specific to only one command in a
codepath that is shared across multiple commands.

I like the general thrust of the change, but have some comments on
the implementation.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index a5155cf55c..b09322f7c8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -489,13 +489,11 @@ static int checkout_paths(const struct checkout_opts *opts,
>  		die(_("'%s' must be used when '%s' is not specified"),
>  		    "--worktree", "--source");
>  
> -	if (opts->checkout_index && !opts->checkout_worktree &&
> -	    opts->writeout_stage)
> +	if (!opts->accept_ref && opts->checkout_index && opts->writeout_stage)
>  		die(_("'%s' or '%s' cannot be used with %s"),
>  		    "--ours", "--theirs", "--staged");

We used to die when "--ours/--theirs" is given (i.e. writeout_stage
is not 0), checkout_index is set *AND* checkout_worktree is not set,
i.e. when "--staged" (i.e. restore the path in the index) but not
"--worktree" is in effect.

Now, we drop "checkout_worktree is not set" as the condition, but
only when we are doing "git restore".  We die "--ours/--theirs" is
given and checkout_index is set, i.e. "--staged" is there, whether
"--worktree" is given or not.

Makes sense.

> -	if (opts->checkout_index && !opts->checkout_worktree &&
> -	    opts->merge)
> +	if (!opts->accept_ref && opts->checkout_index && opts->merge)
>  		die(_("'%s' or '%s' cannot be used with %s"),
>  		    "--merge", "--conflict", "--staged");

Likewise.

> +test_expect_success 'restore with merge options rejects --staged' '
> +	test_must_fail git restore --staged --merge . -- 2>err1 &&

What is "." meant to be on this command line?  If it is "the whole
working tree", it should come after the double-dash "--", no?  As
written, I _think_ it is stripping "--" at the end, but ".", which
was written before "--" to explicitly say "this is not a pathspec",
is still taken as a pathspec (which may be a bug in the option
parsing code).

> +	test_i18ngrep "cannot be used with" err1 &&

"test_i18ngrep" is on its way out (it was part of an older way for
i18n testing that has been removed).  We can use "grep" instead.

> +	test_must_fail git restore --staged --conflict=diff3 . -- 2>err2 &&
> +	test_i18ngrep "cannot be used with" err2 &&
> +	test_must_fail git restore --staged --ours . -- 2>err3 &&
> +	test_i18ngrep "cannot be used with" err3 &&
> +	test_must_fail git restore --staged --theirs . -- 2>err4 &&
> +	test_i18ngrep "cannot be used with" err4
> +'

Not making a suggestion yet, but thinking aloud.  Would it make it
easier to see what is being tested if we wrote these as a loop:

	for opts in \
		"--staged --merge" \
		"--staged --conflict=diff3" \
		"--staged --ours" \
		"--staged --theirs"
	do
		test_must_fail git restore $opts 2>err &&
		grep "cannot be used with" err || return
	done

Without having to skip every alternating lines, we can see what
option combinations are being tested fairly easily when written that
way, perhaps?

> +test_expect_success 'restore with merge options rejects --staged --worktree' '
> +	test_must_fail git restore --staged --worktree --merge . -- 2>err1 &&
> +	test_i18ngrep "cannot be used with" err1 &&
> +	test_must_fail git restore --staged --worktree --conflict=diff3 . -- 2>err2 &&
> +	test_i18ngrep "cannot be used with" err2 &&
> +	test_must_fail git restore --staged --worktree --ours . -- 2>err3 &&
> +	test_i18ngrep "cannot be used with" err3 &&
> +	test_must_fail git restore --staged --worktree --theirs . -- 2>err4 &&
> +	test_i18ngrep "cannot be used with" err4
> +'
> +
>  test_done

Thanks.