Re: [PATCH v13 10/14] apply: change error_routine when silent

From: Stefan Beller <sbeller@google.com>
Date: Aug 31, 2016, 22:20 UTC
Message-ID: <CAGZ79kbdoF-1=ZBZG8y3sEz6LVaNP4Ou+KVk+=M7y9PSzQ1J0Q@mail.gmail.com>
In-Reply-To: <20160827184547.4365-11-chriscool@tuxfamily.org>

On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 23 quoted lines

> To avoid printing anything when applying with
> `state->apply_verbosity == verbosity_silent`, let's save the
> existing warn and error routines before applying, and let's
> replace them with a routine that does nothing.
>
> Then after applying, let's restore the saved routines.
>
> Helped-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  apply.c | 21 ++++++++++++++++++++-
>  apply.h |  8 ++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index ddbb0a2..bf81b70 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state)
>         /* &state->fn_table is cleared at the end of apply_patch() */
>  }
>
> +static void mute_routine(const char *bla, va_list params)

Instead of 'bla' you could go with 'format' as the man page for [f]printf puts it. Or you could leave it empty, i.e.

    static void mute_routine(const char *, va_list)
    ...

I personally do not mind bla, as I know that the first parameter is supposed to be the format, but let's not add unneeded information. (Side question: Is there a culture that doesn't recognize 'bla' as a fill word?)

Show 27 quoted lines

> +{
> +       /* do nothing */
> +}
> +
>  int check_apply_state(struct apply_state *state, int force_apply)
>  {
>         int is_not_gitdir = !startup_info->have_repository;
> @@ -144,6 +149,13 @@ int check_apply_state(struct apply_state *state, int force_apply)
>         if (!state->lock_file)
>                 return error("BUG: state->lock_file should not be NULL");
>
> +       if (state->apply_verbosity <= verbosity_silent) {
> +               state->saved_error_routine = get_error_routine();
> +               state->saved_warn_routine = get_warn_routine();
> +               set_error_routine(mute_routine);
> +               set_warn_routine(mute_routine);
> +       }
> +
>         return 0;
>  }
>
> @@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state,
>                 state->newfd = -1;
>         }
>
> -       return !!errs;
> +       res = !!errs;

I am trying to understand this and the next chunk (they work together?)

Show 15 quoted lines

>
>  end:
>         if (state->newfd >= 0) {
> @@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state,
>                 state->newfd = -1;
>         }
>
> +       if (state->apply_verbosity <= verbosity_silent) {
> +               set_error_routine(state->saved_error_routine);
> +               set_warn_routine(state->saved_warn_routine);
> +       }
> +
> +       if (res > -1)
> +               return res;
>         return (res == -1 ? 1 : 128);

So anything > -1 is returned as is, and == -1 returns 1 and <-1  returns 128 ?

So I guess a reminder/explanation on why we need to fiddle with return codes in the commit message would help. (I do not understand how the verbosity influences return codes.)