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

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

[Andy Koppe]

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(-)

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

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

[Junio C Hamano]

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.

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

[Andy Koppe]

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.

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

[Junio C Hamano]

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.

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

[Andy Koppe]

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 test for both '--staged' and '--staged --worktree'.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---

CI: https://github.com/ak2/git/actions/runs/4276063110

 builtin/checkout.c | 29 +++++++++++++++++++++--------
 t/t2070-restore.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a5155cf55c..17b179a797 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -489,15 +489,28 @@ 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)
-		die(_("'%s' or '%s' cannot be used with %s"),
-		    "--ours", "--theirs", "--staged");
+	/*
+	 * Reject --staged option to the restore command when combined with
+	 * merge-related options. Use the accept_ref flag to distinguish it
+	 * from the checkout command, which does not accept --staged anyway.
+	 *
+	 * `restore --ours|--theirs --worktree --staged` could mean resolving
+	 * conflicted paths to one side in both the worktree and the index,
+	 * but does not currently.
+	 *
+	 * `restore --merge|--conflict=<style>` already recreates conflicts
+	 * in both the worktree and the index, so adding --staged would be
+	 * meaningless.
+	 */
+	if (!opts->accept_ref && opts->checkout_index) {
+		if (opts->writeout_stage)
+			die(_("'%s' or '%s' cannot be used with %s"),
+			    "--ours", "--theirs", "--staged");
 
-	if (opts->checkout_index && !opts->checkout_worktree &&
-	    opts->merge)
-		die(_("'%s' or '%s' cannot be used with %s"),
-		    "--merge", "--conflict", "--staged");
+		if (opts->merge)
+			die(_("'%s' or '%s' cannot be used with %s"),
+			    "--merge", "--conflict", "--staged");
+	}
 
 	if (opts->patch_mode) {
 		enum add_p_mode patch_mode;
diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh
index 7c43ddf1d9..c5d19dd973 100755
--- a/t/t2070-restore.sh
+++ b/t/t2070-restore.sh
@@ -137,4 +137,20 @@ 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' '
+	for opts in \
+		"--staged --ours" \
+		"--staged --theirs" \
+		"--staged --merge" \
+		"--staged --conflict=diff3" \
+		"--staged --worktree --ours" \
+		"--staged --worktree --theirs" \
+		"--staged --worktree --merge" \
+		"--staged --worktree --conflict=zdiff3"
+	do
+		test_must_fail git restore $opts . 2>err &&
+		grep "cannot be used with --staged" err || return
+	done
+'
+
 test_done
-- 
2.39.0

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

[Junio C Hamano]

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

> +	/*
> +	 * Reject --staged option to the restore command when combined with
> +	 * merge-related options. Use the accept_ref flag to distinguish it
> +	 * from the checkout command, which does not accept --staged anyway.

Understandable.

> +	 * `restore --ours|--theirs --worktree --staged` could mean resolving
> +	 * conflicted paths to one side in both the worktree and the index,
> +	 * but does not currently.

Understandable, especially with an understanding that "does not
currently" hints our wish to eventually support it.

> +	 * `restore --merge|--conflict=<style>` already recreates conflicts
> +	 * in both the worktree and the index, so adding --staged would be
> +	 * meaningless.

And from the same line of reasoning, I do not know if this is a good
idea.  If "--merge|--conflict=<style>" should recreate conflicts in
both when given to "restore --staged --worktree", and if it does so
already, then shouldn't it be simply allowed?

Why would it be meaningless?

Now, it may be understandable to say that it is meaningless to ask
merge conflict recreated only in the working tree file but not in
the index, or done only in the index but not in the working tree,
and erroring out such a request might make sense, but even then, if
we do not plan to change the behaviour in the future when "restore
--staged --merge" without "--worktree" from what we currently do, I
am not sure if it makes sense to error out such a "meaningless"
request.

Or perhaps I misunderstood the conditional below?

> +	 */
> +	if (!opts->accept_ref && opts->checkout_index) {
> +		if (opts->writeout_stage)
> +			die(_("'%s' or '%s' cannot be used with %s"),
> +			    "--ours", "--theirs", "--staged");
>  
> -	if (opts->checkout_index && !opts->checkout_worktree &&
> -	    opts->merge)
> -		die(_("'%s' or '%s' cannot be used with %s"),
> -		    "--merge", "--conflict", "--staged");
> +		if (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..c5d19dd973 100755
> --- a/t/t2070-restore.sh
> +++ b/t/t2070-restore.sh
> @@ -137,4 +137,20 @@ 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' '
> +	for opts in \
> +		"--staged --ours" \
> +		"--staged --theirs" \
> +		"--staged --merge" \
> +		"--staged --conflict=diff3" \
> +		"--staged --worktree --ours" \
> +		"--staged --worktree --theirs" \
> +		"--staged --worktree --merge" \
> +		"--staged --worktree --conflict=zdiff3"
> +	do
> +		test_must_fail git restore $opts . 2>err &&
> +		grep "cannot be used with --staged" err || return
> +	done
> +'

It is quite clear what cases are (and are not) being tested here
when written this way.

Thanks.