| Message ID | 20230218163936.980-1-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | restore: fault --staged --worktree with merge opts | expand |
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.
On Tue, 21 Feb 2023 at 18:38, Junio C Hamano wrote: > 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. Same with restore --worktree --staged --theirs and stage #3? That's basically what I thought these combinations would do when I noticed that they were accepted. I think they would be quite convenient compared to separate restore and add. They'd be the equivalent of 'svn resolve --accept=mine-full/theirs-full'. > But until such an improved behaviour gets > implemented, it is probably better to error it out for now. Indeed. Unfortunately implementing that improvement is beyond my knowledge of git internals. >> +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? Sorry, that was accidental. The command requires a path argument to get to the option conflict error, but as you say, putting it before the "--" doesn't make sense. Thank you very much for the thorough review. I'll prepare a new version of the patch.
Andy Koppe <andy.koppe@gmail.com> writes: > On Tue, 21 Feb 2023 at 18:38, Junio C Hamano wrote: >> 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. > > Same with restore --worktree --staged --theirs and stage #3? These two work pretty much symmetrical, so the same story should apply there, I would think. > Thank you very much for the thorough review. I'll prepare a new > version of the patch. Thanks.
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"); - 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"); diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh index 7c43ddf1d9..373dc1657e 100755 --- a/t/t2070-restore.sh +++ b/t/t2070-restore.sh @@ -137,4 +137,26 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' ' test_must_fail git rev-parse HEAD:new1 ' +test_expect_success 'restore with merge options rejects --staged' ' + test_must_fail git restore --staged --merge . -- 2>err1 && + test_i18ngrep "cannot be used with" err1 && + 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 +' + +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
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. So reject those options with '--staged --worktree' as well, using opts->accept_ref to distinguish restore from checkout. Add tests for both --staged and '--staged --worktree'. Signed-off-by: Andy Koppe <andy.koppe@gmail.com> --- CI run: https://github.com/ak2/git/actions/runs/4210823089 Some more explanation: when finding that 'restore --staged --worktree' with --ours or --theirs was accepted, I assumed that it would do the equivalent of 'restore --ours/--theirs <paths> && add --update <paths>'. As it doesn't do that, I think it's better to raise the same error as without --worktree. builtin/checkout.c | 6 ++---- t/t2070-restore.sh | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-)