git work trees
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.
Quoted 6 lines · 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.
Quoted 6 lines
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)?
Quoted 5 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?
Quoted 5 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.
Quoted 2 lines
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.
Signature · 1 line
Duy
PATCH 1/1 worktree add: add --lock option
In reply to taylor, davidAs 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.
---
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(-)
Documentation/git-worktree.txt +6 -1
@@ -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
builtin/worktree.c +11 -1
@@ -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() };
t/t2025-worktree-add.sh +6 -0
@@ -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 &&
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH] worktree add: add --lock option
Quoted 5 lines · 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).
Quoted 7 lines
"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 <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
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
Quoted 15 lines · Junio C Hamano
Quoted 5 lines · 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).
Quoted 7 lines
"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 <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
A patch that adds --lock may look like this.
Quoted 4 lines
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.
Quoted 3 lines
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.
Signature · 1 line
Duy
RE: [PATCH] worktree add: add --lock option
Quoted 34 lines
From: Duy Nguyen [mailto:[email protected]]
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 optionQuoted 5 lines · Junio C Hamano
Quoted 3 lines · 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,
Quoted 10 lines
Quoted 9 lines
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 <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
A patch that adds --lock may look like this.Quoted 4 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 2 lines
--
Duy
Re: [PATCH] worktree add: add --lock option
In reply to Junio C HamanoQuoted 20 lines · Junio C Hamano
Quoted 5 lines · 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).
Quoted 7 lines
"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 <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
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.
Quoted 5 lines
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
In reply to Nguyễn Thái Ngọc Duybuiltin/worktree.c static int add_worktree(const char *path, const char *refname, · line 317
Quoted 10 lines · Nguyễn Thái Ngọc Duy
builtin/worktree.c +9
-1-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
Quoted 15 lines · Junio C Hamano
Review
Quoted 10 lines · Nguyễn Thái Ngọc Duy
(quoted diff) +9
-1- 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 attributewarn_unused_result[-Werror=unused-result]
Quoted 4 lines
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(-)
builtin/worktree.c +7 -9
@@ -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);
t/t2025-worktree-add.sh +1 -2
@@ -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
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · Nguyễn Thái Ngọc Duy
(quoted diff) +9
-1- 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); + }
Quoted 4 lines
Quoted 4 lines
builtin/worktree.c: In function 'add_worktree':builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared with attributewarn_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.
Quoted 6 lines
Quoted 2 lines
cc1: all warnings being treated as errors
make: *** [builtin/worktree.o] Error 1I 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.
Quoted 44 lines
builtin/worktree.c | 16 +++++++--------- t/t2025-worktree-add.sh | 3 +-- 2 files changed, 8 insertions(+), 11 deletions(-)builtin/worktree.c +7
-9@@ -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);t/t2025-worktree-add.sh +0
-0
t/t2025-worktree-add.sh test_expect_success '"add" worktree' ' · line 68
Quoted 6 lines
t/t2025-worktree-add.sh +1
-2@@ -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.
Quoted 3 lines
t/t2025-worktree-add.sh +0
-0' test_expect_success '"add" worktree from a subdir' '
Signature · 1 line
Duy