git work trees

git work trees

[taylor, david]

We are using Git in a distributed environment.

In the United States, we have the master repository in one state and a build cluster in a different state.
In addition to people in the US doing builds, we have people in other countries (Ireland, India, Israel,
Russia, possibly others) doing builds -- using the build cluster.

The local mirror of the repository is NFS accessible.  The plan is to make builds faster through the use
of work trees.  The build cluster nodes involved in the build will have a worktree in RAM -- checked out
for the duration of the build.  Since the worktree is in RAM, it will not be NFS accessible.

[Cloning takes 20+ minutes when the network is unloaded.  Building, with sources NFS mounted, takes
5-10 minutes.]

This presents a few problems.

When we are done with a work tree, we want to clean up -- think: prune.  But, you cannot prune just
one worktree; you have to prune the set.  And no machine has access to all the worktrees.  So, no
machine knows which ones are prunable.

There is no 'lock' option to 'add'.  If someone does a 'prune' after you do an 'add' and before you do a
'lock', then your 'add' is undone.

Are there any plans to add a '[--lock]' option to 'add' to create the worktree in the locked state?  And/or
plans to add a [<path>...] option to prune to say 'prune only this path / these paths'?

If there are no plans, is the above an acceptable interface?  And if we implemented it, would it be looked
upon favorably?

Thanks.

Re: git work trees

[Duy Nguyen]

On Tue, Apr 11, 2017 at 10:14 PM, taylor, david <David.Taylor@dell.com> wrote:
> We are using Git in a distributed environment.
>
> In the United States, we have the master repository in one state and a build cluster in a different state.
> In addition to people in the US doing builds, we have people in other countries (Ireland, India, Israel,
> Russia, possibly others) doing builds -- using the build cluster.
>
> The local mirror of the repository is NFS accessible.  The plan is to make builds faster through the use
> of work trees.  The build cluster nodes involved in the build will have a worktree in RAM -- checked out
> for the duration of the build.  Since the worktree is in RAM, it will not be NFS accessible.
>
> [Cloning takes 20+ minutes when the network is unloaded.  Building, with sources NFS mounted, takes
> 5-10 minutes.]

Using worktrees in this scenario kinda defeats the distributed nature
of git. Cloning takes long, yes. But what about just "git pull" (or
"git fetch && git checkout -f" if you want to avoid merging)?

> This presents a few problems.
>
> When we are done with a work tree, we want to clean up -- think: prune.  But, you cannot prune just
> one worktree; you have to prune the set.  And no machine has access to all the worktrees.  So, no
> machine knows which ones are prunable.

By "prune one worktree", did you mean delete one? Or delete a branch
the worktree uses and prune the object database?

> There is no 'lock' option to 'add'.  If someone does a 'prune' after you do an 'add' and before you do a
> 'lock', then your 'add' is undone.
>
> Are there any plans to add a '[--lock]' option to 'add' to create the worktree in the locked state?  And/or
> plans to add a [<path>...] option to prune to say 'prune only this path / these paths'?

So this is "git worktree prune". Adding "worktree add --locked" sounds
reasonable (and quite simple too, because "worktree add" does lock the
worktree at creation time; we just need to stop it from releasing the
lock). I might be able to do it quickly (it does not mean "available
in the next release" though).

If you need to just prune "this path", I think it's the equivalent of
"git worktree remove" (i.e. delete a specific worktree). Work has been
going on for a while to add that command. Maybe it'll be available
later this year.

> If there are no plans, is the above an acceptable interface?  And if we implemented it, would it be looked
> upon favorably?

Speaking of this use case (and this is my own opinion) I think this is
stretching "git worktree" too much. When I created it, I imagined this
functionality to be used by a single person.
-- 
Duy

[PATCH] worktree add: add --lock option

[Nguyễn Thái Ngọc Duy]

As explained in the document. This option has an advantage over the
command sequence "git worktree add && git worktree lock": there will be
no gap that somebody can accidentally "prune" the new worktree (or soon,
explicitly "worktree remove" it).

"worktree add" does keep a lock on while it's preparing the worktree.
If --lock is specified, this lock remains after the worktree is created.

Suggested-by: David Taylor <David.Taylor@dell.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 A patch that adds --lock may look like this.

 Documentation/git-worktree.txt |  7 ++++++-
 builtin/worktree.c             | 12 +++++++++++-
 t/t2025-worktree-add.sh        |  6 ++++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 553cf8413f..b472acc356 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] [--checkout] [-b <new-branch>] <path> [<branch>]
