| Message ID | 20200501082746.23943-3-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enhance "git restore --worktree --staged" behavior | expand |
Eric Sunshine <sunshine@sunshineco.com> writes: > The default restore source for --worktree is the index, and the default > source for --staged is HEAD. However, when combining --worktree and > --staged in the same invocation, git-restore requires the source to be > specified explicitly via --source since it would otherwise be ambiguous > ("should it restore from the index or from HEAD?"). This requirement > makes it cumbersome to restore a file in both the worktree and the > index. > > However, HEAD is also a reasonably intuitive default restore source when > --worktree and --staged are combined. After all, if a user is asking to > throw away all local changes to a file (on disk and in the index) > without specifying a restore source explicitly -- and the user expects > the file to be restored from _somewhere_ -- then it is likely that the > user expects them to be restored from HEAD, which is an intuitive and > logical place to find a recent unadulterated copy of the file. > > Therefore, make HEAD the default restore source when --worktree and > --staged are combined. The mention in the second paragraph that you are dealing with the case where you are updating both the index and the working tree is acceptable. It explains why HEAD is a reasonable default in that case. But the third paragraph is totally redundant. I also found that these two paragraphs a bit too long, and by the time I finished reading them I forgot that you mentioned that HEAD is the default when --staged is given. It ended up giving me "ok, you made the default to HEAD when both are given; but what about the case when only --staged is given?" reaction X-<. ... This requirement makes it cumbersome to restore a file in both the worktree and the index. As we are *not* going to restore the index and the working tree from two different sources, we need to pick a single default when both options are given, and the default used for restoring the index, HEAD, is a reasonable one in this case, too. Another plausible source might be the index, but that does not make any sense to the user who explicitly gave the `--staged` option. So, make HEAD the default source when --staged is given, whether --worktree is used at the same time. perhaps? > +By default, the restore source for `--worktree` is the index, and the > +restore source for `--staged` is `HEAD`. When combining `--worktree` and > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify > +a different commit as the restore source. Clear enough, but I wonder if we can simplify it even further. By default, if `--staged` is given, the contents are restored from `HEAD`. Otherwise, the contents are restored from the index. because `--worktree` is the default for the command when neither `--staged` or `--worktree` is given. > @@ -40,10 +40,10 @@ OPTIONS > tree. It is common to specify the source tree by naming a > commit, branch or tag associated with it. > + > -If not specified, the default restore source for the working tree is > -the index, and the default restore source for the index is > +If not specified, the default restore source for `--worktree` is > +the index, and the default restore source for `--staged` is Likewise. > `HEAD`. When both `--staged` and `--worktree` are specified, > -`--source` must also be specified. > +the default restore source is `HEAD`. > > -p:: > --patch:: > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 7a01d00f53..500c3e23ff 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > } > if (opts->checkout_index < 0 || opts->checkout_worktree < 0) > BUG("these flags should be non-negative by now"); > - if (opts->checkout_index > 0 && opts->checkout_worktree > 0 && > - !opts->from_treeish) > - die(_("--source required when using --worktree and --staged")); > /* > - * convenient shortcut: "git restore --staged" equals > - * "git restore --staged --source HEAD" > + * convenient shortcut: "git restore --staged [--worktree]" equals > + * "git restore --staged [--worktree] --source HEAD" > */ Good. > - if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree) > + if (!opts->from_treeish && opts->checkout_index) > opts->from_treeish = "HEAD"; This succinctly tells the gist of this change. When source is not given, the default is HEAD when we are updating the index, regardless of any other condition. Good. > diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh > index 19efa21fdb..89e5a142c9 100755 > --- a/t/t2070-restore.sh > +++ b/t/t2070-restore.sh > @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' ' > test_cmp expected actual > ' > > -test_expect_success 'restore --worktree --staged requires --source' ' > - test_must_fail git restore --worktree --staged first.t 2>err && > - test_i18ngrep "source required when using --worktree and --staged" err > +test_expect_success 'restore --worktree --staged uses HEAD as source' ' > + test_when_finished git reset --hard && > + git show HEAD:./first.t >expected && > + echo dirty >>first.t && > + git add first.t && > + git restore --worktree --staged first.t && > + git show :./first.t >actual && > + test_cmp expected actual && > + test_cmp expected first.t > ' Quite straight-forward and makes sense.
On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote: > The default restore source for --worktree is the index, and the default > source for --staged is HEAD. However, when combining --worktree and I think that you could very reasonably drop the first sentence here, especially because it is repeated verbatim from the previous commit. In fact... this whole paragraph looks similar to me. Maybe just: When invoking 'git restore' with both '--worktree' and '--staged', it is required that the ambiguity of which source to restore from be resolved by also passing '--source'. > --staged in the same invocation, git-restore requires the source to be > specified explicitly via --source since it would otherwise be ambiguous > ("should it restore from the index or from HEAD?"). This requirement > makes it cumbersome to restore a file in both the worktree and the > index. > > However, HEAD is also a reasonably intuitive default restore source when > --worktree and --staged are combined. After all, if a user is asking to > throw away all local changes to a file (on disk and in the index) > without specifying a restore source explicitly -- and the user expects > the file to be restored from _somewhere_ -- then it is likely that the > user expects them to be restored from HEAD, which is an intuitive and > logical place to find a recent unadulterated copy of the file. > > Therefore, make HEAD the default restore source when --worktree and > --staged are combined. I think that this is a very sensible default choice here, so I am OK with this second patch, too. > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > Documentation/git-restore.txt | 14 +++++++------- > builtin/checkout.c | 9 +++------ > t/t2070-restore.sh | 12 +++++++++--- > 3 files changed, 19 insertions(+), 16 deletions(-) > > diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt > index 8906499637..5b61812e17 100644 > --- a/Documentation/git-restore.txt > +++ b/Documentation/git-restore.txt > @@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with > `--staged`, or restore both the working tree and the index with > `--staged --worktree`. > > -By default, the restore sources for working tree and the index are the > -index and `HEAD` respectively. `--source` could be used to specify a > -commit as the restore source; it is required when combining `--staged` > -and `--worktree`. > +By default, the restore source for `--worktree` is the index, and the > +restore source for `--staged` is `HEAD`. When combining `--worktree` and > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify This is extremely nit-pick-y, but is this line a little over-long? My memory is that Documentation should be wrapped at 72 characters instead of 80. I culd be totally wrong. > +a different commit as the restore source. > > See "Reset, restore and revert" in linkgit:git[1] for the differences > between the three commands. > @@ -40,10 +40,10 @@ OPTIONS > tree. It is common to specify the source tree by naming a > commit, branch or tag associated with it. > + > -If not specified, the default restore source for the working tree is > -the index, and the default restore source for the index is > +If not specified, the default restore source for `--worktree` is > +the index, and the default restore source for `--staged` is > `HEAD`. When both `--staged` and `--worktree` are specified, > -`--source` must also be specified. > +the default restore source is `HEAD`. > > -p:: > --patch:: > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 7a01d00f53..500c3e23ff 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > } > if (opts->checkout_index < 0 || opts->checkout_worktree < 0) > BUG("these flags should be non-negative by now"); > - if (opts->checkout_index > 0 && opts->checkout_worktree > 0 && > - !opts->from_treeish) > - die(_("--source required when using --worktree and --staged")); > /* > - * convenient shortcut: "git restore --staged" equals > - * "git restore --staged --source HEAD" > + * convenient shortcut: "git restore --staged [--worktree]" equals > + * "git restore --staged [--worktree] --source HEAD" > */ > - if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree) > + if (!opts->from_treeish && opts->checkout_index) > opts->from_treeish = "HEAD"; > > /* > diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh > index 19efa21fdb..89e5a142c9 100755 > --- a/t/t2070-restore.sh > +++ b/t/t2070-restore.sh > @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' ' > test_cmp expected actual > ' > > -test_expect_success 'restore --worktree --staged requires --source' ' > - test_must_fail git restore --worktree --staged first.t 2>err && > - test_i18ngrep "source required when using --worktree and --staged" err > +test_expect_success 'restore --worktree --staged uses HEAD as source' ' > + test_when_finished git reset --hard && > + git show HEAD:./first.t >expected && > + echo dirty >>first.t && > + git add first.t && > + git restore --worktree --staged first.t && > + git show :./first.t >actual && > + test_cmp expected actual && > + test_cmp expected first.t > ' > > test_expect_success 'restore --ignore-unmerged ignores unmerged entries' ' > -- > 2.26.2.526.g744177e7f7 All looks good to me, here, too. Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
On Fri, May 1, 2020 at 11:33 AM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > [...] This requirement > > makes it cumbersome to restore a file in both the worktree and the > > index. > > > > However, HEAD is also a reasonably intuitive default restore source when > > --worktree and --staged are combined. After all, if a user is asking to > > throw away all local changes to a file (on disk and in the index) > > without specifying a restore source explicitly -- and the user expects > > the file to be restored from _somewhere_ -- then it is likely that the > > user expects them to be restored from HEAD, which is an intuitive and > > logical place to find a recent unadulterated copy of the file. > > I also found that these two paragraphs a bit too long, and by the > time I finished reading them I forgot that you mentioned that HEAD > is the default when --staged is given. [...] > > ... This requirement makes it cumbersome to restore a file in > both the worktree and the index. > > As we are *not* going to restore the index and the working tree > from two different sources, we need to pick a single default > when both options are given, and the default used for restoring > the index, HEAD, is a reasonable one in this case, too. Another > plausible source might be the index, but that does not make any > sense to the user who explicitly gave the `--staged` option. > > So, make HEAD the default source when --staged is given, whether > --worktree is used at the same time. Thanks. I was having trouble writing it without being overly subjective (and using words like "intuitive" and "logical"). I'll probably drop the "Another plausible..." bit from the rewrite, though. > > +By default, the restore source for `--worktree` is the index, and the > > +restore source for `--staged` is `HEAD`. When combining `--worktree` and > > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify > > +a different commit as the restore source. > > Clear enough, but I wonder if we can simplify it even further. > > By default, if `--staged` is given, the contents are restored > from `HEAD`. Otherwise, the contents are restored from the > index. > > because `--worktree` is the default for the command when neither > `--staged` or `--worktree` is given. Makes sense. I wasn't terribly happy with it either.
On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote: > On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote: > > The default restore source for --worktree is the index, and the default > > source for --staged is HEAD. However, when combining --worktree and > > I think that you could very reasonably drop the first sentence here, > especially because it is repeated verbatim from the previous commit. The repetition is intentional so that each commit can be understood stand-alone (without having to know what came before it). > In fact... this whole paragraph looks similar to me. Maybe just: > > When invoking 'git restore' with both '--worktree' and '--staged', it > is required that the ambiguity of which source to restore from be > resolved by also passing '--source'. I'll see if I can trim it down a bit -- Junio also found it too long. > > -By default, the restore sources for working tree and the index are the > > -index and `HEAD` respectively. `--source` could be used to specify a > > -commit as the restore source; it is required when combining `--staged` > > -and `--worktree`. > > +By default, the restore source for `--worktree` is the index, and the > > +restore source for `--staged` is `HEAD`. When combining `--worktree` and > > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify > > This is extremely nit-pick-y, but is this line a little over-long? My > memory is that Documentation should be wrapped at 72 characters instead > of 80. I culd be totally wrong. Column 72 for commit messages, certainly, but I don't think there is any such guideline about documentation also being wrapped at 72. As an old-schooler who still uses 80-column terminal and editor windows, I'm quite sensitive to line length -- these lines are wrapped at 79.
On Tue, May 05, 2020 at 12:00:08AM -0400, Eric Sunshine wrote: > On Fri, May 1, 2020 at 6:20 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, May 01, 2020 at 04:27:46AM -0400, Eric Sunshine wrote: > > > The default restore source for --worktree is the index, and the default > > > source for --staged is HEAD. However, when combining --worktree and > > > > I think that you could very reasonably drop the first sentence here, > > especially because it is repeated verbatim from the previous commit. > > The repetition is intentional so that each commit can be understood > stand-alone (without having to know what came before it). Fair enough; I figure that the commits will probably be read most often in conjunction with one another, but a little bit of extra commentary doesn't hurt, either. > > In fact... this whole paragraph looks similar to me. Maybe just: > > > > When invoking 'git restore' with both '--worktree' and '--staged', it > > is required that the ambiguity of which source to restore from be > > resolved by also passing '--source'. > > I'll see if I can trim it down a bit -- Junio also found it too long. Thanks. I'm happy to read whatever you have once you're ready. > > > -By default, the restore sources for working tree and the index are the > > > -index and `HEAD` respectively. `--source` could be used to specify a > > > -commit as the restore source; it is required when combining `--staged` > > > -and `--worktree`. > > > +By default, the restore source for `--worktree` is the index, and the > > > +restore source for `--staged` is `HEAD`. When combining `--worktree` and > > > +`--staged`, the restore source is `HEAD`. `--source` can be used to specify > > > > This is extremely nit-pick-y, but is this line a little over-long? My > > memory is that Documentation should be wrapped at 72 characters instead > > of 80. I culd be totally wrong. > > Column 72 for commit messages, certainly, but I don't think there is > any such guideline about documentation also being wrapped at 72. As an > old-schooler who still uses 80-column terminal and editor windows, I'm > quite sensitive to line length -- these lines are wrapped at 79. Hmm... I've always wrapped changes in the Documentation tree at 72 characters, but I could very easily be in the wrong there ;). I tried to find something in Documentation about wrapping at 72 characters, but I failed. So, please disregard my original suggestion, and sorry for the trouble there. Thanks, Taylor
diff --git a/Documentation/git-restore.txt b/Documentation/git-restore.txt index 8906499637..5b61812e17 100644 --- a/Documentation/git-restore.txt +++ b/Documentation/git-restore.txt @@ -22,10 +22,10 @@ The command can also be used to restore the content in the index with `--staged`, or restore both the working tree and the index with `--staged --worktree`. -By default, the restore sources for working tree and the index are the -index and `HEAD` respectively. `--source` could be used to specify a -commit as the restore source; it is required when combining `--staged` -and `--worktree`. +By default, the restore source for `--worktree` is the index, and the +restore source for `--staged` is `HEAD`. When combining `--worktree` and +`--staged`, the restore source is `HEAD`. `--source` can be used to specify +a different commit as the restore source. See "Reset, restore and revert" in linkgit:git[1] for the differences between the three commands. @@ -40,10 +40,10 @@ OPTIONS tree. It is common to specify the source tree by naming a commit, branch or tag associated with it. + -If not specified, the default restore source for the working tree is -the index, and the default restore source for the index is +If not specified, the default restore source for `--worktree` is +the index, and the default restore source for `--staged` is `HEAD`. When both `--staged` and `--worktree` are specified, -`--source` must also be specified. +the default restore source is `HEAD`. -p:: --patch:: diff --git a/builtin/checkout.c b/builtin/checkout.c index 7a01d00f53..500c3e23ff 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1604,14 +1604,11 @@ static int checkout_main(int argc, const char **argv, const char *prefix, } if (opts->checkout_index < 0 || opts->checkout_worktree < 0) BUG("these flags should be non-negative by now"); - if (opts->checkout_index > 0 && opts->checkout_worktree > 0 && - !opts->from_treeish) - die(_("--source required when using --worktree and --staged")); /* - * convenient shortcut: "git restore --staged" equals - * "git restore --staged --source HEAD" + * convenient shortcut: "git restore --staged [--worktree]" equals + * "git restore --staged [--worktree] --source HEAD" */ - if (!opts->from_treeish && opts->checkout_index && !opts->checkout_worktree) + if (!opts->from_treeish && opts->checkout_index) opts->from_treeish = "HEAD"; /* diff --git a/t/t2070-restore.sh b/t/t2070-restore.sh index 19efa21fdb..89e5a142c9 100755 --- a/t/t2070-restore.sh +++ b/t/t2070-restore.sh @@ -69,9 +69,15 @@ test_expect_success 'restore --staged uses HEAD as source' ' test_cmp expected actual ' -test_expect_success 'restore --worktree --staged requires --source' ' - test_must_fail git restore --worktree --staged first.t 2>err && - test_i18ngrep "source required when using --worktree and --staged" err +test_expect_success 'restore --worktree --staged uses HEAD as source' ' + test_when_finished git reset --hard && + git show HEAD:./first.t >expected && + echo dirty >>first.t && + git add first.t && + git restore --worktree --staged first.t && + git show :./first.t >actual && + test_cmp expected actual && + test_cmp expected first.t ' test_expect_success 'restore --ignore-unmerged ignores unmerged entries' '
The default restore source for --worktree is the index, and the default source for --staged is HEAD. However, when combining --worktree and --staged in the same invocation, git-restore requires the source to be specified explicitly via --source since it would otherwise be ambiguous ("should it restore from the index or from HEAD?"). This requirement makes it cumbersome to restore a file in both the worktree and the index. However, HEAD is also a reasonably intuitive default restore source when --worktree and --staged are combined. After all, if a user is asking to throw away all local changes to a file (on disk and in the index) without specifying a restore source explicitly -- and the user expects the file to be restored from _somewhere_ -- then it is likely that the user expects them to be restored from HEAD, which is an intuitive and logical place to find a recent unadulterated copy of the file. Therefore, make HEAD the default restore source when --worktree and --staged are combined. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- Documentation/git-restore.txt | 14 +++++++------- builtin/checkout.c | 9 +++------ t/t2070-restore.sh | 12 +++++++++--- 3 files changed, 19 insertions(+), 16 deletions(-)