[PATCH] add option -n (--no-checkout) to git-worktree add

[PATCH] add option -n (--no-checkout) to git-worktree add

[Ray Zhang]

By adding option -n, we can make some customizations before checkout, like sparse checkout, etc.

Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
---
 builtin/worktree.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..14ca3d9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int no_checkout;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	cp.argv = NULL;
-	argv_array_clear(&cp.args);
-	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-	cp.env = child_env.argv;
-	ret = run_command(&cp);
+	if (!opts->no_checkout) {
+		cp.argv = NULL;
+		argv_array_clear(&cp.args);
+		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		cp.env = child_env.argv;
+		ret = run_command(&cp);
+	}
 	if (!ret) {
 		is_junk = 0;
 		free(junk_work_tree);
@@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char *prefix)
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
 		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
+		OPT_BOOL('n', "no-checkout", &opts.no_checkout, N_("don't create a checkout")),
 		OPT_END()
 	};
 

--
https://github.com/git/git/pull/217

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

[Eric Sunshine]

On Wed, Mar 23, 2016 at 11:08 AM, Ray Zhang <zhanglei002@gmail.com> wrote:
> add option -n (--no-checkout) to git-worktree add

Alternate:

    worktree: add: introduce --no-checkout option

> By adding option -n, we can make some customizations before checkout, like sparse checkout, etc.

This parallels git-clone's --no-checkout. Okay.

Typically, one would not squat on a short option (-n) when first
introducing a feature and would only add the short equivalent after
the option proved popular, however, in this case, as git-clone
supports -n, I suppose finger muscle-memory is a consideration.

By the way, please wrap the commit message at 70-72 characters or so.

> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---
>  builtin/worktree.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

This change needs corresponding documentation
(Documentation/git-worktree.txt) and test (t/t2025-worktree-add.sh)
updates.

Thanks.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 38b5609..14ca3d9 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
>  struct add_opts {
>         int force;
>         int detach;
> +       int no_checkout;
>         const char *new_branch;
>         int force_new_branch;
>  };
> @@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
>         if (ret)
>                 goto done;
>
> -       cp.argv = NULL;
> -       argv_array_clear(&cp.args);
> -       argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> -       cp.env = child_env.argv;
> -       ret = run_command(&cp);
> +       if (!opts->no_checkout) {
> +               cp.argv = NULL;
> +               argv_array_clear(&cp.args);
> +               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +               cp.env = child_env.argv;
> +               ret = run_command(&cp);
> +       }
>         if (!ret) {
>                 is_junk = 0;
>                 free(junk_work_tree);
> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char *prefix)
>                 OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
>                            N_("create or reset a branch")),
>                 OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
> +               OPT_BOOL('n', "no-checkout", &opts.no_checkout, N_("don't create a checkout")),
>                 OPT_END()
>         };

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

[Junio C Hamano]

Ray Zhang <zhanglei002@gmail.com> writes:

> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char *prefix)
>  		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
>  			   N_("create or reset a branch")),
>  		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
> +		OPT_BOOL('n', "no-checkout", &opts.no_checkout, N_("don't create a checkout")),

This would allow --no-no-checkout, which is idiotic, wouldn't it?

How about

    OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree"))

and set opts.checkout to true when initializing?

Re: [PATCH] add option -n (--no-checkout) to git-worktree add

[Eric Sunshine]

On Wed, Mar 23, 2016 at 11:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ray Zhang <zhanglei002@gmail.com> writes:
>
>> @@ -320,6 +323,7 @@ static int add(int ac, const char **av, const char *prefix)
>>               OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
>>                          N_("create or reset a branch")),
>>               OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
>> +             OPT_BOOL('n', "no-checkout", &opts.no_checkout, N_("don't create a checkout")),
>
> This would allow --no-no-checkout, which is idiotic, wouldn't it?
>
> How about
>
>     OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree"))
>
> and set opts.checkout to true when initializing?

I think this code was copied verbatim from builtin/clone.c, and, as a
newcomer to the project, it's understandable that Ray Zhang imitated
existing code, but I agree that it would be better to avoid repeating
the misbehavior.

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

[Ray Zhang]

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.

Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
---
 Documentation/git-worktree.txt |  6 +++++-
 builtin/worktree.c             | 15 ++++++++++-----
 t/t2025-worktree-add.sh        |  5 +++++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..e96fe0f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 --------
 [verse]
-'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
+'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
 
@@ -87,6 +87,10 @@ OPTIONS
 	With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
 	in linkgit:git-checkout[1].
 
+--checkout::
+	Default option with `add`, populate the new working tree. Use
+	`--no-checkout` to skip the checkout.
+
 -n::
 --dry-run::
 	With `prune`, do not remove anything; just report what it would
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..e677cd7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int checkout;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	cp.argv = NULL;
-	argv_array_clear(&cp.args);
-	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-	cp.env = child_env.argv;
-	ret = run_command(&cp);
+	if (opts->checkout) {
+		cp.argv = NULL;
+		argv_array_clear(&cp.args);
+		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		cp.env = child_env.argv;
+		ret = run_command(&cp);
+	}
 	if (!ret) {
 		is_junk = 0;
 		free(junk_work_tree);
@@ -320,10 +323,12 @@ static int add(int ac, const char **av, const char *prefix)
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
 		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
+		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
 		OPT_END()
 	};
 
 	memset(&opts, 0, sizeof(opts));
