diff mbox series

git-apply: add --quiet flag

Message ID 20210427194106.14500-1-jerry@skydio.com (mailing list archive)
State New
Headers show
Series git-apply: add --quiet flag | expand

Commit Message

Jerry Zhang April 27, 2021, 7:41 p.m. UTC
Replace OPT_VERBOSE with OPT_VERBOSITY.

This adds a --quiet flag to "git apply" so
the user can turn down the verbosity.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
 Documentation/git-apply.txt | 7 ++++++-
 apply.c                     | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Junio C Hamano April 28, 2021, 7:40 a.m. UTC | #1
Jerry Zhang <jerry@skydio.com> writes:

> Replace OPT_VERBOSE with OPT_VERBOSITY.

While it is not an incorrect statement, it is odd to have such an
implementation detail nobody cares as the first thing in the log
message, though.

> This adds a --quiet flag to "git apply" so
> the user can turn down the verbosity.

Sure, I think you can do "apply --no-verbose" to do the same thing
without any change, but we introduced VERBOSITY to replace VERBOSE
exactly so that --verbose can be countermanded with --quiet, and
this patch is a good example of the application of that feature.

I wonder if this deserves a test.

Also, does "git am" have an "--quiet" option (or "--verbose" for
that matter), and if so, should it pass it down to underlying "git
apply" (this is not a rhetorical suggestion --- it is a genuine
question---I am not particularly interested in changing "am")?

The patch text looks good.

Thanks.
Junio C Hamano April 28, 2021, 9:16 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Jerry Zhang <jerry@skydio.com> writes:
>
>> Replace OPT_VERBOSE with OPT_VERBOSITY.
>
> While it is not an incorrect statement, it is odd to have such an
> implementation detail nobody cares as the first thing in the log
> message, though.
>
>> This adds a --quiet flag to "git apply" so
>> the user can turn down the verbosity.
>
> Sure, I think you can do "apply --no-verbose" to do the same thing
> without any change, but we introduced VERBOSITY to replace VERBOSE
> exactly so that --verbose can be countermanded with --quiet, and
> this patch is a good example of the application of that feature.
>
> I wonder if this deserves a test.

Oh, another thing.  "--quiet" with OPT_VERBOSITY is given negative
values, whose magnitude may be used to express "even more quiet".
This is different from "--no-verbose" that is supported by both
OPT_VERBOSITY and OPT_VERBOSE that resets the variable to 0.

So use of OPT_VERBOSITY() to support both --verbose and --quiet is
good, but you'd need to audit the way the verbosity variable is used
by the code.  "if (verbose) perform_verbosely()" would have to be
rewritten as "if (verbose > verbosity_level) perform_verbosely()" 
or something like that, as the "verbose" variable can take a
negative value to mean "less silent than the usual 0".
Jerry Zhang April 28, 2021, 6:18 p.m. UTC | #3
On Wed, Apr 28, 2021 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Jerry Zhang <jerry@skydio.com> writes:
> >
> >> Replace OPT_VERBOSE with OPT_VERBOSITY.
> >
> > While it is not an incorrect statement, it is odd to have such an
> > implementation detail nobody cares as the first thing in the log
> > message, though.
> >
> >> This adds a --quiet flag to "git apply" so
> >> the user can turn down the verbosity.
> >
> > Sure, I think you can do "apply --no-verbose" to do the same thing
> > without any change, but we introduced VERBOSITY to replace VERBOSE
> > exactly so that --verbose can be countermanded with --quiet, and
> > this patch is a good example of the application of that feature.
> >
> > I wonder if this deserves a test.
>
> Oh, another thing.  "--quiet" with OPT_VERBOSITY is given negative
> values, whose magnitude may be used to express "even more quiet".
> This is different from "--no-verbose" that is supported by both
> OPT_VERBOSITY and OPT_VERBOSE that resets the variable to 0.
Ok I didn't realize every flag automatically came with a "no" version but
--quiet is indeed what I'm looking for since I want to silence the status
messages but still print out really critical errors.
>
> So use of OPT_VERBOSITY() to support both --verbose and --quiet is
> good, but you'd need to audit the way the verbosity variable is used
> by the code.  "if (verbose) perform_verbosely()" would have to be
> rewritten as "if (verbose > verbosity_level) perform_verbosely()"
> or something like that, as the "verbose" variable can take a
> negative value to mean "less silent than the usual 0".
Yeah luckily apply.c uses verbosity correctly and consistently
throughout.

> Also, does "git am" have an "--quiet" option (or "--verbose" for
> that matter), and if so, should it pass it down to underlying "git
> apply" (this is not a rhetorical suggestion --- it is a genuine
> question---I am not particularly interested in changing "am")?
am seems to have --quiet but not --verbose. In addition am does
not seem to pass through those options into apply.c. I tested this
using a patch that has whitespace errors and "git am" prints
warning regardless of whether "--quiet" is used. However, interestingly
"git am --3way" does not print warnings, due to these lines:

       /*
        * If we are allowed to fall back on 3-way merge, don't give false
        * errors during the initial attempt.
        */
       if (state->threeway && !index_file)
               apply_state.apply_verbosity = verbosity_silent;

Which no longer are relevant due to the previous changes making 3way
happen first. So in conclusion I think it might be worthwhile to make
the verbosity flags of "am" pass down to "apply", although it would be
out of the scope of this change.
diff mbox series

Patch

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index aa1ae56a25..a32ad64718 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@  SYNOPSIS
 	  [--ignore-space-change | --ignore-whitespace]
 	  [--whitespace=(nowarn|warn|fix|error|error-all)]
 	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
-	  [--verbose] [--unsafe-paths] [<patch>...]
+	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -228,6 +228,11 @@  behavior:
 	current patch being applied will be printed. This option will cause
 	additional information to be reported.
 
+-q::
+--quiet::
+	Suppress stderr output. Messages about patch status and progress
+	will not be printed.
+
 --recount::
 	Do not trust the line counts in the hunk headers, but infer them
 	by inspecting the patch (e.g. after editing the patch without
diff --git a/apply.c b/apply.c
index 7aa49e2048..918e0988bb 100644
--- a/apply.c
+++ b/apply.c
@@ -5051,7 +5051,7 @@  int apply_parse_options(int argc, const char **argv,
 			N_("leave the rejected hunks in corresponding *.rej files")),
 		OPT_BOOL(0, "allow-overlap", &state->allow_overlap,
 			N_("allow overlapping hunks")),
-		OPT__VERBOSE(&state->apply_verbosity, N_("be verbose")),
+		OPT__VERBOSITY(&state->apply_verbosity),
 		OPT_BIT(0, "inaccurate-eof", options,
 			N_("tolerate incorrectly detected missing new-line at the end of file"),
 			APPLY_OPT_INACCURATE_EOF),