Re: [PATCH 14/21] i18n: rebase-interactive: mark strings for translation

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 21, 2016, 12:57 UTC
Message-ID: <CACBZZX7T7b9xMM_mDcH6JBnuCap7RtKvSYoFNDrN5mLdmLLYHQ@mail.gmail.com>
In-Reply-To: <1463585274-9027-15-git-send-email-vascomalmeida@sapo.pt>

On Wed, May 18, 2016 at 5:27 PM, Vasco Almeida <vascomalmeida@sapo.pt> wrote:

> Mark strings in git-rebase--interactive.sh for translation. There is no
> need to source git-sh-i18n since git-rebase.sh already does so.

Cool, thanks for working on this.

Show 11 quoted lines

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -128,7 +128,7 @@ mark_action_done () {
>         if test "$last_count" != "$new_count"
>         then
>                 last_count=$new_count
> -               printf "Rebasing (%d/%d)\r" $new_count $total
> +               printf "$(gettext Rebasing) (%d/%d)\r" $new_count $total
>                 test -z "$verbose" || echo
>         fi
>  }

Things like these should be converted into something you can pass to eval_gettext. I.e. For any message the translator needs to be able to translate the whole message. Consider e.g. RTL languages where the (%d/%d) will be on the right-hand-side of the message.

Show 16 quoted lines

> @@ -201,11 +201,13 @@ exit_with_patch () {
>         make_patch $1
>         git rev-parse --verify HEAD > "$amend"
>         gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")}
> -       warn "You can amend the commit now, with"
> +       # TRANSLATORS: after this line is a command to be issued by the user
> +       warn "$(gettext "You can amend the commit now, with")"
>         warn
>         warn "  git commit --amend $gpg_sign_opt_quoted"
>         warn
> -       warn "Once you are satisfied with your changes, run"
> +       # TRANSLATORS: after this line is a command to be issued by the user
> +       warn "$(gettext "Once you are satisfied with your changes, run")"
>         warn
>         warn "  git rebase --continue"
>         warn

Stuff like this should be one big call to gettext. I.e. everything from "You can amend" up to and including "--continue". Even if the translators probably don't want to change the order here it helps a lot to have the extra context. I.e. do it like ...

Show 15 quoted lines

> @@ -536,10 +543,11 @@ do_next () {
>                 mark_action_done
>                 do_pick $sha1 "$rest"
>                 git commit --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> -                       warn "Could not amend commit after successfully picking $sha1... $rest"
> -                       warn "This is most likely due to an empty commit message, or the pre-commit hook"
> -                       warn "failed. If the pre-commit hook failed, you may need to resolve the issue before"
> -                       warn "you are able to reword the commit."
> +                       warn "$(eval_gettext "\
> +Could not amend commit after successfully picking \$sha1... \$rest
> +This is most likely due to an empty commit message, or the pre-commit hook
> +failed. If the pre-commit hook failed, you may need to resolve the issue before
> +you are able to reword the commit.")"
>                         exit_with_patch $sha1 1
>                 }

... this! :)

Show 9 quoted lines

> @@ -607,7 +615,7 @@ do_next () {
>         x|"exec")
>                 read -r command rest < "$todo"
>                 mark_action_done
> -               printf 'Executing: %s\n' "$rest"
> +               printf "$(gettext Executing:) %s\n" "$rest"
>                 "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>                 status=$?
>                 # Run in subshell because require_clean_work_tree can die.

Ditto my first comment about the whole thing needing to be a call to eval_gettext. I.e. also that " %s" part.

Show 21 quoted lines

> -                       warn "You can fix the problem, and then run"
> +                       # TRANSLATORS: after this line is a command to be issued by the user
> +                       warn "$(gettext "You can fix the problem, and then run")"
>                         warn
>                         warn "  git rebase --continue"
>                         warn
> @@ -630,9 +639,11 @@ do_next () {
>                         exit "$status"
>                 elif test "$dirty" = t
>                 then
> -                       warn "Execution succeeded: $rest"
> -                       warn "but left changes to the index and/or the working tree"
> -                       warn "Commit or stash your changes, and then run"
> +                       # TRANSLATORS: after these lines is a command to be issued by the user
> +                       warn "$(eval_gettext "\
> +Execution succeeded: \$rest
> +but left changes to the index and/or the working tree
> +Commit or stash your changes, and then run")"
>                         warn
>                         warn "  git rebase --continue"
>                         warn

Both ditto the above. I.e. just include the command to be issued in the message. Then you can also skip the TRANSLATORS comment since this won't be confusing to them anymore.

Show 32 quoted lines

> @@ -991,28 +1002,26 @@ check_todo_list () {
>                 then
>                         test "$check_level" = error && raise_error=t
>
> -                       warn "Warning: some commits may have been dropped" \
> -                               "accidentally."
> -                       warn "Dropped commits (newer to older):"
> +                       warn "$(gettext "\
> +Warning: some commits may have been dropped accidentally.
> +Dropped commits (newer to older):")"
>
>                         # Make the list user-friendly and display
>                         opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
>                         git rev-list $opt <"$todo".miss | warn_lines
>
> -                       warn "To avoid this message, use \"drop\" to" \
> -                               "explicitly remove a commit."
> -                       warn
> -                       warn "Use 'git config rebase.missingCommitsCheck' to change" \
> -                               "the level of warnings."
> -                       warn "The possible behaviours are: ignore, warn, error."
> +                       warn "$(gettext "\
> +To avoid this message, use \"drop\" to explicitly remove a commit.
> +
> +Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
> +The possible behaviours are: ignore, warn, error.")"
>                         warn
>                 fi
>                 ;;
>         ignore)
>                 ;;
>         *)

Making this into one big eval_gettext where we stash away those "newer to older" commits into a variable would be easier on translators, but maybe there are performance considerations and we could just live with two messages. Not sure how large this list might get in practice.

I haven't tried to apply & run this patch. But aside from the chunks I commented on the rest of this looks fine to me.