{"thread":{"id":"59267","subject":"[PATCH] restore: fault --staged --worktree with merge opts","startedAt":"2023-02-18T16:41:35Z","lastAt":"2023-02-28T01:03:08Z","messageCount":6,"participants":["Andy Koppe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472302","messageId":"20230218163936.980-1-andy.koppe@gmail.com","threadId":"59267","inReplyTo":null,"subject":"[PATCH] restore: fault --staged --worktree with merge opts","fromName":"Andy Koppe","fromEmail":"andy.koppe@gmail.com","sentAt":"2023-02-18T16:39:36Z","receivedAt":"2023-02-18T16:41:35Z","isPatch":true,"sender":{"key":"andy.koppe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/223411?v=4"},"body":"The 'restore' command already rejects the --merge, --conflict, --ours\nand --theirs options when combined with --staged, but accepts them when\n--worktree is added as well.\n\nUnfortunately that doesn't appear to do anything useful. The --ours and\n--theirs options seem to be ignored when both --staged and --worktree\nare given, whereas with --merge or --conflict, the command has the same\neffect as if the --staged option wasn't present.\n\nSo reject those options with '--staged --worktree' as well, using\nopts->accept_ref to distinguish restore from checkout.\n\nAdd tests for both --staged and '--staged --worktree'.\n\nSigned-off-by: Andy Koppe \n---\n\nCI run: https://github.com/ak2/git/actions/runs/4210823089\n\nSome more explanation: when finding that 'restore --staged --worktree'\nwith --ours or --theirs was accepted, I assumed that it would do the\nequivalent of 'restore --ours/--theirs && add --update '.\nAs it doesn't do that, I think it's better to raise the same error as\nwithout --worktree.\n\n builtin/checkout.c | 6 ++----\n t/t2070-restore.sh | 22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a5155cf55c..b09322f7c8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -489,13 +489,11 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\tdie(_(\"'%s' must be used when '%s' is not specified\"),\n \t\t \"--worktree\", \"--source\");\n \n-\tif (opts->checkout_index && !opts->checkout_worktree &&\n-\t opts->writeout_stage)\n+\tif (!opts->accept_ref && opts->checkout_index && opts->writeout_stage)\n \t\tdie(_(\"'%s' or '%s' cannot be used with %s\"),\n \t\t \"--ours\", \"--theirs\", \"--staged\");\n \n-\tif (opts->checkout_index && !opts->checkout_worktree &&\n-\t opts->merge)\n+\tif (!opts->accept_ref && opts->checkout_index && opts->merge)\n \t\tdie(_(\"'%s' or '%s' cannot be used with %s\"),\n \t\t \"--merge\", \"--conflict\", \"--staged\");\n \ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 7c43ddf1d9..373dc1657e 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -137,4 +137,26 @@ test_expect_success 'restore --staged invalidates cache tree for deletions' '\n \ttest_must_fail git rev-parse HEAD:new1\n '\n \n+test_expect_success 'restore with merge options rejects --staged' '\n+\ttest_must_fail git restore --staged --merge . -- 2>err1 &&\n+\ttest_i18ngrep \"cannot be used with\" err1 &&\n+\ttest_must_fail git restore --staged --conflict=diff3 . -- 2>err2 &&\n+\ttest_i18ngrep \"cannot be used with\" err2 &&\n+\ttest_must_fail git restore --staged --ours . -- 2>err3 &&\n+\ttest_i18ngrep \"cannot be used with\" err3 &&\n+\ttest_must_fail git restore --staged --theirs . -- 2>err4 &&\n+\ttest_i18ngrep \"cannot be used with\" err4\n+'\n+\n+test_expect_success 'restore with merge options rejects --staged --worktree' '\n+\ttest_must_fail git restore --staged --worktree --merge . -- 2>err1 &&\n+\ttest_i18ngrep \"cannot be used with\" err1 &&\n+\ttest_must_fail git restore --staged --worktree --conflict=diff3 . -- 2>err2 &&\n+\ttest_i18ngrep \"cannot be used with\" err2 &&\n+\ttest_must_fail git restore --staged --worktree --ours . -- 2>err3 &&\n+\ttest_i18ngrep \"cannot be used with\" err3 &&\n+\ttest_must_fail git restore --staged --worktree --theirs . -- 2>err4 &&\n+\ttest_i18ngrep \"cannot be used with\" err4\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"472389","messageId":"xmqqa616g8yv.fsf@gitster.g","threadId":"59267","inReplyTo":"20230218163936.980-1-andy.koppe@gmail.com","subject":"Re: [PATCH] restore: fault --staged --worktree with merge opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-21T18:38:16Z","receivedAt":"2023-02-21T18:38:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Koppe writes:\n\n> The 'restore' command already rejects the --merge, --conflict, --ours\n> and --theirs options when combined with --staged, but accepts them when\n> --worktree is added as well.\n>\n> Unfortunately that doesn't appear to do anything useful. The --ours and\n> --theirs options seem to be ignored when both --staged and --worktree\n> are given, whereas with --merge or --conflict, the command has the same\n> effect as if the --staged option wasn't present.\n\nI think \"--ours\" and \"--theirs\" should not have any effect unless\nyou are checking out from the index to the working tree. And\n\"--worktree --staged\" (i.e. update both working tree and the index\n[*]) is clearly outside that use case. It is understandable that\nthese options are not \"honored\", simply because there is no sane way\nto \"honor\" them [*], but it may give us a nicer end-user experience\nif we noticed such incompatible combinations of options and errored\nout, instead of silently ignored them.\n\n\tSide note: \"--staged\" here is a bit of misnomer, but it\n unfortunately is way too late to fix. When an option\n affects only the index, \"--cached\" is how we spell it (and\n \"--index\" is an option that makes the command affect both\n the index and the working tree).\n\n\tSide note 2: it is conceivable that --worktree --staged\n\t--ours may want to (1) resolve the conflicted path to stage\n\t#2 in the index and (2) check out the result in the working\n\ttree. But until such an improved behaviour gets\n\timplemented, it is probably better to error it out for now.\n\tIt is much easier to allow what has been forbidden later,\n\tthan changing the behaviour of a command to work\n\tdifferently.\n\n> So reject those options with '--staged --worktree' as well, using\n> opts->accept_ref to distinguish restore from checkout.\n\nOK. This probably deserves in-code comment, if the patch is\nintroducing behaviour that is specific to only one command in a\ncodepath that is shared across multiple commands.\n\nI like the general thrust of the change, but have some comments on\nthe implementation.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index a5155cf55c..b09322f7c8 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -489,13 +489,11 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\tdie(_(\"'%s' must be used when '%s' is not specified\"),\n> \t\t \"--worktree\", \"--source\");\n> \n> -\tif (opts->checkout_index && !opts->checkout_worktree &&\n> -\t opts->writeout_stage)\n> +\tif (!opts->accept_ref && opts->checkout_index && opts->writeout_stage)\n> \t\tdie(_(\"'%s' or '%s' cannot be used with %s\"),\n> \t\t \"--ours\", \"--theirs\", \"--staged\");\n\nWe used to die when \"--ours/--theirs\" is given (i.e. writeout_stage\nis not 0), checkout_index is set *AND* checkout_worktree is not set,\ni.e. when \"--staged\" (i.e. restore the path in the index) but not\n\"--worktree\" is in effect.\n\nNow, we drop \"checkout_worktree is not set\" as the condition, but\nonly when we are doing \"git restore\". We die \"--ours/--theirs\" is\ngiven and checkout_index is set, i.e. \"--staged\" is there, whether\n\"--worktree\" is given or not.\n\nMakes sense.\n\n> -\tif (opts->checkout_index && !opts->checkout_worktree &&\n> -\t opts->merge)\n> +\tif (!opts->accept_ref && opts->checkout_index && opts->merge)\n> \t\tdie(_(\"'%s' or '%s' cannot be used with %s\"),\n> \t\t \"--merge\", \"--conflict\", \"--staged\");\n\nLikewise.\n\n> +test_expect_success 'restore with merge options rejects --staged' '\n> +\ttest_must_fail git restore --staged --merge . -- 2>err1 &&\n\nWhat is \".\" meant to be on this command line? If it is \"the whole\nworking tree\", it should come after the double-dash \"--\", no? As\nwritten, I _think_ it is stripping \"--\" at the end, but \".\", which\nwas written before \"--\" to explicitly say \"this is not a pathspec\",\nis still taken as a pathspec (which may be a bug in the option\nparsing code).\n\n> +\ttest_i18ngrep \"cannot be used with\" err1 &&\n\n\"test_i18ngrep\" is on its way out (it was part of an older way for\ni18n testing that has been removed). We can use \"grep\" instead.\n\n> +\ttest_must_fail git restore --staged --conflict=diff3 . -- 2>err2 &&\n> +\ttest_i18ngrep \"cannot be used with\" err2 &&\n> +\ttest_must_fail git restore --staged --ours . -- 2>err3 &&\n> +\ttest_i18ngrep \"cannot be used with\" err3 &&\n> +\ttest_must_fail git restore --staged --theirs . -- 2>err4 &&\n> +\ttest_i18ngrep \"cannot be used with\" err4\n> +'\n\nNot making a suggestion yet, but thinking aloud. Would it make it\neasier to see what is being tested if we wrote these as a loop:\n\n\tfor opts in \\\n\t\t\"--staged --merge\" \\\n\t\t\"--staged --conflict=diff3\" \\\n\t\t\"--staged --ours\" \\\n\t\t\"--staged --theirs\"\n\tdo\n\t\ttest_must_fail git restore $opts 2>err &&\n\t\tgrep \"cannot be used with\" err || return\n\tdone\n\nWithout having to skip every alternating lines, we can see what\noption combinations are being tested fairly easily when written that\nway, perhaps?\n\n> +test_expect_success 'restore with merge options rejects --staged --worktree' '\n> +\ttest_must_fail git restore --staged --worktree --merge . -- 2>err1 &&\n> +\ttest_i18ngrep \"cannot be used with\" err1 &&\n> +\ttest_must_fail git restore --staged --worktree --conflict=diff3 . -- 2>err2 &&\n> +\ttest_i18ngrep \"cannot be used with\" err2 &&\n> +\ttest_must_fail git restore --staged --worktree --ours . -- 2>err3 &&\n> +\ttest_i18ngrep \"cannot be used with\" err3 &&\n> +\ttest_must_fail git restore --staged --worktree --theirs . -- 2>err4 &&\n> +\ttest_i18ngrep \"cannot be used with\" err4\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"472414","messageId":"CAHWeT-Zy=iJZoVAWZHqnGGFb-KMbg1fk_mmdE2T=K4+xRHpG0A@mail.gmail.com","threadId":"59267","inReplyTo":"xmqqa616g8yv.fsf@gitster.g","subject":"Re: [PATCH] restore: fault --staged --worktree with merge opts","fromName":"Andy Koppe","fromEmail":"andy.koppe@gmail.com","sentAt":"2023-02-21T22:27:27Z","receivedAt":"2023-02-21T22:27:42Z","isPatch":true,"sender":{"key":"andy.koppe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/223411?v=4"},"body":"On Tue, 21 Feb 2023 at 18:38, Junio C Hamano wrote:\n> Side note 2: it is conceivable that --worktree --staged\n> --ours may want to (1) resolve the conflicted path to stage\n> #2 in the index and (2) check out the result in the working\n> tree.\n\nSame with restore --worktree --staged --theirs and stage #3?\n\nThat's basically what I thought these combinations would do when I\nnoticed that they were accepted. I think they would be quite\nconvenient compared to separate restore and add. They'd be the\nequivalent of 'svn resolve --accept=mine-full/theirs-full'.\n\n> But until such an improved behaviour gets\n> implemented, it is probably better to error it out for now.\n\nIndeed. Unfortunately implementing that improvement is beyond my\nknowledge of git internals.\n\n>> +test_expect_success 'restore with merge options rejects --staged' '\n>> + test_must_fail git restore --staged --merge . -- 2>err1 &&\n\n> What is \".\" meant to be on this command line? If it is \"the whole\n> working tree\", it should come after the double-dash \"--\", no?\n\nSorry, that was accidental. The command requires a path argument to\nget to the option conflict error, but as you say, putting it before\nthe \"--\" doesn't make sense.\n\nThank you very much for the thorough review. I'll prepare a new\nversion of the patch.\n"},{"id":"472463","messageId":"xmqq356xcn5z.fsf@gitster.g","threadId":"59267","inReplyTo":"CAHWeT-Zy=iJZoVAWZHqnGGFb-KMbg1fk_mmdE2T=K4+xRHpG0A@mail.gmail.com","subject":"Re: [PATCH] restore: fault --staged --worktree with merge opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-22T23:09:44Z","receivedAt":"2023-02-22T23:09:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Koppe writes:\n\n> On Tue, 21 Feb 2023 at 18:38, Junio C Hamano wrote:\n>> Side note 2: it is conceivable that --worktree --staged\n>> --ours may want to (1) resolve the conflicted path to stage\n>> #2 in the index and (2) check out the result in the working\n>> tree.\n>\n> Same with restore --worktree --staged --theirs and stage #3?\n\nThese two work pretty much symmetrical, so the same story should\napply there, I would think.\n\n> Thank you very much for the thorough review. I'll prepare a new\n> version of the patch.\n\nThanks.\n"},{"id":"472758","messageId":"20230226184354.221-1-andy.koppe@gmail.com","threadId":"59267","inReplyTo":"xmqq356xcn5z.fsf@gitster.g","subject":"[PATCH v2] restore: fault --staged --worktree with merge opts","fromName":"Andy Koppe","fromEmail":"andy.koppe@gmail.com","sentAt":"2023-02-26T18:43:54Z","receivedAt":"2023-02-26T18:47:03Z","isPatch":true,"sender":{"key":"andy.koppe@gmail.com","avatar":"https://avatars.githubusercontent.com/u/223411?v=4"},"body":"The 'restore' command already rejects the --merge, --conflict, --ours\nand --theirs options when combined with --staged, but accepts them when\n--worktree is added as well.\n\nUnfortunately that doesn't appear to do anything useful. The --ours and\n--theirs options seem to be ignored when both --staged and --worktree\nare given, whereas with --merge or --conflict, the command has the same\neffect as if the --staged option wasn't present.\n\nSo reject those options with '--staged --worktree' as well, using\nopts->accept_ref to distinguish restore from checkout.\n\nAdd test for both '--staged' and '--staged --worktree'.\n\nSigned-off-by: Andy Koppe \n---\n\nCI: https://github.com/ak2/git/actions/runs/4276063110\n\n builtin/checkout.c | 29 +++++++++++++++++++++--------\n t/t2070-restore.sh | 16 ++++++++++++++++\n 2 files changed, 37 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a5155cf55c..17b179a797 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -489,15 +489,28 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\tdie(_(\"'%s' must be used when '%s' is not specified\"),\n \t\t \"--worktree\", \"--source\");\n \n-\tif (opts->checkout_index && !opts->checkout_worktree &&\n-\t opts->writeout_stage)\n-\t\tdie(_(\"'%s' or '%s' cannot be used with %s\"),\n-\t\t \"--ours\", \"--theirs\", \"--staged\");\n+\t/*\n+\t * Reject --staged option to the restore command when combined with\n+\t * merge-related options. Use the accept_ref flag to distinguish it\n+\t * from the checkout command, which does not accept --staged anyway.\n+\t *\n+\t * `restore --ours|--theirs --worktree --staged` could mean resolving\n+\t * conflicted paths to one side in both the worktree and the index,\n+\t * but does not currently.\n+\t *\n+\t * `restore --merge|--conflict=