diff mbox series

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

Message ID xmqqk0eugjcc.fsf_-_@gitster.g (mailing list archive)
State New, archived
Headers show
Series Re* [PATCH v7 0/3] fetch: skip unnecessary tasks when using --negotiate-only | expand

Commit Message

Junio C Hamano Jan. 20, 2022, 9:58 p.m. UTC
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.

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.

----- >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(-)

Comments

Glen Choo Jan. 20, 2022, 11:15 p.m. UTC | #1
Junio C Hamano <gitster@pobox.com> writes:

> 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.

>
> 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.

> ----- >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!
Jiang Xin Jan. 21, 2022, 2:17 a.m. UTC | #2
On Fri, Jan 21, 2022 at 5:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
>
> 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

How to spot typos in l10n messages in time, instead of waiting until
the very end of the release cycle, has always puzzled me for a long
time. Until two months ago, by introducing a new GitHub Actions[1] in
the "git-l10n/git-po" repository[2], it was possible to generate
temporary "po/git.pot" file based on new changes in "git.git" in time,
and create incremental diffs of "po/git.pot" as separate files in the
"pot/master", "pot/next", and "pot/seen" branches in the "git-po"
repository. By checking the diffs daily, several i18n issues were
found in recent months.

We can make some improvements to the workflow:

1. Send each incremental diff of "po/git.pot" to active l10n team
leaders, and also to this mailing list. Git contributors can find
"side effects" introduced by their patches.

2. Just like using the "git-po-helper" program[3] to check "msgstr" in
l10n translations, develop a new helper to check "msgid". E.g.: check
if we can reuse existing and identical message to reduce load on
translators.

This workflow only works when patches go into the branch "seen" of
git.git repository. Lagging behind code reviews.

[1]: https://github.com/git-l10n/git-po/blob/pot/CI/.github/workflows/git-pot.yml
[2]: https://github.com/git-l10n/git-po
[3]: https://github.com/git-l10n/git-po-helper

>  - 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.
>
> ----- >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");
>                 }
>         }
>
diff mbox series

Patch

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");
 		}
 	}