Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings
From: Đoàn Trần Công Danh <hidden>
Date: 2021-07-07 16:57:36
Hi Jialun, On 2021-07-08 00:23:07+0800, Hu Jialun [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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