Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings
- From
Đoàn Trần Công Danh <congdanhqx@gmail.com>
- Date
- Jul 7, 2021, 16:57 UTC
- Message-ID
- <YOXc+mT01vptY9Nx@danh.dev>
- In-Reply-To
- <20210707162308.2438170-2-hujialun@comp.nus.edu.sg>
Hi Jialun,
On 2021-07-08 00:23:07+0800, Hu Jialun <hujialun@comp.nus.edu.sg> wrote:
Show 29 quoted lines
> 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@comp.nus.edu.sg>
> ---
> 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.
Show 18 quoted lines
> + 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.
Show 6 quoted lines
> > /* > * These should never fail because they come from our own > -- > 2.32.0 >
-- Danh