Re: Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only

From: Glen Choo <chooglen@google.com>
Date: Jan 20, 2022, 23:15 UTC
Message-ID: <kl6lee52xalp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqk0eugjcc.fsf_-_@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> Glen Choo <chooglen@google.com> writes:
>
>> Changes since v6:
>> * use standard message format introduced in 246cac8505 (i18n: turn even
>>   more messages into "cannot be used together" ones, 2022-01-05) (thanks
>>   Jiang Xin!)
>
> As v6 is already in 'next' since yesterday, let's make it an
> incremental update.  It would give us a place to spell out why we
> consider this change is desirable.

Ah, yes. I will take note of this for the future.

Show 19 quoted lines

>
> This is a tangent, but I recall there was a talk about "reviewer
> checklist".  Things like:
>
>  - check if we can reuse existing and identical message to reduce
>    load on translators
>
>  - when we are creating a file in a subdirectory of $GIT_DIR, be
>    prepared to see any directories other than $GIT_DIR itself
>    missing and create them as necessary
>
>  - use safe_create_leading_directories() and adjust_shared_perm() on
>    things under $GIT_DIR but not in the working tree
>
>
> may belong there.
>
> I am not sure if it is feasible to create and maintain such a list,
> though.

This sounds like a combination of low-hanging fruit things to check when submitting/reviewing. I think that even a minimal list is preferable to the toil of spotting and fixing the same mistakes over and over.

A ReviewingPatches doc has been discussed internally for a while, but I don't recall if this checklist was part of it.

Show 28 quoted lines

> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] fetch: help translators by reusing the same message template
>
> Follow the example set by 12909b6b (i18n: turn "options are
> incompatible" into "cannot be used together", 2022-01-05) and use
> the same message string to reduce the need for translation.
>
> Reported-by: Jiang Xin <worldhello.net@gmail.com>
> Helped-by: Glen Choo <chooglen@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/fetch.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git c/builtin/fetch.c w/builtin/fetch.c
> index dc6e637428..5c329f9835 100644
> --- c/builtin/fetch.c
> +++ w/builtin/fetch.c
> @@ -2014,7 +2014,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			break;
>  
>  		default:
> -			die(_("--negotiate-only and --recurse-submodules cannot be used together"));
> +			die(_("options '%s' and '%s' cannot be used together"),
> +			    "--negotiate-only", "--recurse-submodules");
>  		}
>  	}
>

The diff looks good. Thanks!