Re: [PATCH v2 03/10] promisor-remote: clarify that a remote is ignored

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> In should_accept_remote() and parse_one_advertised_remote(), when a
> remote is ignored, we tell users why it is ignored in a warning, but we
> don't tell them that the remote is actually ignored.
>
> Let's clarify that, so users have a better idea of what's actually
> happening.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  promisor-remote.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

I agree that it makes sense to add the final disposition to the
message.  It would be even better to rephrase so that it, the most
important part of the message, comes first.  E.g.,

>
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 6c935f855a..8e062ec160 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -670,15 +670,16 @@ static int should_accept_remote(enum accept_promisor accept,
>  		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
>  
>  	if (!remote_url || !*remote_url) {
> -		warning(_("no or empty URL advertised for remote '%s'"), remote_name);
> +		warning(_("no or empty URL advertised for remote '%s', "
> +			  "ignoring this remote"), remote_name);

I would find it easier to understand if it is phrased this way.

		warning(_("ignoring remote '%s' that advertises no usable URL"),
			remote_name);

i.e., conclusion first, the reason for the conclusion next.

Thanks.