+	opts.checkout = 1;
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
 		die(_("-b, -B, and --detach are mutually exclusive"));
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cbfa41e..601f963 100755
--- 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)
+'
+
 test_done

--
https://github.com/git/git/pull/217

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

[Duy Nguyen]

On Thu, Mar 24, 2016 at 1:07 PM, 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.

I think we can follow git-clone and use '-n' for this. But if it's
sparse checkout that's you're after, be warned that it's not fully
supported (you either enable sparse chekcuot for all worktrees, or
none).
-- 
Duy

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

[Zhang Lei]

hi Duy,
My PATCH v1 did follow git-clone -n, however, Junio C Hamano and Eric Sunshine
suggested that we should avoid doing so , as --no-no-checkout could be
confusing.

Yes, core.sparsecheckout is the global switch for all worktrees, but
every worktree
can have its own info/sparse-checkout.

2016-03-24 17:16 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>
> On Thu, Mar 24, 2016 at 1:07 PM, 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.
>
> I think we can follow git-clone and use '-n' for this. But if it's
> sparse checkout that's you're after, be warned that it's not fully
> supported (you either enable sparse chekcuot for all worktrees, or
> none).
> --
> Duy

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

[Eric Sunshine]

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

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

[Eric Sunshine]

[please respond inline rather than top-posting]

On Thu, Mar 24, 2016 at 5:52 AM, Zhang Lei <zhanglei002@gmail.com> wrote:
> 2016-03-24 17:16 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>> I think we can follow git-clone and use '-n' for this. [...]
>
> My PATCH v1 did follow git-clone -n, however, Junio C Hamano and Eric Sunshine
> suggested that we should avoid doing so , as --no-no-checkout could be
> confusing.

My impression was that Duy was suggesting only that -n be recognized
as shorthand for --no-checkout, however, git-worktree already
recognizes -n as shorthand for --dry-run (as a consequence of using
OPT__DRY_RUN), so -n as shorthand for --no-checkout is a no-go.

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

[Eric Sunshine]

On Thu, Mar 24, 2016 at 9:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 24, 2016 at 5:52 AM, Zhang Lei <zhanglei002@gmail.com> wrote:
>> 2016-03-24 17:16 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>>> I think we can follow git-clone and use '-n' for this. [...]
>>
>> My PATCH v1 did follow git-clone -n, however, Junio C Hamano and Eric Sunshine
>> suggested that we should avoid doing so , as --no-no-checkout could be
>> confusing.
>
> My impression was that Duy was suggesting only that -n be recognized
> as shorthand for --no-checkout, however, git-worktree already
> recognizes -n as shorthand for --dry-run (as a consequence of using
> OPT__DRY_RUN), so -n as shorthand for --no-checkout is a no-go.

Ignore this. It's only 'prune' which recognizes -n, so it's possible
that 'add' could recognize it for an alternate meaning (though the
documentation would want to make this very clear).

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

[Duy Nguyen]

On Fri, Mar 25, 2016 at 8:29 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 24, 2016 at 9:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Mar 24, 2016 at 5:52 AM, Zhang Lei <zhanglei002@gmail.com> wrote:
>>> 2016-03-24 17:16 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>>>> I think we can follow git-clone and use '-n' for this. [...]
>>>
>>> My PATCH v1 did follow git-clone -n, however, Junio C Hamano and Eric Sunshine
>>> suggested that we should avoid doing so , as --no-no-checkout could be
>>> confusing.
>>
>> My impression was that Duy was suggesting only that -n be recognized
>> as shorthand for --no-checkout, however, git-worktree already
>> recognizes -n as shorthand for --dry-run (as a consequence of using
>> OPT__DRY_RUN), so -n as shorthand for --no-checkout is a no-go.
>
> Ignore this. It's only 'prune' which recognizes -n, so it's possible
> that 'add' could recognize it for an alternate meaning (though the
> documentation would want to make this very clear).

To make it clear, I don't feel strongly about '-n'. Yes muscle memory
may count. But if '-n' may become a new confusion source in
git-worktree then perhaps we should avoid it and go with
--[no-]checkout
-- 
Duy

[PATCH v3] worktree: add: introduce --checkout option

[Ray Zhang]

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.

Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
---
1. reword on `--no-checkout` in Documentation/git-worktree.txt
2. update the test for `--no-checkout`
3. add a test for `--checkout`
Previous version of this patch:[v2]

[v2]: http://article.gmane.org/gmane.comp.version-control.git/289713
---
 Documentation/git-worktree.txt |  8 +++++++-
 builtin/worktree.c             | 15 ++++++++++-----
 t/t2025-worktree-add.sh        | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..c2796bb 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 --------
 [verse]
-'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
+'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
 
@@ -87,6 +87,12 @@ OPTIONS
 	With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
 	in linkgit:git-checkout[1].
 
