[PATCH v2] worktree: add --quiet option

[PATCH v2] worktree: add --quiet option

[Elia Pinto]

Add the '--quiet' option to git worktree,
as for the other git commands. 'add' is the
only command affected by it since all other
commands, except 'list', are currently
silent by default.

Helped-by: Martin Ågren <martin.agren@gmail.com>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
This is the second version of the patch.

Changes from the first version
(https://public-inbox.org/git/CACsJy8A=zp7nFBuWyfeP4UFf3KSsiaor3m0mtgVnhcEYHSw4HA@mail.gmail.com/T/):

- deleted garbage in git-worktree.c and deleted
superfluous blank line in git-worktree.txt.
- when giving "--quiet" to 'add', call git symbolic-ref also with
"--quiet".
- changed the commit message to be more general, but
specifying why the "--quiet" option is meaningful only for
the 'add' command of git-worktree.
- in git-worktree.txt the option
"--quiet" is described near the "--verbose" option.

 Documentation/git-worktree.txt |  4 ++++
 builtin/worktree.c             | 16 +++++++++++++---
 t/t2025-worktree-add.sh        |  5 +++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9c26be40f..29a5b7e25 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -173,6 +173,10 @@ This can also be set up as the default behaviour by using the
 	This format will remain stable across Git versions and regardless of user
 	configuration.  See below for details.
 
+-q::
+--quiet::
+	With 'add', suppress feedback messages.
+
 -v::
 --verbose::
 	With `prune`, report all removals.
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a763dbdcc..41e771439 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,6 +27,7 @@ static const char * const worktree_usage[] = {
 struct add_opts {
 	int force;
 	int detach;
+	int quiet;
 	int checkout;
 	int keep_locked;
 };
@@ -303,9 +304,13 @@ static int add_worktree(const char *path, const char *refname,
 	if (!is_branch)
 		argv_array_pushl(&cp.args, "update-ref", "HEAD",
 				 oid_to_hex(&commit->object.oid), NULL);
-	else
+	else {
 		argv_array_pushl(&cp.args, "symbolic-ref", "HEAD",
 				 symref.buf, NULL);
+		if (opts->quiet)
+			argv_array_push(&cp.args, "--quiet");
+	}
+
 	cp.env = child_env.argv;
 	ret = run_command(&cp);
 	if (ret)
@@ -315,6 +320,8 @@ static int add_worktree(const char *path, const char *refname,
 		cp.argv = NULL;
 		argv_array_clear(&cp.args);
 		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		if (opts->quiet)
+			argv_array_push(&cp.args, "--quiet");
 		cp.env = child_env.argv;
 		ret = run_command(&cp);
 		if (ret)
@@ -437,6 +444,7 @@ static int add(int ac, const char **av, const char *prefix)
 		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__QUIET(&opts.quiet, N_("suppress progress reporting")),
 		OPT_PASSTHRU(0, "track", &opt_track, NULL,
 			     N_("set up tracking mode (see git-branch(1))"),
 			     PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
@@ -491,8 +499,8 @@ static int add(int ac, const char **av, const char *prefix)
 			}
 		}
 	}
-
-	print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
+	if (!opts.quiet)
+		print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
 
 	if (new_branch) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -500,6 +508,8 @@ static int add(int ac, const char **av, const char *prefix)
 		argv_array_push(&cp.args, "branch");
 		if (new_branch_force)
 			argv_array_push(&cp.args, "--force");
+		if (opts.quiet)
+			argv_array_push(&cp.args, "--quiet");
 		argv_array_push(&cp.args, new_branch);
 		argv_array_push(&cp.args, branch);
 		if (opt_track)
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index be6e09314..658647d83 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -252,6 +252,11 @@ test_expect_success 'add -B' '
 	test_cmp_rev master^ poodle
 '
 
+test_expect_success 'add --quiet' '
+	git worktree add --quiet ../foo master >expected 2>&1 &&
+	test_must_be_empty expected
+'
+
 test_expect_success 'local clone from linked checkout' '
 	git clone --local here here-clone &&
 	( cd here-clone && git fsck )
-- 
2.18.0.723.g64e6cc43e.dirty

Re: [PATCH v2] worktree: add --quiet option

[Thomas Gummerer]

On 08/15, Elia Pinto wrote:
> Add the '--quiet' option to git worktree,
> as for the other git commands. 'add' is the
> only command affected by it since all other
> commands, except 'list', are currently
> silent by default.
> 
> Helped-by: Martin Ågren <martin.agren@gmail.com>
> Helped-by: Duy Nguyen <pclouds@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is the second version of the patch.
> 
> Changes from the first version
> (https://public-inbox.org/git/CACsJy8A=zp7nFBuWyfeP4UFf3KSsiaor3m0mtgVnhcEYHSw4HA@mail.gmail.com/T/):
> 
> - deleted garbage in git-worktree.c and deleted
> superfluous blank line in git-worktree.txt.
> - when giving "--quiet" to 'add', call git symbolic-ref also with
> "--quiet".
> - changed the commit message to be more general, but
> specifying why the "--quiet" option is meaningful only for
> the 'add' command of git-worktree.
> - in git-worktree.txt the option
> "--quiet" is described near the "--verbose" option.
> 
>  Documentation/git-worktree.txt |  4 ++++
>  builtin/worktree.c             | 16 +++++++++++++---
>  t/t2025-worktree-add.sh        |  5 +++++
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9c26be40f..29a5b7e25 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -173,6 +173,10 @@ This can also be set up as the default behaviour by using the
>  	This format will remain stable across Git versions and regardless of user
>  	configuration.  See below for details.
>  
> +-q::
> +--quiet::
> +	With 'add', suppress feedback messages.

Very minor nit here, we seem to use backticks everywhere else in this
document, maybe we sould do that here as well?  Not sure it's worth
another iteration though.

The rest of the patch looks good to me, thanks!

>  -v::
>  --verbose::
>  	With `prune`, report all removals.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index a763dbdcc..41e771439 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -27,6 +27,7 @@ static const char * const worktree_usage[] = {
>  struct add_opts {
>  	int force;
>  	int detach;
> +	int quiet;
>  	int checkout;
>  	int keep_locked;
>  };
> @@ -303,9 +304,13 @@ static int add_worktree(const char *path, const char *refname,
>  	if (!is_branch)
>  		argv_array_pushl(&cp.args, "update-ref", "HEAD",
>  				 oid_to_hex(&commit->object.oid), NULL);
> -	else
> +	else {
>  		argv_array_pushl(&cp.args, "symbolic-ref", "HEAD",
>  				 symref.buf, NULL);
> +		if (opts->quiet)
> +			argv_array_push(&cp.args, "--quiet");
> +	}
> +
>  	cp.env = child_env.argv;
>  	ret = run_command(&cp);
>  	if (ret)
> @@ -315,6 +320,8 @@ static int add_worktree(const char *path, const char *refname,
>  		cp.argv = NULL;
>  		argv_array_clear(&cp.args);
>  		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +		if (opts->quiet)
> +			argv_array_push(&cp.args, "--quiet");
>  		cp.env = child_env.argv;
>  		ret = run_command(&cp);
>  		if (ret)
> @@ -437,6 +444,7 @@ static int add(int ac, const char **av, const char *prefix)
>  		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__QUIET(&opts.quiet, N_("suppress progress reporting")),
>  		OPT_PASSTHRU(0, "track", &opt_track, NULL,
>  			     N_("set up tracking mode (see git-branch(1))"),
>  			     PARSE_OPT_NOARG | PARSE_OPT_OPTARG),
> @@ -491,8 +499,8 @@ static int add(int ac, const char **av, const char *prefix)
>  			}
>  		}
>  	}
> -
> -	print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
> +	if (!opts.quiet)
> +		print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
>  
>  	if (new_branch) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
> @@ -500,6 +508,8 @@ static int add(int ac, const char **av, const char *prefix)
>  		argv_array_push(&cp.args, "branch");
>  		if (new_branch_force)
>  			argv_array_push(&cp.args, "--force");
> +		if (opts.quiet)
> +			argv_array_push(&cp.args, "--quiet");
>  		argv_array_push(&cp.args, new_branch);
>  		argv_array_push(&cp.args, branch);
>  		if (opt_track)
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index be6e09314..658647d83 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -252,6 +252,11 @@ test_expect_success 'add -B' '
>  	test_cmp_rev master^ poodle
>  '
>  
> +test_expect_success 'add --quiet' '
> +	git worktree add --quiet ../foo master >expected 2>&1 &&
> +	test_must_be_empty expected
> +'
> +
>  test_expect_success 'local clone from linked checkout' '
>  	git clone --local here here-clone &&
>  	( cd here-clone && git fsck )
> -- 
> 2.18.0.723.g64e6cc43e.dirty
> 

Re: [PATCH v2] worktree: add --quiet option

[Martin Ågren]

On Wed, 15 Aug 2018 at 22:56, Elia Pinto <gitter.spiros@gmail.com> wrote:
> Add the '--quiet' option to git worktree,
> as for the other git commands. 'add' is the
> only command affected by it since all other
> commands, except 'list', are currently
> silent by default.

Thanks for a follow-up.

The word "currently" means I can't shake the feeling that Eric has a
very good point in [1]:

  It might make sense to say instead that this is adding a --quiet
  option _in general_, rather than doing so specifically for 'add'.

As an example, if `git worktree move` ever learns to produce some sort
of output, then Eric's approach would mean that such a hypothetical
`move` is buggy until it learns to respect `--quiet`. With your
approach, it would mean that we would get feature requests that `move`
should also respect `--quiet` (which we would then need to redefine in
the documentation) or that it should learn of a `--quiet-move` (which I
do not think is a particularly good UI).

Doing such a patch instead would mean tweaking the commit message
slightly...

  Add the '--quiet' option to git worktree, as for the other git
  commands. Currently, 'add' is the only command affected by it since
  all other commands, except 'list' obviously, are already silent by
  default.

... and the documentation slightly ...

  Suppress feedback messages.

It might make sense adding a comment to the documentation about how this
currently only affects `add`, but such comments do risk going stale. I
am on the fence about whether the documentation needs to say something
about `list` -- who in their right mind would expect `list --quiet` to
actually suppress the list? And if `list` ever learns to give some
feedback messages in addition to the list itself, then we would want
`--quiet` to suppress *those*, I guess.

Others might disagree violently with this, but I wonder whether it makes
sense to add a test to verify that, e.g., `git worktree move --quiet` is
quiet. Such a test would demonstrate that this is your intention, i.e.,
anyone digging into a report on "why does git worktree foo not respect
--quiet?" would be 100% convinced that your intention back in 2018 really
was to respect it everywhere. It seems wasteful to add such a test for
all other modes, but maybe you can identify one which you think has a
higher risk of learning to output some feedback in the future.

To be clear, it is fine for you to disagree with the above! :-)

>         }
> -
> -       print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);
> +       if (!opts.quiet)
> +               print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);

