Re: [PATCH v2] worktree: add: introduce --checkout option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 25, 2016, 01:18 UTC
Message-ID: <CAPig+cQGTcX+1nKM-zzGrJXgKsa6aaZxAHwD=z1M8-s+ugJRFg@mail.gmail.com>
In-Reply-To: <01020153a73bbb70-11a8482f-1a90-49e4-a56c-b311e12a85a2-000000@eu-west-1.amazonses.com>

On Thu, Mar 24, 2016 at 2:07 AM, Ray Zhang <zhanglei002@gmail.com> wrote:

> By adding this option which defaults to true, we can use the
> corresponding --no-checkout to make some customizations before
> the checkout, like sparse checkout, etc.

This version of the patch looks better. Thanks. A few comments below...

> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---

Here, below the "---" line is a good place to explain to reviewers what changed since the previous version of the patch. It's also helpful to provide a link to the previous version, like this[1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289659

Show 6 quoted lines

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -87,6 +87,10 @@ OPTIONS
>         With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
>         in linkgit:git-checkout[1].
>
> +--checkout::

We can make it more clear that this is a boolean option by formatting it either like this:

    --[no-]checkout::

or this:

    --checkout::
    --no-checkout::

I don't have a strong preference, and existing documentation uses either form.

> +       Default option with `add`, populate the new working tree. Use
> +       `--no-checkout` to skip the checkout.

It's subjective, but "Default option with `add`" doesn't quite convey to me that this is the default behavior of "add". Also, readers would likely benefit from some explanation of why they might ever want to use this option. Perhaps it could be rewritten something like this:

    By default, `add` checks out HEAD, however, `--no-checkout` can
    be used to suppress checkout in order to make customizations,
    such as configuring sparse-checkout (see ...).

Show 8 quoted lines

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -213,4 +213,9 @@ test_expect_success 'local clone from linked checkout' '
>         ( cd here-clone && git fsck )
>  '
>
> +test_expect_success '"add" worktree without a checkout' '
> +       git worktree add --no-checkout -b swamp swamp &&
> +       ( cd swamp && git reset --hard && git fsck)

To match the style of the test immediately above this one, you'd want a space before the closing ')'.

However, I'm not convinced that reset+fsck is is really telling you much, as fsck is about checking the object database (which was already the subject of earlier tests in the script) and doesn't say anything about the working directory which is the point of --no-checkout. Much more interesting would be to verify that no files were checked out. There are many ways to do so; here's one:

    git worktree add --no-checkout -b swamp swamp &&
    ls swamp >actual &&
    test_line_count = 0 actual

> +'

Finally, it wouldn't hurt to also add a test to verify that --checkout works as expected (because the tests should check expected *behavior*, not *implementation*).