+--[no-]checkout::
+	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 "Sparse checkout"
+	in linkgit:git-read-tree[1].
+
 -n::
 --dry-run::
 	With `prune`, do not remove anything; just report what it would
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..e677cd7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int checkout;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	cp.argv = NULL;
-	argv_array_clear(&cp.args);
-	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-	cp.env = child_env.argv;
-	ret = run_command(&cp);
+	if (opts->checkout) {
+		cp.argv = NULL;
+		argv_array_clear(&cp.args);
+		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		cp.env = child_env.argv;
+		ret = run_command(&cp);
+	}
 	if (!ret) {
 		is_junk = 0;
 		free(junk_work_tree);
@@ -320,10 +323,12 @@ static int add(int ac, const char **av, const char *prefix)
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
 		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
+		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
 		OPT_END()
 	};
 
 	memset(&opts, 0, sizeof(opts));
+	opts.checkout = 1;
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
 		die(_("-b, -B, and --detach are mutually exclusive"));
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cbfa41e..1ff96af 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -213,4 +213,18 @@ test_expect_success 'local clone from linked checkout' '
 	( cd here-clone && git fsck )
 '
 
+test_expect_success '"add" worktree with --no-checkout' '
+	git worktree add --no-checkout -b swamp swamp &&
+	ls swamp >actual && test_line_count = 0 actual &&
+	(
+		cd swamp && git reset --hard &&
+		test_cmp ../init.t init.t
+	)
+'
+
+test_expect_success '"add" worktree with --checkout' '
+	git worktree add --checkout -b swmap2 swamp2 &&
+	( cd swamp2 && test_cmp ../init.t init.t )
+'
+
 test_done

--
https://github.com/git/git/pull/217

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

[Zhang Lei]

Thanks for the clarification.
By the way, Duy, another unrelated question: why worktree name under
.git/worktrees is being named
after the working tree path basename? I think branch name is more
reasonable since we don't allow checking out
the same branch twice.

2016-03-25 9:49 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Fri, Mar 25, 2016 at 8:29 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Mar 24, 2016 at 9:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Thu, Mar 24, 2016 at 5:52 AM, Zhang Lei <zhanglei002@gmail.com> wrote:
>>>> 2016-03-24 17:16 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>>>>> I think we can follow git-clone and use '-n' for this. [...]
>>>>
>>>> My PATCH v1 did follow git-clone -n, however, Junio C Hamano and Eric Sunshine
>>>> suggested that we should avoid doing so , as --no-no-checkout could be
>>>> confusing.
>>>
>>> My impression was that Duy was suggesting only that -n be recognized
>>> as shorthand for --no-checkout, however, git-worktree already
>>> recognizes -n as shorthand for --dry-run (as a consequence of using
>>> OPT__DRY_RUN), so -n as shorthand for --no-checkout is a no-go.
>>
>> Ignore this. It's only 'prune' which recognizes -n, so it's possible
>> that 'add' could recognize it for an alternate meaning (though the
>> documentation would want to make this very clear).
>
> To make it clear, I don't feel strongly about '-n'. Yes muscle memory
> may count. But if '-n' may become a new confusion source in
> git-worktree then perhaps we should avoid it and go with
> --[no-]checkout
> --
> Duy

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

[Duy Nguyen]

On Fri, Mar 25, 2016 at 6:31 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
> By the way, Duy, another unrelated question: why worktree name under
> .git/worktrees is being named
> after the working tree path basename? I think branch name is more
> reasonable since we don't allow checking out
> the same branch twice.

Because branch name is not always available (e.g. detached HEAD) and
checkout branch can be switched later on. And normally you'll get
branch name there anyway with "git worktree add something" because the
branch "something" is automatically created. I've been wondering if
it's worth supporting "git worktree -b abc ./" where we create
worktree "./abc" based on branch name too.
-- 
Duy

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

[Zhang Lei]

Yes, path basename makes sense.
I am asking this question because we have some legacy code requires
that working tree
called something like 'src', as a result, multiple branch would have
src1 src2 src3 under .git/worktrees
which could not be easy to maintain.
I agreed with you, we should give users such option.


2016-03-25 19:41 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Fri, Mar 25, 2016 at 6:31 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
>> By the way, Duy, another unrelated question: why worktree name under
>> .git/worktrees is being named
>> after the working tree path basename? I think branch name is more
>> reasonable since we don't allow checking out
>> the same branch twice.
>
> Because branch name is not always available (e.g. detached HEAD) and
> checkout branch can be switched later on. And normally you'll get
> branch name there anyway with "git worktree add something" because the
> branch "something" is automatically created. I've been wondering if
> it's worth supporting "git worktree -b abc ./" where we create
> worktree "./abc" based on branch name too.
> --
> Duy

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

[Duy Nguyen]

Please don't top-post.

On Fri, Mar 25, 2016 at 7:06 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
> Yes, path basename makes sense.
> I am asking this question because we have some legacy code requires
> that working tree
> called something like 'src', as a result, multiple branch would have
> src1 src2 src3 under .git/worktrees
> which could not be easy to maintain.

If you really need to care about these names, I think a new option to
let you control the naming explicitly would be better.

