Re: [PATCH] push: anonymize URLs in error messages and warnings
Hi Junio,
On Fri, 24 Apr 2020, Junio C Hamano wrote:
Show 27 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > builtin/push.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Is this something we can protect with a test? perhaps like 882d49ca
> (push: anonymize URL in status output, 2016-07-13) did?
>
> > diff --git a/builtin/push.c b/builtin/push.c
> > index 6dbf0f0bb71..bd2a2cbfbd7 100644
> > --- a/builtin/push.c
> > +++ b/builtin/push.c
> > @@ -340,6 +340,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
> > {
> > int err;
> > unsigned int reject_reasons;
> > + char *anon_url = transport_anonymize_url(transport->url);
>
> Do we need to anonymize this early, way before we know we'd fail the
> push? It wouldn't be that transport->url is going to be munged
> before we realize that we have to issue an error message---otherwise
> you'd not be writing a patch to replace transport->url in the error
> message. So this placement of anonymize call sounds like taking the
> error path as the main flow of control.
>
> In other words, would it break to squash the following change in?It would break it _if I hadn't forgotten to include this_:
diff --git a/builtin/push.c b/builtin/push.c
index bd2a2cbfbd73..59c8acb55680 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -358,7 +358,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
}
if (verbosity > 0)
- fprintf(stderr, _("Pushing to %s\n"), transport->url);
+ fprintf(stderr, _("Pushing to %s\n"), anon_url);
trace2_region_enter("push", "transport_push", the_repository);
err = transport_push(the_repository, transport,
rs, flags, &reject_reasons);
And it's not like we change the URL in `push_with_options()`: it is
expected to remain unmodified throughout this function.
Do you want me to send a v2 with above diff squashed in, or can you apply
locally (unless more reviews trickle in that require changes)?
Ciao,
Dscho
>
> builtin/push.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index bd2a2cbfbd..b88948b07e 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -340,7 +340,6 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
> {
> int err;
> unsigned int reject_reasons;
> - char *anon_url = transport_anonymize_url(transport->url);
>
> transport_set_verbosity(transport, verbosity, progress);
> transport->family = family;
> @@ -364,13 +363,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
> rs, flags, &reject_reasons);
> trace2_region_leave("push", "transport_push", the_repository);
> if (err != 0) {
> + char *anon_url = transport_anonymize_url(transport->url);
> fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
> error(_("failed to push some refs to '%s'"), anon_url);
> fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));
> + free(anon_url);
> }
>
> err |= transport_disconnect(transport);
> - free(anon_url);
> if (!err)
> return 0;
>
>
>
Johannes Schindelin <johannes.schindelin@gmx.de>