Re: [PATCH] worktree: accept -f as short for --force for removal

[Eric Sunshine]

On Tue, Apr 17, 2018 at 8:17 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> Makes sense. A possible rewrite (of the entire commit message):
>>
>> worktree: remove: recognize -f as short for --force
>>
>> Many commands support a --force option, frequently abbreviated as
>> -f, however, "git worktree remove"'s hand-rolled OPT_BOOL forgets
>> to recognize the short form, despite git-worktree.txt documenting
>> -f as supported. Replace OPT_BOOL with OPT__FORCE, which provides
>> -f for free, and makes 'remove' consistent with 'add' option
>> parsing (which also specifies the PARSE_OPT_NOCOMPLETE flag).
>>
> Looks better.  I am not sure if s/--force/-f/ in the synopsis
> section is warranted, but '-f' is commonly understood as '--force'
> (and that is the point of this patch after all), so it is probably
> an improvement to be briefer.

I meant to mention the synopsis change in the rewritten commit
message. The s/--force/-f/ for 'remove' makes it consistent with the
how it's shown for 'add' in the synopsis. (Changing it the other way,
so 'add' shows --force would also work.)

Re: [PATCH] worktree: accept -f as short for --force for removal

[Junio C Hamano]

Eric Sunshine  writes:

> On Tue, Apr 17, 2018 at 2:19 PM, Stefan Beller  wrote:
>> The force flag is very common in many commands and is commonly
>> abbreviated with '-f', so add that to worktree removal, too by using
>> OPT__FORCE instead of a self cooked OPT_BOOL for force.
>
> The missing bit of this sentence:
>
> ...self cooked OPT_BOOL for force which forgets '-f'.
>
>> While at it, add the PARSE_OPT_NOCOMPLETE flag to the force command line
>> option as forcing a removal may lose files.
>>
>> The short form of "-f" is already documented in the man page,
>> so we do not have to adjust the docs.
>
> Makes sense. A possible rewrite (of the entire commit message):
>
> worktree: remove: recognize -f as short for --force
>
> Many commands support a --force option, frequently abbreviated as
> -f, however, "git worktree remove"'s hand-rolled OPT_BOOL forgets
> to recognize the short form, despite git-worktree.txt documenting
> -f as supported. Replace OPT_BOOL with OPT__FORCE, which provides
> -f for free, and makes 'remove' consistent with 'add' option
> parsing (which also specifies the PARSE_OPT_NOCOMPLETE flag).
>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Stefan Beller 

Looks better.  I am not sure if s/--force/-f/ in the synopsis
section is warranted, but '-f' is commonly understood as '--force'
(and that is the point of this patch after all), so it is probably
an improvement to be briefer.

Thanks, both.

>
> The patch itself looks good. Thanks. With or without the above rewrite
> or minor adjustment, this patch is:
>
> Reviewed-by: Eric Sunshine 

Re: [PATCH] worktree: accept -f as short for --force for removal

[Eric Sunshine]

On Tue, Apr 17, 2018 at 2:19 PM, Stefan Beller  wrote:
> The force flag is very common in many commands and is commonly
> abbreviated with '-f', so add that to worktree removal, too by using
> OPT__FORCE instead of a self cooked OPT_BOOL for force.

The missing bit of this sentence:

...self cooked OPT_BOOL for force which forgets '-f'.

> While at it, add the PARSE_OPT_NOCOMPLETE flag to the force command line
> option as forcing a removal may lose files.
>
> The short form of "-f" is already documented in the man page,
> so we do not have to adjust the docs.

Makes sense. A possible rewrite (of the entire commit message):

worktree: remove: recognize -f as short for --force

Many commands support a --force option, frequently abbreviated as
-f, however, "git worktree remove"'s hand-rolled OPT_BOOL forgets
to recognize the short form, despite git-worktree.txt documenting
-f as supported. Replace OPT_BOOL with OPT__FORCE, which provides
-f for free, and makes 'remove' consistent with 'add' option
parsing (which also specifies the PARSE_OPT_NOCOMPLETE flag).