I think that empty line should be kept. Probably not worth a reroll.

Good work!

[1] https://public-inbox.org/git/CAPig+cS-b2yL2FeLRzS+jW-O5fRd1g8Kqak7j1QX5PRP0ojQEQ@mail.gmail.com/

Martin

Re: [PATCH v2] worktree: add --quiet option

[Eric Sunshine]

On Thu, Aug 16, 2018 at 12:41 AM Martin Ågren <martin.agren@gmail.com> wrote:
> On Wed, 15 Aug 2018 at 22:56, Elia Pinto <gitter.spiros@gmail.com> wrote:
> The word "currently" means I can't shake the feeling that Eric has a
> very good point in [1]:
>
>   It might make sense to say instead that this is adding a --quiet
>   option _in general_, rather than doing so specifically for 'add'.
>
> As an example, if `git worktree move` ever learns to produce some sort
> of output, then Eric's approach would mean that such a hypothetical
> `move` is buggy until it learns to respect `--quiet`. With your
> approach, it would mean that we would get feature requests that `move`
> should also respect `--quiet` [...]
>
> Doing such a patch instead would mean tweaking the commit message
> slightly...
>
>   Add the '--quiet' option to git worktree, as for the other git
>   commands. Currently, 'add' is the only command affected by it since
>   all other commands, except 'list' obviously, are already silent by
>   default.
>
> ... and the documentation slightly ...
>
>   Suppress feedback messages.

