{"thread":{"id":"53365","subject":"[PATCH 0/2] enhance \"git restore --worktree --staged\" behavior","startedAt":"2020-05-01T08:28:56Z","lastAt":"2020-05-05T04:44:55Z","messageCount":10,"participants":["Eric Sunshine","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"396731","messageId":"20200501082746.23943-1-sunshine@sunshineco.com","threadId":"53365","inReplyTo":null,"subject":"[PATCH 0/2] enhance \"git restore --worktree --staged\" behavior","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-01T08:27:44Z","receivedAt":"2020-05-01T08:28:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series addresses a shortcoming or two when git-restore's --worktree\nand --staged options are combined. The first patch tightens the\nimplementation to match the documentation. The second patch loosens the\ndocumented restriction by adding a bit of DWIMing to make it more\nconvenient to combine the two options.\n\nAlthough the second patch effectively throws away the changes of the\nfirst patch, I kept them separate in case someone comes up with a good\nobjection to the new DWIMing (which escaped me), in which case the first\npatch can be kept and the second thrown away.\n\nEric Sunshine (2):\n  restore: require --source when combining --worktree and --staged\n  restore: default to HEAD when combining --worktree and --staged\n\n Documentation/git-restore.txt | 13 +++++++------\n builtin/checkout.c            |  6 +++---\n t/t2070-restore.sh            | 11 +++++++++++\n 3 files changed, 21 insertions(+), 9 deletions(-)\n\n-- \n2.26.2.526.g744177e7f7\n\n"},{"id":"396732","messageId":"20200501082746.23943-2-sunshine@sunshineco.com","threadId":"53365","inReplyTo":"20200501082746.23943-1-sunshine@sunshineco.com","subject":"[PATCH 1/2] restore: require --source when combining --worktree and --staged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-01T08:27:45Z","receivedAt":"2020-05-01T08:28:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The default restore source for --worktree is the index, and the default\nsource for --staged is HEAD. When combining --worktree and --staged in\nthe same invocation, the restore source is ambiguous (\"should it restore\nfrom the index or from HEAD?\"). To avoid such ambiguity, the git-restore\ndocumentation has always stated that --source must be used when\ncombining --worktree and --staged. However, this restriction is not\nactually enforced. Address this deficiency by making the implementation\nmatch the documented behavior (to wit, error out if --source is not\nspecified when combining --worktree and --staged).\n\nWhile at it, enhance the documentation to mention the --source\nrequirement prominently in the \"Description\" section (rather than only\nin the description of the --source option itself).\n\nReported-by: Harri Mehtälä <harri.mehtala@finago.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/git-restore.txt | 3 ++-\n builtin/checkout.c            | 3 +++\n t/t2070-restore.sh            | 5 +++++\n 3 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\nindex 8e3b339802..8906499637 100644\n--- a/Documentation/git-restore.txt\n+++ b/Documentation/git-restore.txt\n@@ -24,7 +24,8 @@ The command can also be used to restore the content in the index with\n \n By default, the restore sources for working tree and the index are the\n index and `HEAD` respectively. `--source` could be used to specify a\n-commit as the restore source.\n+commit as the restore source; it is required when combining `--staged`\n+and `--worktree`.\n \n See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n between the three commands.\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8bc94d392b..7a01d00f53 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \tif (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n \t\tBUG(\"these flags should be non-negative by now\");\n+\tif (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n+\t    !opts->from_treeish)\n+\t\tdie(_(\"--source required when using --worktree and --staged\"));\n \t/*\n \t * convenient shortcut: \"git restore --staged\" equals\n \t * \"git restore --staged --source HEAD\"\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 076d0df7fc..19efa21fdb 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -69,6 +69,11 @@ test_expect_success 'restore --staged uses HEAD as source' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'restore --worktree --staged requires --source' '\n+\ttest_must_fail git restore --worktree --staged first.t 2>err &&\n+\ttest_i18ngrep \"source required when using --worktree and --staged\" err\n+'\n+\n test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n \tgit init unmerged &&\n \t(\n-- \n2.26.2.526.g744177e7f7\n\n"},{"id":"396733","messageId":"20200501082746.23943-3-sunshine@sunshineco.com","threadId":"53365","inReplyTo":"20200501082746.23943-1-sunshine@sunshineco.com","subject":"[PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-01T08:27:46Z","receivedAt":"2020-05-01T08:28:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The default restore source for --worktree is the index, and the default\nsource for --staged is HEAD. However, when combining --worktree and\n--staged in the same invocation, git-restore requires the source to be\nspecified explicitly via --source since it would otherwise be ambiguous\n(\"should it restore from the index or from HEAD?\"). This requirement\nmakes it cumbersome to restore a file in both the worktree and the\nindex.\n\nHowever, HEAD is also a reasonably intuitive default restore source when\n--worktree and --staged are combined. After all, if a user is asking to\nthrow away all local changes to a file (on disk and in the index)\nwithout specifying a restore source explicitly -- and the user expects\nthe file to be restored from _somewhere_ -- then it is likely that the\nuser expects them to be restored from HEAD, which is an intuitive and\nlogical place to find a recent unadulterated copy of the file.\n\nTherefore, make HEAD the default restore source when --worktree and\n--staged are combined.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/git-restore.txt | 14 +++++++-------\n builtin/checkout.c            |  9 +++------\n t/t2070-restore.sh            | 12 +++++++++---\n 3 files changed, 19 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\nindex 8906499637..5b61812e17 100644\n--- a/Documentation/git-restore.txt\n+++ b/Documentation/git-restore.txt\n@@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with\n `--staged`, or restore both the working tree and the index with\n `--staged --worktree`.\n \n-By default, the restore sources for working tree and the index are the\n-index and `HEAD` respectively. `--source` could be used to specify a\n-commit as the restore source; it is required when combining `--staged`\n-and `--worktree`.\n+By default, the restore source for `--worktree` is the index, and the\n+restore source for `--staged` is `HEAD`. When combining `--worktree` and\n+`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n+a different commit as the restore source.\n \n See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n between the three commands.\n@@ -40,10 +40,10 @@ OPTIONS\n \ttree. It is common to specify the source tree by naming a\n \tcommit, branch or tag associated with it.\n +\n-If not specified, the default restore source for the working tree is\n-the index, and the default restore source for the index is\n+If not specified, the default restore source for `--worktree` is\n+the index, and the default restore source for `--staged` is\n `HEAD`. When both `--staged` and `--worktree` are specified,\n-`--source` must also be specified.\n+the default restore source is `HEAD`.\n \n -p::\n --patch::\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7a01d00f53..500c3e23ff 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \tif (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n \t\tBUG(\"these flags should be non-negative by now\");\n-\tif (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n-\t    !opts->from_treeish)\n-\t\tdie(_(\"--source required when using --worktree and --staged\"));\n \t/*\n-\t * convenient shortcut: \"git restore --staged\" equals\n-\t * \"git restore --staged --source HEAD\"\n+\t * convenient shortcut: \"git restore --staged [--worktree]\" equals\n+\t * \"git restore --staged [--worktree] --source HEAD\"\n \t */\n-\tif (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)\n+\tif (!opts->from_treeish && opts->checkout_index)\n \t\topts->from_treeish = \"HEAD\";\n \n \t/*\ndiff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\nindex 19efa21fdb..89e5a142c9 100755\n--- a/t/t2070-restore.sh\n+++ b/t/t2070-restore.sh\n@@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'restore --worktree --staged requires --source' '\n-\ttest_must_fail git restore --worktree --staged first.t 2>err &&\n-\ttest_i18ngrep \"source required when using --worktree and --staged\" err\n+test_expect_success 'restore --worktree --staged uses HEAD as source' '\n+\ttest_when_finished git reset --hard &&\n+\tgit show HEAD:./first.t >expected &&\n+\techo dirty >>first.t &&\n+\tgit add first.t &&\n+\tgit restore --worktree --staged first.t &&\n+\tgit show :./first.t >actual &&\n+\ttest_cmp expected actual &&\n+\ttest_cmp expected first.t\n '\n \n test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n-- \n2.26.2.526.g744177e7f7\n\n"},{"id":"396735","messageId":"CAPig+cR4VpyAzqok8+QoUgQQc=mS-ooD8+PUCPMuO6XQ21dL6w@mail.gmail.com","threadId":"53365","inReplyTo":"20200501082746.23943-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/2] restore: require --source when combining --worktree and --staged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-01T08:49:50Z","receivedAt":"2020-05-01T08:50:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 1, 2020 at 4:28 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> @@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>         }\n>         if (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n>                 BUG(\"these flags should be non-negative by now\");\n> +       if (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n> +           !opts->from_treeish)\n\nIf I re-roll the series for some reason, I'll drop the unnecessary \">\n0\". (But this code goes away in patch 2/2 anyhow, so I'm not too\nworried about it.)\n"},{"id":"396768","messageId":"xmqqimhfoelf.fsf@gitster.c.googlers.com","threadId":"53365","inReplyTo":"20200501082746.23943-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T15:32:44Z","receivedAt":"2020-05-01T15:32:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> The default restore source for --worktree is the index, and the default\n> source for --staged is HEAD. However, when combining --worktree and\n> --staged in the same invocation, git-restore requires the source to be\n> specified explicitly via --source since it would otherwise be ambiguous\n> (\"should it restore from the index or from HEAD?\"). This requirement\n> makes it cumbersome to restore a file in both the worktree and the\n> index.\n>\n> However, HEAD is also a reasonably intuitive default restore source when\n> --worktree and --staged are combined. After all, if a user is asking to\n> throw away all local changes to a file (on disk and in the index)\n> without specifying a restore source explicitly -- and the user expects\n> the file to be restored from _somewhere_ -- then it is likely that the\n> user expects them to be restored from HEAD, which is an intuitive and\n> logical place to find a recent unadulterated copy of the file.\n>\n> Therefore, make HEAD the default restore source when --worktree and\n> --staged are combined.\n\nThe mention in the second paragraph that you are dealing with the\ncase where you are updating both the index and the working tree is\nacceptable.  It explains why HEAD is a reasonable default in that\ncase.  But the third paragraph is totally redundant.  \n\nI also found that these two paragraphs a bit too long, and by the\ntime I finished reading them I forgot that you mentioned that HEAD\nis the default when --staged is given.  It ended up giving me \"ok,\nyou made the default to HEAD when both are given; but what about the\ncase when only --staged is given?\" reaction X-<.\n\n    ... This requirement makes it cumbersome to restore a file in\n    both the worktree and the index.\n\n    As we are *not* going to restore the index and the working tree\n    from two different sources, we need to pick a single default\n    when both options are given, and the default used for restoring\n    the index, HEAD, is a reasonable one in this case, too.  Another\n    plausible source might be the index, but that does not make any\n    sense to the user who explicitly gave the `--staged` option.\n\n    So, make HEAD the default source when --staged is given, whether\n    --worktree is used at the same time.\n\nperhaps?\n\n> +By default, the restore source for `--worktree` is the index, and the\n> +restore source for `--staged` is `HEAD`. When combining `--worktree` and\n> +`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n> +a different commit as the restore source.\n\nClear enough, but I wonder if we can simplify it even further.\n\n    By default, if `--staged` is given, the contents are restored\n    from `HEAD`.  Otherwise, the contents are restored from the\n    index.\n\nbecause `--worktree` is the default for the command when neither\n`--staged` or `--worktree` is given.\n\n> @@ -40,10 +40,10 @@ OPTIONS\n>  \ttree. It is common to specify the source tree by naming a\n>  \tcommit, branch or tag associated with it.\n>  +\n> -If not specified, the default restore source for the working tree is\n> -the index, and the default restore source for the index is\n> +If not specified, the default restore source for `--worktree` is\n> +the index, and the default restore source for `--staged` is\n\nLikewise.\n\n>  `HEAD`. When both `--staged` and `--worktree` are specified,\n> -`--source` must also be specified.\n> +the default restore source is `HEAD`.\n>  \n>  -p::\n>  --patch::\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 7a01d00f53..500c3e23ff 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t}\n>  \tif (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n>  \t\tBUG(\"these flags should be non-negative by now\");\n> -\tif (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n> -\t    !opts->from_treeish)\n> -\t\tdie(_(\"--source required when using --worktree and --staged\"));\n>  \t/*\n> -\t * convenient shortcut: \"git restore --staged\" equals\n> -\t * \"git restore --staged --source HEAD\"\n> +\t * convenient shortcut: \"git restore --staged [--worktree]\" equals\n> +\t * \"git restore --staged [--worktree] --source HEAD\"\n>  \t */\n\nGood.\n\n\n> -\tif (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)\n> +\tif (!opts->from_treeish && opts->checkout_index)\n>  \t\topts->from_treeish = \"HEAD\";\n\nThis succinctly tells the gist of this change.  \n\nWhen source is not given, the default is HEAD when we are updating\nthe index, regardless of any other condition.\n\nGood.\n\n> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\n> index 19efa21fdb..89e5a142c9 100755\n> --- a/t/t2070-restore.sh\n> +++ b/t/t2070-restore.sh\n> @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> -test_expect_success 'restore --worktree --staged requires --source' '\n> -\ttest_must_fail git restore --worktree --staged first.t 2>err &&\n> -\ttest_i18ngrep \"source required when using --worktree and --staged\" err\n> +test_expect_success 'restore --worktree --staged uses HEAD as source' '\n> +\ttest_when_finished git reset --hard &&\n> +\tgit show HEAD:./first.t >expected &&\n> +\techo dirty >>first.t &&\n> +\tgit add first.t &&\n> +\tgit restore --worktree --staged first.t &&\n> +\tgit show :./first.t >actual &&\n> +\ttest_cmp expected actual &&\n> +\ttest_cmp expected first.t\n>  '\n\nQuite straight-forward and makes sense.\n"},{"id":"396793","messageId":"20200501221613.GC41612@syl.local","threadId":"53365","inReplyTo":"20200501082746.23943-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/2] restore: require --source when combining --worktree and --staged","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-01T22:16:13Z","receivedAt":"2020-05-01T22:16:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 01, 2020 at 04:27:45AM -0400, Eric Sunshine wrote:\n> The default restore source for --worktree is the index, and the default\n> source for --staged is HEAD. When combining --worktree and --staged in\n> the same invocation, the restore source is ambiguous (\"should it restore\n> from the index or from HEAD?\"). To avoid such ambiguity, the git-restore\n> documentation has always stated that --source must be used when\n> combining --worktree and --staged. However, this restriction is not\n> actually enforced. Address this deficiency by making the implementation\n> match the documented behavior (to wit, error out if --source is not\n> specified when combining --worktree and --staged).\n\nThis explanation is very helpful, and makes the fix below very natural.\nThanks for a helpful explanation.\n\n> While at it, enhance the documentation to mention the --source\n> requirement prominently in the \"Description\" section (rather than only\n> in the description of the --source option itself).\n>\n> Reported-by: Harri Mehtälä <harri.mehtala@finago.com>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  Documentation/git-restore.txt | 3 ++-\n>  builtin/checkout.c            | 3 +++\n>  t/t2070-restore.sh            | 5 +++++\n>  3 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\n> index 8e3b339802..8906499637 100644\n> --- a/Documentation/git-restore.txt\n> +++ b/Documentation/git-restore.txt\n> @@ -24,7 +24,8 @@ The command can also be used to restore the content in the index with\n>\n>  By default, the restore sources for working tree and the index are the\n>  index and `HEAD` respectively. `--source` could be used to specify a\n> -commit as the restore source.\n> +commit as the restore source; it is required when combining `--staged`\n> +and `--worktree`.\n>\n>  See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n>  between the three commands.\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 8bc94d392b..7a01d00f53 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1604,6 +1604,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t}\n>  \tif (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n>  \t\tBUG(\"these flags should be non-negative by now\");\n> +\tif (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n> +\t    !opts->from_treeish)\n> +\t\tdie(_(\"--source required when using --worktree and --staged\"));\n>  \t/*\n>  \t * convenient shortcut: \"git restore --staged\" equals\n>  \t * \"git restore --staged --source HEAD\"\n> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\n> index 076d0df7fc..19efa21fdb 100755\n> --- a/t/t2070-restore.sh\n> +++ b/t/t2070-restore.sh\n> @@ -69,6 +69,11 @@ test_expect_success 'restore --staged uses HEAD as source' '\n>  \ttest_cmp expected actual\n>  '\n>\n> +test_expect_success 'restore --worktree --staged requires --source' '\n> +\ttest_must_fail git restore --worktree --staged first.t 2>err &&\n> +\ttest_i18ngrep \"source required when using --worktree and --staged\" err\n> +'\n> +\n>  test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n>  \tgit init unmerged &&\n>  \t(\n> --\n> 2.26.2.526.g744177e7f7\n\nVery sane.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"396794","messageId":"20200501221951.GD41612@syl.local","threadId":"53365","inReplyTo":"20200501082746.23943-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-01T22:19:51Z","receivedAt":"2020-05-01T22:19:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:\n> The default restore source for --worktree is the index, and the default\n> source for --staged is HEAD. However, when combining --worktree and\n\nI think that you could very reasonably drop the first sentence here,\nespecially because it is repeated verbatim from the previous commit.\n\nIn fact... this whole paragraph looks similar to me. Maybe just:\n\n  When invoking 'git restore' with both '--worktree' and '--staged', it\n  is required that the ambiguity of which source to restore from be\n  resolved by also passing '--source'.\n\n> --staged in the same invocation, git-restore requires the source to be\n> specified explicitly via --source since it would otherwise be ambiguous\n> (\"should it restore from the index or from HEAD?\"). This requirement\n> makes it cumbersome to restore a file in both the worktree and the\n> index.\n>\n> However, HEAD is also a reasonably intuitive default restore source when\n> --worktree and --staged are combined. After all, if a user is asking to\n> throw away all local changes to a file (on disk and in the index)\n> without specifying a restore source explicitly -- and the user expects\n> the file to be restored from _somewhere_ -- then it is likely that the\n> user expects them to be restored from HEAD, which is an intuitive and\n> logical place to find a recent unadulterated copy of the file.\n>\n> Therefore, make HEAD the default restore source when --worktree and\n> --staged are combined.\n\nI think that this is a very sensible default choice here, so I am OK\nwith this second patch, too.\n\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  Documentation/git-restore.txt | 14 +++++++-------\n>  builtin/checkout.c            |  9 +++------\n>  t/t2070-restore.sh            | 12 +++++++++---\n>  3 files changed, 19 insertions(+), 16 deletions(-)\n>\n> diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt\n> index 8906499637..5b61812e17 100644\n> --- a/Documentation/git-restore.txt\n> +++ b/Documentation/git-restore.txt\n> @@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with\n>  `--staged`, or restore both the working tree and the index with\n>  `--staged --worktree`.\n>\n> -By default, the restore sources for working tree and the index are the\n> -index and `HEAD` respectively. `--source` could be used to specify a\n> -commit as the restore source; it is required when combining `--staged`\n> -and `--worktree`.\n> +By default, the restore source for `--worktree` is the index, and the\n> +restore source for `--staged` is `HEAD`. When combining `--worktree` and\n> +`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n\nThis is extremely nit-pick-y, but is this line a little over-long? My\nmemory is that Documentation should be wrapped at 72 characters instead\nof 80. I culd be totally wrong.\n\n> +a different commit as the restore source.\n>\n>  See \"Reset, restore and revert\" in linkgit:git[1] for the differences\n>  between the three commands.\n> @@ -40,10 +40,10 @@ OPTIONS\n>  \ttree. It is common to specify the source tree by naming a\n>  \tcommit, branch or tag associated with it.\n>  +\n> -If not specified, the default restore source for the working tree is\n> -the index, and the default restore source for the index is\n> +If not specified, the default restore source for `--worktree` is\n> +the index, and the default restore source for `--staged` is\n>  `HEAD`. When both `--staged` and `--worktree` are specified,\n> -`--source` must also be specified.\n> +the default restore source is `HEAD`.\n>\n>  -p::\n>  --patch::\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 7a01d00f53..500c3e23ff 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>  \t}\n>  \tif (opts->checkout_index < 0 || opts->checkout_worktree < 0)\n>  \t\tBUG(\"these flags should be non-negative by now\");\n> -\tif (opts->checkout_index > 0 && opts->checkout_worktree > 0 &&\n> -\t    !opts->from_treeish)\n> -\t\tdie(_(\"--source required when using --worktree and --staged\"));\n>  \t/*\n> -\t * convenient shortcut: \"git restore --staged\" equals\n> -\t * \"git restore --staged --source HEAD\"\n> +\t * convenient shortcut: \"git restore --staged [--worktree]\" equals\n> +\t * \"git restore --staged [--worktree] --source HEAD\"\n>  \t */\n> -\tif (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree)\n> +\tif (!opts->from_treeish && opts->checkout_index)\n>  \t\topts->from_treeish = \"HEAD\";\n>\n>  \t/*\n> diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh\n> index 19efa21fdb..89e5a142c9 100755\n> --- a/t/t2070-restore.sh\n> +++ b/t/t2070-restore.sh\n> @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' '\n>  \ttest_cmp expected actual\n>  '\n>\n> -test_expect_success 'restore --worktree --staged requires --source' '\n> -\ttest_must_fail git restore --worktree --staged first.t 2>err &&\n> -\ttest_i18ngrep \"source required when using --worktree and --staged\" err\n> +test_expect_success 'restore --worktree --staged uses HEAD as source' '\n> +\ttest_when_finished git reset --hard &&\n> +\tgit show HEAD:./first.t >expected &&\n> +\techo dirty >>first.t &&\n> +\tgit add first.t &&\n> +\tgit restore --worktree --staged first.t &&\n> +\tgit show :./first.t >actual &&\n> +\ttest_cmp expected actual &&\n> +\ttest_cmp expected first.t\n>  '\n>\n>  test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '\n> --\n> 2.26.2.526.g744177e7f7\n\nAll looks good to me, here, too.\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"397043","messageId":"CAPig+cT62cEx9Zk+DN02xGfAmqZD258Toa3PArSxEf8QDFeDww@mail.gmail.com","threadId":"53365","inReplyTo":"xmqqimhfoelf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-05T03:53:31Z","receivedAt":"2020-05-05T03:53:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 1, 2020 at 11:33 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > [...] This requirement\n> > makes it cumbersome to restore a file in both the worktree and the\n> > index.\n> >\n> > However, HEAD is also a reasonably intuitive default restore source when\n> > --worktree and --staged are combined. After all, if a user is asking to\n> > throw away all local changes to a file (on disk and in the index)\n> > without specifying a restore source explicitly -- and the user expects\n> > the file to be restored from _somewhere_ -- then it is likely that the\n> > user expects them to be restored from HEAD, which is an intuitive and\n> > logical place to find a recent unadulterated copy of the file.\n>\n> I also found that these two paragraphs a bit too long, and by the\n> time I finished reading them I forgot that you mentioned that HEAD\n> is the default when --staged is given. [...]\n>\n>   ... This requirement makes it cumbersome to restore a file in\n>   both the worktree and the index.\n>\n>   As we are *not* going to restore the index and the working tree\n>   from two different sources, we need to pick a single default\n>   when both options are given, and the default used for restoring\n>   the index, HEAD, is a reasonable one in this case, too. Another\n>   plausible source might be the index, but that does not make any\n>   sense to the user who explicitly gave the `--staged` option.\n>\n>   So, make HEAD the default source when --staged is given, whether\n>   --worktree is used at the same time.\n\nThanks. I was having trouble writing it without being overly\nsubjective (and using words like \"intuitive\" and \"logical\"). I'll\nprobably drop the \"Another plausible...\" bit from the rewrite, though.\n\n> > +By default, the restore source for `--worktree` is the index, and the\n> > +restore source for `--staged` is `HEAD`. When combining `--worktree` and\n> > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n> > +a different commit as the restore source.\n>\n> Clear enough, but I wonder if we can simplify it even further.\n>\n>   By default, if `--staged` is given, the contents are restored\n>   from `HEAD`. Otherwise, the contents are restored from the\n>   index.\n>\n> because `--worktree` is the default for the command when neither\n> `--staged` or `--worktree` is given.\n\nMakes sense. I wasn't terribly happy with it either.\n"},{"id":"397044","messageId":"CAPig+cTAab6E4TPLL5AA75Ss9hQxCo2ZKLQaDakO=gGPvse+cQ@mail.gmail.com","threadId":"53365","inReplyTo":"20200501221951.GD41612@syl.local","subject":"Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-05T04:00:08Z","receivedAt":"2020-05-05T04:00:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote:\n> On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:\n> > The default restore source for --worktree is the index, and the default\n> > source for --staged is HEAD. However, when combining --worktree and\n>\n> I think that you could very reasonably drop the first sentence here,\n> especially because it is repeated verbatim from the previous commit.\n\nThe repetition is intentional so that each commit can be understood\nstand-alone (without having to know what came before it).\n\n> In fact... this whole paragraph looks similar to me. Maybe just:\n>\n>  When invoking 'git restore' with both '--worktree' and '--staged', it\n>  is required that the ambiguity of which source to restore from be\n>  resolved by also passing '--source'.\n\nI'll see if I can trim it down a bit -- Junio also found it too long.\n\n> > -By default, the restore sources for working tree and the index are the\n> > -index and `HEAD` respectively. `--source` could be used to specify a\n> > -commit as the restore source; it is required when combining `--staged`\n> > -and `--worktree`.\n> > +By default, the restore source for `--worktree` is the index, and the\n> > +restore source for `--staged` is `HEAD`. When combining `--worktree` and\n> > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n>\n> This is extremely nit-pick-y, but is this line a little over-long? My\n> memory is that Documentation should be wrapped at 72 characters instead\n> of 80. I culd be totally wrong.\n\nColumn 72 for commit messages, certainly, but I don't think there is\nany such guideline about documentation also being wrapped at 72. As an\nold-schooler who still uses 80-column terminal and editor windows, I'm\nquite sensitive to line length -- these lines are wrapped at 79.\n"},{"id":"397047","messageId":"20200505044449.GA67132@syl.local","threadId":"53365","inReplyTo":"CAPig+cTAab6E4TPLL5AA75Ss9hQxCo2ZKLQaDakO=gGPvse+cQ@mail.gmail.com","subject":"Re: [PATCH 2/2] restore: default to HEAD when combining --worktree and --staged","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-05T04:44:49Z","receivedAt":"2020-05-05T04:44:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 05, 2020 at 12:00:08AM -0400, Eric Sunshine wrote:\n> On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote:\n> > On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote:\n> > > The default restore source for --worktree is the index, and the default\n> > > source for --staged is HEAD. However, when combining --worktree and\n> >\n> > I think that you could very reasonably drop the first sentence here,\n> > especially because it is repeated verbatim from the previous commit.\n>\n> The repetition is intentional so that each commit can be understood\n> stand-alone (without having to know what came before it).\n\nFair enough; I figure that the commits will probably be read most often\nin conjunction with one another, but a little bit of extra commentary\ndoesn't hurt, either.\n\n> > In fact... this whole paragraph looks similar to me. Maybe just:\n> >\n> >  When invoking 'git restore' with both '--worktree' and '--staged', it\n> >  is required that the ambiguity of which source to restore from be\n> >  resolved by also passing '--source'.\n>\n> I'll see if I can trim it down a bit -- Junio also found it too long.\n\nThanks. I'm happy to read whatever you have once you're ready.\n\n> > > -By default, the restore sources for working tree and the index are the\n> > > -index and `HEAD` respectively. `--source` could be used to specify a\n> > > -commit as the restore source; it is required when combining `--staged`\n> > > -and `--worktree`.\n> > > +By default, the restore source for `--worktree` is the index, and the\n> > > +restore source for `--staged` is `HEAD`. When combining `--worktree` and\n> > > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify\n> >\n> > This is extremely nit-pick-y, but is this line a little over-long? My\n> > memory is that Documentation should be wrapped at 72 characters instead\n> > of 80. I culd be totally wrong.\n>\n> Column 72 for commit messages, certainly, but I don't think there is\n> any such guideline about documentation also being wrapped at 72. As an\n> old-schooler who still uses 80-column terminal and editor windows, I'm\n> quite sensitive to line length -- these lines are wrapped at 79.\n\nHmm... I've always wrapped changes in the Documentation tree at 72\ncharacters, but I could very easily be in the wrong there ;). I tried to\nfind something in Documentation about wrapping at 72 characters, but I\nfailed. So, please disregard my original suggestion, and sorry for the\ntrouble there.\n\n\nThanks,\nTaylor\n"}]}