Hi Eric, On Sat, Jun 13, 2020 at 10:51:47PM -0400, Eric Sunshine wrote: > On Sat, Jun 13, 2020 at 10:25 AM Denton Liu <liu.denton@xxxxxxxxx> wrote: > > [...] > > Teach `git checkout --worktree`, allowing users to checkout files > > directly into the worktree without affecting the index. > > > > Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx> > > --- > > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > > @@ -264,6 +266,12 @@ When switching branches with `--merge`, staged changes may be lost. > > +-W:: > > +--worktree:: > > + When writing contents, only modify files in the worktree. Do not > > + modify the index. This option is essentially a no-op when used > > + without a `<tree-ish>`. > > Why a no-op rather than actually diagnosing that --worktree makes no > sense in that case and erroring out? I decided on this behaviour because I assumed that an empty `git checkout` has `git restore` behaviour but I guess I was mistaken. I'll change it to error out. > > diff --git a/t/t2028-checkout-worktree.sh b/t/t2028-checkout-worktree.sh > > @@ -0,0 +1,51 @@ > > +test_expect_success 'checkout --worktree on a commit' ' > > + test_when_finished "git reset --hard tip" && > > + git diff HEAD HEAD~ >expect && > > + git checkout --worktree HEAD~ file1 && > > + git diff >actual && > > + test_cmp expect actual && > > + git diff --cached --exit-code && > > Would the intent be clearer if you used 'test_expect_code' here? > > test_expect_code 0 git diff --cached --exit-code && > > Same question for remaining tests. I'm not really sure that this adds anything. When I read through the tests, I already expect each command to be successful, i.e. return 0. I don't see how explicitly documenting for this one command would make that more clear. Looking through the test suite, I only see 15 results of `test_expect_code 0 git diff --exit-code` and all of those are in t4035. Meanwhile, I see at least 234 instances without the `test_expect_code`. I believe that we should leave this as-is. Thanks, Denton