> I agreed with you, we should give users such option.
>
>
> 2016-03-25 19:41 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
>> On Fri, Mar 25, 2016 at 6:31 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
>>> By the way, Duy, another unrelated question: why worktree name under
>>> .git/worktrees is being named
>>> after the working tree path basename? I think branch name is more
>>> reasonable since we don't allow checking out
>>> the same branch twice.
>>
>> Because branch name is not always available (e.g. detached HEAD) and
>> checkout branch can be switched later on. And normally you'll get
>> branch name there anyway with "git worktree add something" because the
>> branch "something" is automatically created. I've been wondering if
>> it's worth supporting "git worktree -b abc ./" where we create
>> worktree "./abc" based on branch name too.
>> --
>> Duy



-- 
Duy

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

[Mike Rappazzo]

On Fri, Mar 25, 2016 at 7:41 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 6:31 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
>> By the way, Duy, another unrelated question: why worktree name under
>> .git/worktrees is being named
>> after the working tree path basename? I think branch name is more
>> reasonable since we don't allow checking out
>> the same branch twice.
>
> Because branch name is not always available (e.g. detached HEAD) and
> checkout branch can be switched later on. And normally you'll get
> branch name there anyway with "git worktree add something" because the
> branch "something" is automatically created. I've been wondering if
> it's worth supporting "git worktree -b abc ./" where we create
> worktree "./abc" based on branch name too.

You can switch to any other branch in a worktree.  Consider that you
could switch branches in
worktrees such that you could eventually end up having the branches
swapped from original
worktree setup.

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

[Eric Sunshine]

On Fri, Mar 25, 2016 at 11:25:37AM +0000, Ray Zhang 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.
> 
> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---
> 1. reword on `--no-checkout` in Documentation/git-worktree.txt
> 2. update the test for `--no-checkout`
> 3. add a test for `--checkout`

Thanks, this version of the patch looks good and is:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

with or without the minor suggestions below. (If you do re-roll, feel
free to add my Reviewed-by: if you include these suggestions but not
if you make other major changes.)

> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -87,6 +87,12 @@ OPTIONS
> +--[no-]checkout::
> +	By default, `add` checks out HEAD, however, `--no-checkout` can

I realize that this description is just a verbatim copy of what I
suggested during review[1], but in retrospect, I think
s/HEAD/`<branch>`/ would be clearer and more consistent:

    By default, `add` checks out `<branch>`, however, ...

[1]: http://article.gmane.org/gmane.comp.version-control.git/289840

> +	be used to suppress checkout in order to make customizations,
> +	such as configuring sparse-checkout. See "Sparse checkout"
> +	in linkgit:git-read-tree[1].
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -213,4 +213,18 @@ test_expect_success 'local clone from linked checkout' '
> +test_expect_success '"add" worktree with --no-checkout' '
> +	git worktree add --no-checkout -b swamp swamp &&
> +	ls swamp >actual && test_line_count = 0 actual &&

Style: One statement per line, so this should be split over two
lines.

> +	(
> +		cd swamp && git reset --hard &&
> +		test_cmp ../init.t init.t
> +	)

No need for the subshell. This can be written more simply as:

    git -C swamp reset --hard &&
    test_cmp init.t swamp/init.t

> +'
> +
> +test_expect_success '"add" worktree with --checkout' '
> +	git worktree add --checkout -b swmap2 swamp2 &&
> +	( cd swamp2 && test_cmp ../init.t init.t )

Likewise, you can drop the subshell:

    test_cmp init.t swamp2/init.t

> +'
> +
>  test_done

For convenience, the above suggestions would look like this (applied
atop your patch). Feel free to fold them in if you re-roll.

--- 8< ---
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c2796bb..c622345 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -88,7 +88,7 @@ OPTIONS
 	in linkgit:git-checkout[1].
 
 --[no-]checkout::
-	By default, `add` checks out HEAD, however, `--no-checkout` can
+	By default, `add` checks out `<branch>`, however, `--no-checkout` can
 	be used to suppress checkout in order to make customizations,
 	such as configuring sparse-checkout. See "Sparse checkout"
 	in linkgit:git-read-tree[1].
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 1ff96af..472b811 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -215,16 +215,15 @@ test_expect_success 'local clone from linked checkout' '
 
 test_expect_success '"add" worktree with --no-checkout' '
 	git worktree add --no-checkout -b swamp swamp &&
-	ls swamp >actual && test_line_count = 0 actual &&
-	(
-		cd swamp && git reset --hard &&
-		test_cmp ../init.t init.t
-	)
+	ls swamp >actual &&
+	test_line_count = 0 actual &&
+	git -C swamp reset --hard &&
+	test_cmp init.t swamp/init.t
 '
 
 test_expect_success '"add" worktree with --checkout' '
 	git worktree add --checkout -b swmap2 swamp2 &&
-	( cd swamp2 && test_cmp ../init.t init.t )
+	test_cmp init.t swamp2/init.t
 '
 
 test_done
--- 8< ---

[PATCH v4] worktree: add: introduce --checkout option

[Ray Zhang]

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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
---
Changes since last version of this patch[v3]:
	Documentation/git-worktree.txt: HEAD --> `<branch>`
	t/t2025-worktree-add.sh: fix style

