git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] worktree: accept -f as short for --force for removal
@ 2018-04-16 22:16 Stefan Beller
  2018-04-17  0:21 ` Eric Sunshine
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Beller @ 2018-04-16 22:16 UTC (permalink / raw)
  To: git; +Cc: pclouds, 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 <sbeller@google.com>
---

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", &force,
+		OPT_BOOL('f', "force", &force,
 			 N_("force removing even if the worktree is dirty")),
 		OPT_END()
 	};
-- 
2.17.0.484.g0c8726318c-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] worktree: accept -f as short for --force for removal
  2018-04-16 22:16 [PATCH] worktree: accept -f as short for --force for removal Stefan Beller
@ 2018-04-17  0:21 ` Eric Sunshine
  2018-04-17 18:19   ` Stefan Beller
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sunshine @ 2018-04-17  0:21 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Git List, Nguyễn Thái Ngọc Duy

On Mon, Apr 16, 2018 at 6:16 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---
> 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", &force,
> +               OPT_BOOL('f', "force", &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()
>         };

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] worktree: accept -f as short for --force for removal
  2018-04-17  0:21 ` Eric Sunshine
@ 2018-04-17 18:19   ` Stefan Beller
  2018-04-17 18:46     ` Eric Sunshine
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Beller @ 2018-04-17 18:19 UTC (permalink / raw)
  To: sunshine; +Cc: git, pclouds, sbeller

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 <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

> 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 <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
-'git worktree remove' [--force] <worktree>
+'git worktree remove' [-f] <worktree>
 'git worktree unlock' <worktree>
 
 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", &force,
-			 N_("force removing even if the worktree is dirty")),
+		OPT__FORCE(&force,
+			 N_("force removing even if the worktree is dirty"),
+			 PARSE_OPT_NOCOMPLETE),
 		OPT_END()
 	};
 	struct worktree **worktrees, *wt;
-- 
2.17.0.255.g8bfb7c0704


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] worktree: accept -f as short for --force for removal
  2018-04-17 18:19   ` Stefan Beller
@ 2018-04-17 18:46     ` Eric Sunshine
  2018-04-18  0:17       ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sunshine @ 2018-04-17 18:46 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Git List, Nguyễn Thái Ngọc Duy

On Tue, Apr 17, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> 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 <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>

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

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] worktree: accept -f as short for --force for removal
  2018-04-17 18:46     ` Eric Sunshine
@ 2018-04-18  0:17       ` Junio C Hamano
  2018-04-18  5:01         ` Eric Sunshine
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2018-04-18  0:17 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Stefan Beller, Git List, Nguyễn Thái Ngọc Duy

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Apr 17, 2018 at 2:19 PM, Stefan Beller <sbeller@google.com> 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 <sunshine@sunshineco.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>

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 <sunshine@sunshineco.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] worktree: accept -f as short for --force for removal
  2018-04-18  0:17       ` Junio C Hamano
@ 2018-04-18  5:01         ` Eric Sunshine
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Sunshine @ 2018-04-18  5:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Stefan Beller, Git List, Nguyễn Thái Ngọc Duy

On Tue, Apr 17, 2018 at 8:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> 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.)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16 22:16 [PATCH] worktree: accept -f as short for --force for removal Stefan Beller
2018-04-17  0:21 ` Eric Sunshine
2018-04-17 18:19   ` Stefan Beller
2018-04-17 18:46     ` Eric Sunshine
2018-04-18  0:17       ` Junio C Hamano
2018-04-18  5:01         ` Eric Sunshine

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox