threads / discuss / 45663

git work trees

Subject: git work trees

## tl;dr

10 messages between Apr 11, 2017 and Apr 24, 2017.

replies: 9people: 4as markdown or json

taylor, david· Apr 11, 2017, 15:14 UTC · lore
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.
Duy Nguyen· Apr 12, 2017, 13:36 UTC · re: taylor, david · lore

Re: git work trees

On Tue, Apr 11, 2017 at 10:14 PM, taylor, david <David.Taylor@dell.com> wrote:
Show 12 quoted lines
> 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)?

Show 5 quoted lines
> 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?

Show 5 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Apr 12, 2017, 13:58 UTC · re: taylor, david · lore

[PATCH] worktree add: add --lock option

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
Junio C Hamano· Apr 13, 2017, 22:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.
Duy Nguyen· Apr 14, 2017, 13:00 UTC · re: Junio C Hamano · lore

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

On Fri, Apr 14, 2017 at 5:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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
taylor, david· Apr 24, 2017, 14:06 UTC · re: Duy Nguyen · lore

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

Show 39 quoted lines
> 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
Jacob Keller· Apr 14, 2017, 16:27 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 13, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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

Junio C Hamano· Apr 15, 2017, 07:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 10 quoted lines
> -	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
Junio C Hamano· Apr 15, 2017, 08:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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' '
Duy Nguyen· Apr 15, 2017, 11:34 UTC · re: Junio C Hamano · lore

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

On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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.

Show 6 quoted lines
>> 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.

Show 50 quoted lines
>  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

← back to recent threads