[PATCH v2] worktree: add --quiet option

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 [off-list ref]
Helped-by: Duy Nguyen [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Elia Pinto <redacted>
---
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

On 08/15, Elia Pinto wrote:

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!

On Wed, 15 Aug 2018 at 22:56, Elia Pinto [off-list ref] 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

On Thu, Aug 16, 2018 at 12:41 AM Martin Ågren [off-list ref] wrote:

On Wed, 15 Aug 2018 at 22:56, Elia Pinto [off-list ref] 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.

On Wed, Aug 15, 2018 at 4:56 PM Elia Pinto [off-list ref] 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.

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

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.

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.

Hi Eric,

On Thu, 16 Aug 2018 at 10:25, Eric Sunshine [off-list ref] 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