[v3]: http://article.gmane.org/gmane.comp.version-control.git/289877
[v2]: http://article.gmane.org/gmane.comp.version-control.git/289713
[v1]: http://article.gmane.org/gmane.comp.version-control.git/289659
---
 Documentation/git-worktree.txt |  8 +++++++-
 builtin/worktree.c             | 15 ++++++++++-----
 t/t2025-worktree-add.sh        | 13 +++++++++++++
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..c622345 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 --------
 [verse]
-'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
+'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
 
@@ -87,6 +87,12 @@ OPTIONS
 	With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
 	in linkgit:git-checkout[1].
 
+--[no-]checkout::
+	By default, `add` checks out `<branch>`, however, `--no-checkout` can
+	be used to suppress checkout in order to make customizations,
+	such as configuring sparse-checkout. See "Sparse checkout"
+	in linkgit:git-read-tree[1].
+
 -n::
 --dry-run::
 	With `prune`, do not remove anything; just report what it would
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..e677cd7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int checkout;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	cp.argv = NULL;
-	argv_array_clear(&cp.args);
-	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-	cp.env = child_env.argv;
-	ret = run_command(&cp);
+	if (opts->checkout) {
+		cp.argv = NULL;
+		argv_array_clear(&cp.args);
+		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		cp.env = child_env.argv;
+		ret = run_command(&cp);
+	}
 	if (!ret) {
 		is_junk = 0;
 		free(junk_work_tree);
@@ -320,10 +323,12 @@ static int add(int ac, const char **av, const char *prefix)
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
 		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
+		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
 		OPT_END()
 	};
 
 	memset(&opts, 0, sizeof(opts));
+	opts.checkout = 1;
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
 		die(_("-b, -B, and --detach are mutually exclusive"));
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cbfa41e..472b811 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -213,4 +213,17 @@ test_expect_success 'local clone from linked checkout' '
 	( cd here-clone && git fsck )
 '
 
+test_expect_success '"add" worktree with --no-checkout' '
+	git worktree add --no-checkout -b swamp swamp &&
+	ls swamp >actual &&
+	test_line_count = 0 actual &&
+	git -C swamp reset --hard &&
+	test_cmp init.t swamp/init.t
+'
+
+test_expect_success '"add" worktree with --checkout' '
+	git worktree add --checkout -b swmap2 swamp2 &&
+	test_cmp init.t swamp2/init.t
+'
+
 test_done

--
https://github.com/git/git/pull/217

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

[Junio C Hamano]

Ray Zhang <zhanglei002@gmail.com> writes:

> 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.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---

Thanks.  Will queue.

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index cbfa41e..472b811 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -213,4 +213,17 @@ test_expect_success 'local clone from linked checkout' '
>  	( cd here-clone && git fsck )
>  '
>  
> +test_expect_success '"add" worktree with --no-checkout' '
> +	git worktree add --no-checkout -b swamp swamp &&
> +	ls swamp >actual &&
> +	test_line_count = 0 actual &&

The remainder of the test (both existing and added parts)
seems to only care about init.t; running "ls swamp" feels
somewhat inconsistent.  We could replace the two lines with

	! test -e swamp/init.t

to address this, but I do not think it is worth a patch churn.

> +	git -C swamp reset --hard &&
> +	test_cmp init.t swamp/init.t
> +'
> +test_expect_success '"add" worktree with --checkout' '
> +	git worktree add --checkout -b swmap2 swamp2 &&
> +	test_cmp init.t swamp2/init.t
> +'
> +
>  test_done

[PATCH v5] worktree: add: introduce --checkout option

[Ray Zhang]

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.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
---
Changes since last version of this patch[v4]:
	t/t2025-worktree-add.sh: use test -e to test file existence.
	builtin/worktree.c: refactor the code a little bit.

[v4]: http://article.gmane.org/gmane.comp.version-control.git/290030
[v3]: http://article.gmane.org/gmane.comp.version-control.git/289877
[v2]: http://article.gmane.org/gmane.comp.version-control.git/289713
[v1]: http://article.gmane.org/gmane.comp.version-control.git/289659
---
 Documentation/git-worktree.txt |  8 +++++++-
 builtin/worktree.c             | 29 ++++++++++++++++++-----------
 t/t2025-worktree-add.sh        | 12 ++++++++++++
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..c622345 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
 SYNOPSIS
 --------
 [verse]
-'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
+'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree list' [--porcelain]
 
@@ -87,6 +87,12 @@ OPTIONS
 	With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
 	in linkgit:git-checkout[1].
 
+--[no-]checkout::
+	By default, `add` checks out `<branch>`, however, `--no-checkout` can
+	be used to suppress checkout in order to make customizations,
+	such as configuring sparse-checkout. See "Sparse checkout"
+	in linkgit:git-read-tree[1].
+
 -n::
 --dry-run::
 	With `prune`, do not remove anything; just report what it would
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..d8e3795 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int checkout;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	cp.argv = NULL;
-	argv_array_clear(&cp.args);
-	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-	cp.env = child_env.argv;
-	ret = run_command(&cp);
-	if (!ret) {
-		is_junk = 0;
-		free(junk_work_tree);
-		free(junk_git_dir);
-		junk_work_tree = NULL;
-		junk_git_dir = NULL;
+	if (opts->checkout) {
+		cp.argv = NULL;
+		argv_array_clear(&cp.args);
+		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		cp.env = child_env.argv;
+		ret = run_command(&cp);
+		if (ret)
+			goto done;
 	}
+
+	is_junk = 0;
+	free(junk_work_tree);
+	free(junk_git_dir);
+	junk_work_tree = NULL;
+	junk_git_dir = NULL;
+
 done:
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
@@ -320,10 +325,12 @@ static int add(int ac, const char **av, const char *prefix)
 		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
 			   N_("create or reset a branch")),
 		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