+'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree prune' [-n] [-v] [--expire <expire>]
@@ -107,6 +107,11 @@ OPTIONS
 	such as configuring sparse-checkout. See "Sparse checkout"
 	in linkgit:git-read-tree[1].
 
+--lock::
+	Keep the working tree locked after creation. This is the
+	equivalent of `git worktree lock` after `git worktree add`,
+	but without race condition.
+
 -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 9993ded41a..3dab07c829 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -24,6 +24,7 @@ struct add_opts {
 	int force;
 	int detach;
 	int checkout;
+	int keep_locked;
 	const char *new_branch;
 	int force_new_branch;
 };
@@ -305,7 +306,15 @@ static int add_worktree(const char *path, const char *refname,
 done:
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	unlink_or_warn(sb.buf);
+	if (!ret && opts->keep_locked) {
+		/*
+		 * Don't keep the confusing "initializing" message
+		 * after it's already over.
+		 */
+		truncate(sb.buf, 0);
+	} else {
+		unlink_or_warn(sb.buf);
+	}
 	argv_array_clear(&child_env);
 	strbuf_release(&sb);
 	strbuf_release(&symref);
@@ -328,6 +337,7 @@ static int add(int ac, const char **av, const char *prefix)
 			   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_BOOL(0, "lock", &opts.keep_locked, N_("keep the new working tree locked")),
 		OPT_END()
 	};
 
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index b618d6be21..6dce920c03 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -63,6 +63,12 @@ test_expect_success '"add" worktree' '
 	)
 '
 
