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

[Eric Sunshine <sunshine@sunshineco.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

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

> 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*).