+		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
 		OPT_END()
 	};
 
 	memset(&opts, 0, sizeof(opts));
+	opts.checkout = 1;
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
 		die(_("-b, -B, and --detach are mutually exclusive"));
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cbfa41e..3acb992 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -213,4 +213,16 @@ test_expect_success 'local clone from linked checkout' '
 	( cd here-clone && git fsck )
 '
 
+test_expect_success '"add" worktree with --no-checkout' '
+	git worktree add --no-checkout -b swamp swamp &&
+	! test -e swamp/init.t &&
+	git -C swamp reset --hard &&
+	test_cmp init.t swamp/init.t
+'
+
+test_expect_success '"add" worktree with --checkout' '
+	git worktree add --checkout -b swmap2 swamp2 &&
+	test_cmp init.t swamp2/init.t
+'
+
 test_done

--
https://github.com/git/git/pull/217

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

[John Keeping]

On Tue, Mar 29, 2016 at 10:11:01AM +0000, Ray Zhang 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.
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---
> Changes since last version of this patch[v4]:
> 	t/t2025-worktree-add.sh: use test -e to test file existence.
> 	builtin/worktree.c: refactor the code a little bit.
> 
> [v4]: http://article.gmane.org/gmane.comp.version-control.git/290030
> [v3]: http://article.gmane.org/gmane.comp.version-control.git/289877
> [v2]: http://article.gmane.org/gmane.comp.version-control.git/289713
> [v1]: http://article.gmane.org/gmane.comp.version-control.git/289659
> ---
>  Documentation/git-worktree.txt |  8 +++++++-
>  builtin/worktree.c             | 29 ++++++++++++++++++-----------
>  t/t2025-worktree-add.sh        | 12 ++++++++++++
>  3 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 62c76c1..c622345 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
>  SYNOPSIS
>  --------
>  [verse]
> -'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
> +'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
>  'git worktree prune' [-n] [-v] [--expire <expire>]
>  'git worktree list' [--porcelain]
>  
> @@ -87,6 +87,12 @@ OPTIONS
>  	With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
>  	in linkgit:git-checkout[1].
>  
> +--[no-]checkout::

This should be:

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

(see for example --progress in Documentation/merge-options.txt).

> +	By default, `add` checks out `<branch>`, however, `--no-checkout` can
> +	be used to suppress checkout in order to make customizations,
> +	such as configuring sparse-checkout. See "Sparse checkout"
> +	in linkgit:git-read-tree[1].
> +
>  -n::
>  --dry-run::
>  	With `prune`, do not remove anything; just report what it would
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 38b5609..d8e3795 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
>  struct add_opts {
>  	int force;
>  	int detach;
> +	int checkout;
>  	const char *new_branch;
>  	int force_new_branch;
>  };
> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char *refname,
>  	if (ret)
>  		goto done;
>  
> -	cp.argv = NULL;
> -	argv_array_clear(&cp.args);
> -	argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> -	cp.env = child_env.argv;
> -	ret = run_command(&cp);
> -	if (!ret) {
> -		is_junk = 0;
> -		free(junk_work_tree);
> -		free(junk_git_dir);
> -		junk_work_tree = NULL;
> -		junk_git_dir = NULL;
> +	if (opts->checkout) {
> +		cp.argv = NULL;
> +		argv_array_clear(&cp.args);
> +		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +		cp.env = child_env.argv;
> +		ret = run_command(&cp);
> +		if (ret)
> +			goto done;
>  	}
> +
> +	is_junk = 0;
> +	free(junk_work_tree);
> +	free(junk_git_dir);
> +	junk_work_tree = NULL;
> +	junk_git_dir = NULL;
> +
>  done:
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> @@ -320,10 +325,12 @@ static int add(int ac, const char **av, const char *prefix)
>  		OPT_STRING('B', NULL, &new_branch_force, N_("branch"),
>  			   N_("create or reset a branch")),
>  		OPT_BOOL(0, "detach", &opts.detach, N_("detach HEAD at named commit")),
> +		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
>  		OPT_END()
>  	};
>  
>  	memset(&opts, 0, sizeof(opts));
> +	opts.checkout = 1;
>  	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
>  	if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
>  		die(_("-b, -B, and --detach are mutually exclusive"));
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index cbfa41e..3acb992 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -213,4 +213,16 @@ test_expect_success 'local clone from linked checkout' '
>  	( cd here-clone && git fsck )
>  '
>  
> +test_expect_success '"add" worktree with --no-checkout' '
> +	git worktree add --no-checkout -b swamp swamp &&
> +	! test -e swamp/init.t &&
> +	git -C swamp reset --hard &&
> +	test_cmp init.t swamp/init.t
> +'
> +
> +test_expect_success '"add" worktree with --checkout' '
> +	git worktree add --checkout -b swmap2 swamp2 &&
> +	test_cmp init.t swamp2/init.t
> +'
> +
>  test_done
> 
> --
> https://github.com/git/git/pull/217
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, Mar 29, 2016 at 6:54 AM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 29, 2016 at 10:11:01AM +0000, Ray Zhang wrote:
>>       With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
>>       in linkgit:git-checkout[1].
>>
>> +--[no-]checkout::
>
> This should be:
>
> --checkout::
> --no-checkout::
>
> (see for example --progress in Documentation/merge-options.txt).

