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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.

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

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

> @@ -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! :)

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

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

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