+test_expect_success '"add" worktree with lock' '
+	git rev-parse HEAD >expect &&
+	git worktree add --detach --lock here-with-lock master &&
+	test_must_be_empty .git/worktrees/here-with-lock/locked
+'
+
 test_expect_success '"add" worktree from a subdir' '
 	(
 		mkdir sub &&
-- 
2.11.0.157.gd943d85

Re: [PATCH] worktree add: add --lock option

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> As explained in the document. This option has an advantage over the
> command sequence "git worktree add && git worktree lock": there will be
> no gap that somebody can accidentally "prune" the new worktree (or soon,
> explicitly "worktree remove" it).
>
> "worktree add" does keep a lock on while it's preparing the worktree.
> If --lock is specified, this lock remains after the worktree is created.
>
> Suggested-by: David Taylor <David.Taylor@dell.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  A patch that adds --lock may look like this.

This looks more like "I do believe the idea by David is a useful
addition and here is how I did it to the best of my ability---let's
make sure we polish it for eventual inclusion" than a mere "it may
look like so---do whatever you want with it" patch.

To me "git worktree add --lock" somehow sounds less correct than
"git worktree add --locked", but I'd appreciate if natives can
correct me.

Thanks.

Re: [PATCH] worktree add: add --lock option

[Duy Nguyen]

On Fri, Apr 14, 2017 at 5:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> As explained in the document. This option has an advantage over the
>> command sequence "git worktree add && git worktree lock": there will be
>> no gap that somebody can accidentally "prune" the new worktree (or soon,
>> explicitly "worktree remove" it).
>>
>> "worktree add" does keep a lock on while it's preparing the worktree.
>> If --lock is specified, this lock remains after the worktree is created.
>>
>> Suggested-by: David Taylor <David.Taylor@dell.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  A patch that adds --lock may look like this.
>
> This looks more like "I do believe the idea by David is a useful
> addition and here is how I did it to the best of my ability---let's
> make sure we polish it for eventual inclusion" than a mere "it may
> look like so---do whatever you want with it" patch.

It is a good addition, which is why I added tests and documents, so it
may have a chance for inclusion. I would not strongly defend it though
if there's objection.

> To me "git worktree add --lock" somehow sounds less correct than
> "git worktree add --locked", but I'd appreciate if natives can
> correct me.

That was my first choice too. Then I saw --detach (instead of
--detached). I didn't care much and went with a verb like the rest.
-- 
Duy

Re: [PATCH] worktree add: add --lock option

[Jacob Keller]

On Thu, Apr 13, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> As explained in the document. This option has an advantage over the
>> command sequence "git worktree add && git worktree lock": there will be
>> no gap that somebody can accidentally "prune" the new worktree (or soon,
>> explicitly "worktree remove" it).
>>
>> "worktree add" does keep a lock on while it's preparing the worktree.
>> If --lock is specified, this lock remains after the worktree is created.
>>
>> Suggested-by: David Taylor <David.Taylor@dell.com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  A patch that adds --lock may look like this.
>
> This looks more like "I do believe the idea by David is a useful
> addition and here is how I did it to the best of my ability---let's
> make sure we polish it for eventual inclusion" than a mere "it may
> look like so---do whatever you want with it" patch.
>
> To me "git worktree add --lock" somehow sounds less correct than
> "git worktree add --locked", but I'd appreciate if natives can
> correct me.
>
> Thanks.

I think either "--lock" or "--locked" works for me. "--locked'
suggests "this is the state I want the tree in" while "--lock"
suggests "this is the action I want taken on the tree".

Thanks,
Jake

Re: [PATCH] worktree add: add --lock option

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> -	unlink_or_warn(sb.buf);
> +	if (!ret && opts->keep_locked) {
> +		/*
> +		 * Don't keep the confusing "initializing" message
> +		 * after it's already over.
> +		 */
> +		truncate(sb.buf, 0);
> +	} else {
> +		unlink_or_warn(sb.buf);
> +	}

builtin/worktree.c: In function 'add_worktree':
builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared with attribute warn_unused_result [-Werror=unused-result]
   truncate(sb.buf, 0);
           ^
cc1: all warnings being treated as errors
make: *** [builtin/worktree.o] Error 1

Re: [PATCH] worktree add: add --lock option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> -	unlink_or_warn(sb.buf);
>> +	if (!ret && opts->keep_locked) {
>> +		/*
>> +		 * Don't keep the confusing "initializing" message
>> +		 * after it's already over.
>> +		 */
>> +		truncate(sb.buf, 0);
>> +	} else {
>> +		unlink_or_warn(sb.buf);
>> +	}
>
> builtin/worktree.c: In function 'add_worktree':
> builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared with attribute warn_unused_result [-Werror=unused-result]
>    truncate(sb.buf, 0);
>            ^
> cc1: all warnings being treated as errors
> make: *** [builtin/worktree.o] Error 1

I wonder why we need to have "initializing" and then remove the
string.  Wouldn't it be simpler to do something like this instead?
Does an empty lockfile have some special meaning?

 builtin/worktree.c      | 16 +++++++---------
 t/t2025-worktree-add.sh |  3 +--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 3dab07c829..5ebdcce793 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -243,7 +243,10 @@ static int add_worktree(const char *path, const char *refname,
 	 * after the preparation is over.
 	 */
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	write_file(sb.buf, "initializing");
+	if (!opts->keep_locked)
+		write_file(sb.buf, "initializing");
+	else
+		write_file(sb.buf, "added with --lock");
 
 	strbuf_addf(&sb_git, "%s/.git", path);
 	if (safe_create_leading_directories_const(sb_git.buf))
@@ -306,15 +309,10 @@ static int add_worktree(const char *path, const char *refname,
 done:
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
-	if (!ret && opts->keep_locked) {
-		/*
-		 * Don't keep the confusing "initializing" message
-		 * after it's already over.
-		 */
-		truncate(sb.buf, 0);
-	} else {
+	if (!ret && opts->keep_locked)
+		;
+	else
 		unlink_or_warn(sb.buf);
-	}
 	argv_array_clear(&child_env);
 	strbuf_release(&sb);
 	strbuf_release(&symref);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 6dce920c03..304f25fcd1 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -65,8 +65,7 @@ test_expect_success '"add" worktree' '
 
 test_expect_success '"add" worktree with lock' '
 	git rev-parse HEAD >expect &&
-	git worktree add --detach --lock here-with-lock master &&
-	test_must_be_empty .git/worktrees/here-with-lock/locked
+	git worktree add --detach --lock here-with-lock master
 '
 
 test_expect_success '"add" worktree from a subdir' '

Re: [PATCH] worktree add: add --lock option

[Duy Nguyen]

On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> -    unlink_or_warn(sb.buf);
>>> +    if (!ret && opts->keep_locked) {
>>> +            /*
>>> +             * Don't keep the confusing "initializing" message
>>> +             * after it's already over.
>>> +             */
>>> +            truncate(sb.buf, 0);
>>> +    } else {
>>> +            unlink_or_warn(sb.buf);
>>> +    }
>>
>> builtin/worktree.c: In function 'add_worktree':
>> builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared with attribute warn_unused_result [-Werror=unused-result]
>>    truncate(sb.buf, 0);
>>            ^

Yes it's supposed to be safe to ignore the error in this case. I
thought of adding (void) last minute but didn't do it.


>> cc1: all warnings being treated as errors
>> make: *** [builtin/worktree.o] Error 1
>
> I wonder why we need to have "initializing" and then remove the
> string.  Wouldn't it be simpler to do something like this instead?
> Does an empty lockfile have some special meaning?

It's mostly for troubleshooting. If "git worktree add" fails in a
later phase with a valid/locked worktree remains, this gives us a
clue.

>  builtin/worktree.c      | 16 +++++++---------
>  t/t2025-worktree-add.sh |  3 +--
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 3dab07c829..5ebdcce793 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -243,7 +243,10 @@ static int add_worktree(const char *path, const char *refname,
>          * after the preparation is over.
>          */
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> -       write_file(sb.buf, "initializing");
> +       if (!opts->keep_locked)
> +               write_file(sb.buf, "initializing");
> +       else
> +               write_file(sb.buf, "added with --lock");
>
>         strbuf_addf(&sb_git, "%s/.git", path);
>         if (safe_create_leading_directories_const(sb_git.buf))
> @@ -306,15 +309,10 @@ static int add_worktree(const char *path, const char *refname,
>  done:
>         strbuf_reset(&sb);
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> -       if (!ret && opts->keep_locked) {
> -               /*
> -                * Don't keep the confusing "initializing" message
> -                * after it's already over.
> -                */
> -               truncate(sb.buf, 0);
> -       } else {
> +       if (!ret && opts->keep_locked)
> +               ;
> +       else
>                 unlink_or_warn(sb.buf);
> -       }
>         argv_array_clear(&child_env);
>         strbuf_release(&sb);
>         strbuf_release(&symref);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 6dce920c03..304f25fcd1 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -65,8 +65,7 @@ test_expect_success '"add" worktree' '
>
>  test_expect_success '"add" worktree with lock' '
>         git rev-parse HEAD >expect &&
> -       git worktree add --detach --lock here-with-lock master &&
> -       test_must_be_empty .git/worktrees/here-with-lock/locked
> +       git worktree add --detach --lock here-with-lock master

I think you still need to check for the presence of "locked" file.

>  '
>
>  test_expect_success '"add" worktree from a subdir' '
-- 
Duy

RE: [PATCH] worktree add: add --lock option

[taylor, david]

> From: Duy Nguyen [mailto:pclouds@gmail.com]
> Sent: Friday, April 14, 2017 9:01 AM
> To: Junio C Hamano
> Cc: Git Mailing List; taylor, david
> Subject: Re: [PATCH] worktree add: add --lock option
> 
> On Fri, Apr 14, 2017 at 5:50 AM, Junio C Hamano <gitster@pobox.com>
> wrote:
> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> >
> >> As explained in the document. This option has an advantage over the
> >> command sequence "git worktree add && git worktree lock": there will be
> >> no gap that somebody can accidentally "prune" the new worktree (or
> soon,
> >> explicitly "worktree remove" it).
> >>
> >> "worktree add" does keep a lock on while it's preparing the worktree.
> >> If --lock is specified, this lock remains after the worktree is created.
> >>
> >> Suggested-by: David Taylor <David.Taylor@dell.com>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >> ---
> >>  A patch that adds --lock may look like this.
> >
> > This looks more like "I do believe the idea by David is a useful
> > addition and here is how I did it to the best of my ability---let's
> > make sure we polish it for eventual inclusion" than a mere "it may
> > look like so---do whatever you want with it" patch.
> 
> It is a good addition, which is why I added tests and documents, so it
> may have a chance for inclusion. I would not strongly defend it though
> if there's objection.
> 
> > To me "git worktree add --lock" somehow sounds less correct than
> > "git worktree add --locked", but I'd appreciate if natives can
> > correct me.
> 
> That was my first choice too. Then I saw --detach (instead of
> --detached). I didn't care much and went with a verb like the rest.

While I personally would prefer --locked, I also prefer keeping it 'parallel
construction' with --detach.  That is either [--detach][--lock] or
[--detached][--locked].  But, ultimately, my intended  use is within a script,
so even if it was --totally-non-mnemonic-option I would cope.

A stronger desire, regardless of whether it's Duy's implementation, mine, or
someone elses, is to have something acceptable to the community so that
we are not maintaining a fork with the attendant need to merge changes
in each time we upgrade.

> --
> Duy