Re: [PATCH] restore: fault --staged --worktree with merge opts
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 21, 2023, 18:38 UTC
- Message-ID
- <xmqqa616g8yv.fsf@gitster.g>
- In-Reply-To
- <20230218163936.980-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 13 quoted lines
> 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.
Show 5 quoted lines
> - 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.
Show 7 quoted lines
> + 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?
Show 12 quoted lines
> +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.