Hi Jialun,
On 2021-07-08 00:23:07+0800, Hu Jialun <hujialun@xxxxxxxxxxxxxxx> wrote:
> While the prefilled commit prompt is mostly the same for different
> cleanup modes, those are separately repeated, which violates the DRY
> principle and hinders maintainability.
>
> Unify and reorder identical substrings to improve.
>
> Signed-off-by: Hu Jialun <hujialun@xxxxxxxxxxxxxxx>
> ---
> builtin/commit.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43..815b408002 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -910,21 +910,24 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> }
>
> fprintf(s->fp, "\n");
> - if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)
> - status_printf(s, GIT_COLOR_NORMAL,
> - _("Please enter the commit message for your changes."
> - " Lines starting\nwith '%c' will be ignored, and an empty"
> - " message aborts the commit.\n"), comment_line_char);
> + const char *msg_enter_prompt = _("Please enter the commit message for your changes.");
> + const char *keep_char_prompt = _("Lines starting with '%c' will be kept;"
> + " you may remove them yourself if you want to.");
> + const char *ignore_char_prompt = _("Lines starting with '%c' will be ignored.");
> + const char *empty_msg_abort_prompt = _("An empty message aborts the commit.");
In Git project, it's enforced to have -Wdeclaration-after-statement,
IOW, move all declaration before statement.
> + if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
> + status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
builtin/commit.c:919:4: error: format not a string literal and no format arguments [-Werror=format-security]
919 | status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
msg_enter_prompt will come from translator and may have '%' inside it.
We can solve it by inserting "%s" there.
However, I think we shouldn't take this route, because splitting likes this
will make a translation lego. I can't speak for Junio, but from my
observation, it's preferred to have 3 variables for 3 full-text, and
we will pick the suitable text in each if-leg.
> + status_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);
> + status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> + }
> else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
> if (whence == FROM_COMMIT && !merge_contains_scissors)
> wt_status_add_cut_line(s->fp);
> - } else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> - status_printf(s, GIT_COLOR_NORMAL,
> - _("Please enter the commit message for your changes."
> - " Lines starting\n"
> - "with '%c' will be kept; you may remove them"
> - " yourself if you want to.\n"
> - "An empty message aborts the commit.\n"), comment_line_char);
> + } else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> + status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
> + status_printf_ln(s, GIT_COLOR_NORMAL, keep_char_prompt, comment_line_char);
> + status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> + }
After changing those texts, the tests should be updated, too.
It's a customary service for the next developer, who needs to bisect
this project to have all test-cases pass on each changes.
With this change, t7500.50 and t7502.37 runs into failures.
Please fix them here, instead of next change.
>
> /*
> * These should never fail because they come from our own
> --
> 2.32.0
>
--
Danh
