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

2017-04-24 Thread taylor, david 

> 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

2017-04-15 Thread Duy Nguyen 
On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano  wrote:
> 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

2017-04-15 Thread Junio C Hamano 
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);
>^
> 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

2017-04-15 Thread Junio C Hamano 
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

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

2017-04-14 Thread Jacob Keller 
On Thu, Apr 13, 2017 at 3:50 PM, Junio C Hamano  wrote:
> 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

2017-04-14 Thread Duy Nguyen 
On Fri, Apr 14, 2017 at 5:50 AM, Junio C Hamano  wrote:
> 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

2017-04-13 Thread Junio C Hamano 
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.

[PATCH] worktree add: add --lock option

2017-04-12 Thread 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 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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 ]  
[]
+'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] 
 []
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason ] 
 'git worktree prune' [-n] [-v] [--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();
strbuf_addf(, "%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(_env);
strbuf_release();
strbuf_release();
@@ -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", , N_("detach HEAD at named 
commit")),
OPT_BOOL(0, "checkout", , N_("populate the new 
working tree")),
+   OPT_BOOL(0, "lock", _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