> Helped-by: Eric Sunshine 
> Signed-off-by: Stefan Beller 

The patch itself looks good. Thanks. With or without the above rewrite
or minor adjustment, this patch is:

Reviewed-by: Eric Sunshine 

[PATCH] worktree: accept -f as short for --force for removal

[Stefan Beller]

The force flag is very common in many commands and is commonly
abbreviated with '-f', so add that to worktree removal, too by using
OPT__FORCE instead of a self cooked OPT_BOOL for force.
While at it, add the PARSE_OPT_NOCOMPLETE flag to the force command line
option as forcing a removal may lose files.

The short form of "-f" is already documented in the man page,
so we do not have to adjust the docs.

Helped-by: Eric Sunshine 
Signed-off-by: Stefan Beller 
---

> Should this be using OPT__FORCE, rather than OPT_BOOL, to be
> consistent with worktree.c:add()?

yes it should.

> Also, can you amend the commit message to mention that "git worktree
> remove -f" was already documented, and that it was only the
> implementation which was lacking? Thanks.

done.
I tried to clear up the docs, but after reading them a couple times,
it looks good as-is.

 Documentation/git-worktree.txt | 2 +-
 builtin/worktree.c | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e7eb24ab85..99b713c849 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason ] 
 'git worktree move'  
 'git worktree prune' [-n] [-v] [--expire ]
-'git worktree remove' [--force] 
+'git worktree remove' [-f] 
 'git worktree unlock' 
 
 DESCRIPTION
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 40a438ed6c..30647b30c5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -783,8 +783,9 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
 {
int force = 0;
struct option options[] = {
-   OPT_BOOL(0, "force", ,
-N_("force removing even if the worktree is dirty")),
+   OPT__FORCE(,
+N_("force removing even if the worktree is dirty"),
+PARSE_OPT_NOCOMPLETE),
OPT_END()
};
struct worktree **worktrees, *wt;
-- 
2.17.0.255.g8bfb7c0704

Re: [PATCH] worktree: accept -f as short for --force for removal

[Eric Sunshine]

On Mon, Apr 16, 2018 at 6:16 PM, Stefan Beller  wrote:
> The force flag is very common in many commands and is commonly
> abbreviated with '-f', so add that to worktree removal, too
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -783,7 +783,7 @@ static int remove_worktree(int ac, const char **av, const 
> char *prefix)
>  {
> int force = 0;
> struct option options[] = {
> -   OPT_BOOL(0, "force", ,
> +   OPT_BOOL('f', "force", ,
>  N_("force removing even if the worktree is dirty")),

Should this be using OPT__FORCE, rather than OPT_BOOL, to be
consistent with worktree.c:add()?

Also, can you amend the commit message to mention that "git worktree
remove -f" was already documented, and that it was only the
implementation which was lacking? Thanks.

> OPT_END()
> };

[PATCH] worktree: accept -f as short for --force for removal

[Stefan Beller]

The force flag is very common in many commands and is commonly
abbreviated with '-f', so add that to worktree removal, too

Signed-off-by: Stefan Beller 
---

As a side note:

 $ git worktree add ../test HEAD^
 $ cd ../test
 $ make
 $ git worktree remove test
 fatal: working trees containing submodules cannot be moved or removed
 
 (This is on git.git, which does have submodules, but they should not 
 be relevant here? Instead we rather want to point out files left over.
 Not sure. I also plan on having worktrees and submodules to work well
 together eventually)


 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 40a438ed6c..d734874d58 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -783,7 +783,7 @@ static int remove_worktree(int ac, const char **av, const 
char *prefix)
 {
int force = 0;
struct option options[] = {
-   OPT_BOOL(0, "force", ,
+   OPT_BOOL('f', "force", ,
 N_("force removing even if the worktree is dirty")),
OPT_END()
};
-- 
2.17.0.484.g0c8726318c-goog