[1] suggested either form without stating a preference since existing
Git documentation uses a mixture of the two. See, for instance,
git-format-patch.txt. However, I see now that --[no-]-option is the
minority.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289840

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

[Eric Sunshine]

[re-sending this to the list since it's relevant, and I somehow
accidentally dropped the list]

On Mon, Mar 28, 2016 at 1:13 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Mar 28, 2016 at 6:52 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.
>>
>> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
>> ---
>> Changes since last version of this patch[v3]:
>>         Documentation/git-worktree.txt: HEAD --> `<branch>`
>>         t/t2025-worktree-add.sh: fix style
>
> Thanks, this version addresses the minor issues raised with the previous one.
>
> One observation below...
>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -284,11 +285,13 @@ static int add_worktree(const char *path, const char *refname,
>>         if (ret)
>>                 goto done;
>>
>> -       cp.argv = NULL;
>> -       argv_array_clear(&cp.args);
>> -       argv_array_pushl(&cp.args, "reset", "--hard", NULL);
>> -       cp.env = child_env.argv;
>> -       ret = run_command(&cp);
>> +       if (opts->checkout) {
>> +               cp.argv = NULL;
>> +               argv_array_clear(&cp.args);
>> +               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
>> +               cp.env = child_env.argv;
>> +               ret = run_command(&cp);
>> +       }
>>         if (!ret) {
>>                 is_junk = 0;
>>                 free(junk_work_tree);
>
> In the no-checkout case, this code effectively becomes:
>
>     ret = run_command("update-ref"/"symbolic-ref");
>     if (ret)
>         goto done;
>     ...
>     if (!ret)
>         ...free stuff...
>     done:
>         ...
>
> 'ret' does not change value, so it's a bit odd to have an 'if (ret)'
> conditional immediately followed by an 'if (!ret)'.
>
> It might be cleaner to re-organize the code slightly so that 'if
> (ret)' is used after all run_command()s, and then outdent the "...free
> stuff..." code, like this:
>
>     ret = run_command(...);
>     if (ret)
>         goto done;
>     if (checkout) {
>         ret = run_command(...)
>         if (ret)
>             goto done;
>     }
>     ...free stuff...
>     done:
>         ...
>
> This is a very minor issue, and I'm not even convinced that the code
> is any clearer this way, so it's not something which should hold up
> this patch. If you try it and find it nicer, then it could be done as
> a follow-on cleanup patch (or a preparatory cleanup patch if you want
> to re-roll, but that's probably not necessary).
>
> I should have mentioned this earlier since I specifically looked for
> it with a previous version of the patch, but unfortunately didn't
> quite spot it. This time I did spot it.

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

[Eric Sunshine]

On Tue, Mar 29, 2016 at 6:11 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.
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---
> Changes since last version of this patch[v4]:
>         t/t2025-worktree-add.sh: use test -e to test file existence.
>         builtin/worktree.c: refactor the code a little bit.

Thanks, this version is still:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

A couple comments below...

> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char *refname,
>         if (ret)
>                 goto done;
>
> -       cp.argv = NULL;
> -       argv_array_clear(&cp.args);
> -       argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> -       cp.env = child_env.argv;
> -       ret = run_command(&cp);
> -       if (!ret) {
> -               is_junk = 0;
> -               free(junk_work_tree);
> -               free(junk_git_dir);
> -               junk_work_tree = NULL;
> -               junk_git_dir = NULL;
> +       if (opts->checkout) {
> +               cp.argv = NULL;
> +               argv_array_clear(&cp.args);
> +               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +               cp.env = child_env.argv;
> +               ret = run_command(&cp);
> +               if (ret)
> +                       goto done;
>         }
> +
> +       is_junk = 0;
> +       free(junk_work_tree);
> +       free(junk_git_dir);
> +       junk_work_tree = NULL;
> +       junk_git_dir = NULL;

Doing the goto-dance and outdenting the "freeing" code as suggested as
a possible improvement by [1] probably should have been done as a
separate preparatory patch since the result in this patch is fairly
noisy and more difficult to review. However, it's probably not worth
the patch churn to do so now.

>  done:
>         strbuf_reset(&sb);
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> +test_expect_success '"add" worktree with --no-checkout' '
> +       git worktree add --no-checkout -b swamp swamp &&
> +       ! test -e swamp/init.t &&

I realize that this was suggested by [2], however, a more modern way
to state this would be:

    test_path_is_missing swamp/init.t &&

but, as also mentioned in [2], it's probably not worth the patch churn
to change it now.

> +       git -C swamp reset --hard &&
> +       test_cmp init.t swamp/init.t
> +'
> +
> +test_expect_success '"add" worktree with --checkout' '
> +       git worktree add --checkout -b swmap2 swamp2 &&
> +       test_cmp init.t swamp2/init.t
> +'
> +
>  test_done

[1]: http://git.661346.n2.nabble.com/PATCH-add-option-n-no-checkout-to-git-worktree-add-tp7651385p7651884.html

[2]: http://article.gmane.org/gmane.comp.version-control.git/290050

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

[John Keeping]

On Tue, Mar 29, 2016 at 02:04:38PM -0400, Eric Sunshine wrote:
> On Tue, Mar 29, 2016 at 6:54 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Tue, Mar 29, 2016 at 10:11:01AM +0000, Ray Zhang wrote:
> >>       With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
> >>       in linkgit:git-checkout[1].
> >>
> >> +--[no-]checkout::
> >
> > This should be:
> >
> > --checkout::
> > --no-checkout::
> >
> > (see for example --progress in Documentation/merge-options.txt).
> 
> [1] suggested either form without stating a preference since existing
> Git documentation uses a mixture of the two. See, for instance,
> git-format-patch.txt. However, I see now that --[no-]-option is the
> minority.
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289840

I tend to skim the mailing list so I didn't register that at the time.

Having gone looking, I can't find a reference but I for some reason I
was convinced the separate version was preferred in the option
descriptions.  Note that AsciiDoc does handle this specially, at least
when outputting troff (HTML seems to show both on separate lines).

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Having gone looking, I can't find a reference but I for some reason I
> was convinced the separate version was preferred in the option
> descriptions. 

The one that is similar that came back to my mind while reading your
response was that we used to have these:

	--foo, --bar::
		Description...

and corrected them to:

	--foo::
        --bar::
		Description...

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

[Zhang Lei]

Thanks for the review.
Sorry for the patch churn, I wasn't quite familiar with working with
mailing list.

2016-03-30 3:20 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:
> On Tue, Mar 29, 2016 at 6:11 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.
>>
>> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
>> ---
>> Changes since last version of this patch[v4]:
>>         t/t2025-worktree-add.sh: use test -e to test file existence.
>>         builtin/worktree.c: refactor the code a little bit.
>
> Thanks, this version is still:
>
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>
> A couple comments below...
>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char *refname,
>>         if (ret)
>>                 goto done;
>>
>> -       cp.argv = NULL;
>> -       argv_array_clear(&cp.args);
>> -       argv_array_pushl(&cp.args, "reset", "--hard", NULL);
>> -       cp.env = child_env.argv;
>> -       ret = run_command(&cp);
>> -       if (!ret) {
>> -               is_junk = 0;
>> -               free(junk_work_tree);
>> -               free(junk_git_dir);
>> -               junk_work_tree = NULL;
>> -               junk_git_dir = NULL;
>> +       if (opts->checkout) {
>> +               cp.argv = NULL;
>> +               argv_array_clear(&cp.args);
>> +               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
>> +               cp.env = child_env.argv;
>> +               ret = run_command(&cp);
>> +               if (ret)
>> +                       goto done;
>>         }
>> +
>> +       is_junk = 0;
>> +       free(junk_work_tree);
>> +       free(junk_git_dir);
>> +       junk_work_tree = NULL;
>> +       junk_git_dir = NULL;
>
> Doing the goto-dance and outdenting the "freeing" code as suggested as
> a possible improvement by [1] probably should have been done as a
> separate preparatory patch since the result in this patch is fairly
> noisy and more difficult to review. However, it's probably not worth
> the patch churn to do so now.
>
>>  done:
>>         strbuf_reset(&sb);
>>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
>> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
>> +test_expect_success '"add" worktree with --no-checkout' '
>> +       git worktree add --no-checkout -b swamp swamp &&
>> +       ! test -e swamp/init.t &&
>
> I realize that this was suggested by [2], however, a more modern way
> to state this would be:
>
>     test_path_is_missing swamp/init.t &&
>
> but, as also mentioned in [2], it's probably not worth the patch churn
> to change it now.
>
>> +       git -C swamp reset --hard &&
>> +       test_cmp init.t swamp/init.t
>> +'
>> +
>> +test_expect_success '"add" worktree with --checkout' '
>> +       git worktree add --checkout -b swmap2 swamp2 &&
>> +       test_cmp init.t swamp2/init.t
>> +'
>> +
>>  test_done
>
> [1]: http://git.661346.n2.nabble.com/PATCH-add-option-n-no-checkout-to-git-worktree-add-tp7651385p7651884.html
>
> [2]: http://article.gmane.org/gmane.comp.version-control.git/290050

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

[Eric Sunshine]

[please don't top-post]

On Tue, Mar 29, 2016 at 11:11 PM, Zhang Lei <zhanglei002@gmail.com> wrote:
> Thanks for the review.
> Sorry for the patch churn, I wasn't quite familiar with working with
> mailing list.

No need to apologize. Reviewers understand what it is like being a
newcomer and provide additional review comments to help get up to
speed. Thanks for working on this enhancement.