RE: [PATCH] worktree add: add --lock option
> From: Duy Nguyen [mailto:pclo...@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 <gits...@pobox.com> > wrote: > > Nguyễn Thái Ngọc Duy <pclo...@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.tay...@dell.com> > >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@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
Re: [PATCH] worktree add: add --lock option
On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamanowrote: > Junio C Hamano writes: > >> Nguyễn Thái Ngọc Duy 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(, "%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(_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(); > strbuf_addf(, "%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(_env); > strbuf_release(); > strbuf_release(); > 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
Junio C Hamanowrites: > Nguyễn Thái Ngọc Duy 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(, "%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(_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(); strbuf_addf(, "%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(_env); strbuf_release(); strbuf_release(); 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
Nguyễn Thái Ngọc Duywrites: > - 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
On Thu, Apr 13, 2017 at 3:50 PM, Junio C Hamanowrote: > Nguyễn Thái Ngọc Duy 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 >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> 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
On Fri, Apr 14, 2017 at 5:50 AM, Junio C Hamanowrote: > Nguyễn Thái Ngọc Duy 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 >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> 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
Nguyễn Thái Ngọc Duywrites: > 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 > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > 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.