Re: [PATCH] checkout: teach --worktree

From: Denton Liu <liu.denton@gmail.com>
Date: Jun 14, 2020, 07:44 UTC
Message-ID: <20200614074439.GA617439@generichostname>
In-Reply-To: <CAPig+cQ_G+N=mYqO+=7UaAYft27MhZ2_3v8=QzYpDn5hQySp+A@mail.gmail.com>

Hi Eric,

On Sat, Jun 13, 2020 at 10:51:47PM -0400, Eric Sunshine wrote:

Show 17 quoted lines

> On Sat, Jun 13, 2020 at 10:25 AM Denton Liu <liu.denton@gmail.com> 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@gmail.com>
> > ---
> > 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.

Show 15 quoted lines

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