This is a sensible suggestion for the documentation rather than having
it call out "add" as special (unlike the commit message in which case
it makes sense to mention that only the behavior of "add" is
affected).

> It might make sense adding a comment to the documentation about how this
> currently only affects `add`, but such comments do risk going stale.

Let's not mention "add" or any other command specially since the
option is meant to be general.

> I am on the fence about whether the documentation needs to say something
> about `list` -- who in their right mind would expect `list --quiet` to
> actually suppress the list?

(/me nudges Martin off the fence onto the side of not bothering to
mention the obvious)

> Others might disagree violently with this, but I wonder whether it makes
> sense to add a test to verify that, e.g., `git worktree move --quiet` is
> quiet. Such a test would demonstrate that this is your intention, i.e.,
> anyone digging into a report on "why does git worktree foo not respect
> --quiet?" would be 100% convinced that your intention back in 2018 really
> was to respect it everywhere. It seems wasteful to add such a test for
> all other modes, but maybe you can identify one which you think has a
> higher risk of learning to output some feedback in the future.

My knee-jerk reaction was that such tests seem unnecessary, but I
think you convinced me that they would make sense to avoid future
headaches since --quiet should indeed mean "quiet" generally. (Newly
added worktree commands would not be protected by such tests added
today, so it's not foolproof, but still better than nothing.)

Having said that, though, lack of such tests shouldn't block this
patch from being accepted. They can always be added later if someone
wants to do it.

Re: [PATCH v2] worktree: add --quiet option

[Eric Sunshine]

On Wed, Aug 15, 2018 at 4:56 PM Elia Pinto <gitter.spiros@gmail.com> wrote:
> Add the '--quiet' option to git worktree,
> as for the other git commands. 'add' is the
> only command affected by it since all other
> commands, except 'list', are currently
> silent by default.

Nit: wrap the commit message at around 70 columns rather than 45 or so.

> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -303,9 +304,13 @@ static int add_worktree(const char *path, const char *refname,
> +       else {
>                 argv_array_pushl(&cp.args, "symbolic-ref", "HEAD",
>                                  symref.buf, NULL);
> +               if (opts->quiet)
> +                       argv_array_push(&cp.args, "--quiet");
> +       }

This constructs the command as "git symbolic-ref HEAD <ref> --quiet".
Although that's not wrong per-se, it does perhaps set an undesirable
precedent. A more standard construction would be:

    argv_array_push(..., "symbolic-ref");
    if (opts->quiet)
        argv_array_push(..., "--quiet");
    argv_array_pushl(..., "HEAD", symref.buf, NULL);

I used "pushl" on the last one to indicate the semantic relationship
between those two arguments.

> +

Nit: the above code is directly related to the code just below, so the
new blank line you add here somewhat (undesirably) divorces the pieces
of code from each other

>         cp.env = child_env.argv;
>         ret = run_command(&cp);
>         if (ret)
> @@ -315,6 +320,8 @@ static int add_worktree(const char *path, const char *refname,
>                 argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +               if (opts->quiet)
> +                       argv_array_push(&cp.args, "--quiet");

I could also see this one re-written as:

    argv_array_push(...,"reset");
    argv_array_push(...,"--hard");
    if (opts->quiet)
        argv_array_push(...,"--quiet");

now that the command invocation has added another option.

Not at all worth a re-roll.

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -252,6 +252,11 @@ test_expect_success 'add -B' '
> +test_expect_success 'add --quiet' '
> +       git worktree add --quiet ../foo master >expected 2>&1 &&
> +       test_must_be_empty expected
> +'

This test is going to create the new worktree directly in Git's t/
directory due to "../foo". Please don't do that. Use a name without
the leading "../".

Also, you should make sure that the worktree doesn't already exist
(wasn't created by an earlier test), otherwise the "git worktree add"
command will fail. So, either choose a distinct worktree name or use
"test_might_fail git worktree remove -f <worktree>" before the "git
worktree add" command.

Re: [PATCH v2] worktree: add --quiet option

[Martin Ågren]

Hi Eric,

On Thu, 16 Aug 2018 at 10:25, Eric Sunshine <sunshine@sunshineco.com> wrote:
> (/me nudges Martin off the fence onto the side of not bothering to
> mention the obvious)

:-)

Thanks for sanity-checking my thoughts. I agree with everything you
wrote in your reply (and, FWIW, your other findings that you sent
separately).

Martin

Re: [PATCH v2] worktree: add --quiet option

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index be6e09314..658647d83 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -252,6 +252,11 @@ test_expect_success 'add -B' '
>  	test_cmp_rev master^ poodle
>  '
>  
> +test_expect_success 'add --quiet' '
> +	git worktree add --quiet ../foo master >expected 2>&1 &&
> +	test_must_be_empty expected
> +'

That's misnomer.  Unless existing tests in this file are already
bogus, I'd like to see it called 'actual', which is the name we use
to store the actual output (to be compared with another file we
create to hold the expected output, typically called 'expect', like
"test_cmp expect actual").

I noticed the breakage after merging this to 'pu'; it seems to die
with "fatal: ../foo already exists" which comes from die().

Oh, more seriously, since when is it OK to muck with stuff _outside_
the $TRASH_DIRECTORY, e.g. "../foo", which would contaminate t/
directory by creating a direct subdirectly under it?

Ahh, and I suspect that it is exactly why I am seeing a failure you
did not see---from a previously failed test cycle, "t/foo" is left
behind because "make distclean" would not clean it (of course).

Do not ever touch anywhere outside $TRASH_DIRECTORY.  Is this
something we could enforce in our test harness, I wonder...

>  test_expect_success 'local clone from linked checkout' '
>  	git clone --local here here-clone &&
>  	( cd here-clone && git fsck )

Re: [PATCH v2] worktree: add --quiet option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
>> index be6e09314..658647d83 100755
>> --- a/t/t2025-worktree-add.sh
>> +++ b/t/t2025-worktree-add.sh
>> @@ -252,6 +252,11 @@ test_expect_success 'add -B' '
>>  	test_cmp_rev master^ poodle
>>  '
>>  
>> +test_expect_success 'add --quiet' '
>> +	git worktree add --quiet ../foo master >expected 2>&1 &&
>> +	test_must_be_empty expected
>> +'
>
> That's misnomer.  Unless existing tests in this file are already
> bogus, I'd like to see it called 'actual', which is the name we use
> to store the actual output (to be compared with another file we
> create to hold the expected output, typically called 'expect', like
> "test_cmp expect actual").
>
> I noticed the breakage after merging this to 'pu'; it seems to die
> with "fatal: ../foo already exists" which comes from die().
>
> Oh, more seriously, since when is it OK to muck with stuff _outside_
> the $TRASH_DIRECTORY, e.g. "../foo", which would contaminate t/
> directory by creating a direct subdirectly under it?
>
> Ahh, and I suspect that it is exactly why I am seeing a failure you
> did not see---from a previously failed test cycle, "t/foo" is left
> behind because "make distclean" would not clean it (of course).
>
> Do not ever touch anywhere outside $TRASH_DIRECTORY.  Is this
> something we could enforce in our test harness, I wonder...
>
>>  test_expect_success 'local clone from linked checkout' '
>>  	git clone --local here here-clone &&
>>  	( cd here-clone && git fsck )

A quickfix (I wish I had a lot more time to spend to be extra
careful, bit I don't) I'll apply for now to get going...

 t/t2025-worktree-add.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 658647d834..c674697913 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -253,8 +253,8 @@ test_expect_success 'add -B' '
 '
 
 test_expect_success 'add --quiet' '
-	git worktree add --quiet ../foo master >expected 2>&1 &&
-	test_must_be_empty expected
+	git worktree add --quiet another-worktree master 2>actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'local clone from linked checkout' '