* [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings
2021-07-07 16:23 ` Hu Jialun
@ 2021-07-07 16:23 ` Hu Jialun
2021-07-07 16:57 ` Đoàn Trần Công Danh
2021-07-07 16:23 ` [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-07 16:23 UTC (permalink / raw)
To: git; +Cc: gitster, Hu Jialun
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.");
+ if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
+ status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
+ 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);
+ }
/*
* These should never fail because they come from our own
--
2.32.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings
2021-07-07 16:23 ` [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings Hu Jialun
@ 2021-07-07 16:57 ` Đoàn Trần Công Danh
2021-07-07 17:44 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Đoàn Trần Công Danh @ 2021-07-07 16:57 UTC (permalink / raw)
To: Hu Jialun; +Cc: git, gitster
Hi Jialun,
On 2021-07-08 00:23:07+0800, Hu Jialun <hujialun@comp.nus.edu.sg> 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@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.
> + 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings
2021-07-07 16:57 ` Đoàn Trần Công Danh
@ 2021-07-07 17:44 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2021-07-07 17:44 UTC (permalink / raw)
To: Đoàn Trần Công Danh; +Cc: Hu Jialun, git
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
> 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.
Yes, that is what I meant in my earlier suggestion. More like
char *hint_cleanup_all =
_("Please enter the ... , and an empty message aborts the commit.\n");
char *hint_cleanup_space =
_("Please enter the ... if you want to.\n"
"An empty message aborts the commit.\n");
if (allow_empty_message) {
hint_cleanup_all = _("...");
hint_cleanup_space = _("...");
}
... the if/elseif cascade in which calls to status_printf() are made
... using these variables
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-07 16:23 ` Hu Jialun
2021-07-07 16:23 ` [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings Hu Jialun
@ 2021-07-07 16:23 ` Hu Jialun
2021-07-07 17:42 ` Felipe Contreras
2021-07-08 15:19 ` [PATCH] " Hu Jialun
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-07 16:23 UTC (permalink / raw)
To: git; +Cc: gitster, Hu Jialun
Even when the `--allow-empty-message` option is given, "git
commit" offers an interactive editor session with prefilled
message that says the commit will be aborted if the buffer is
emptied, which is wrong.
Remove the "an empty message aborts" part from the message when
the option is given to fix it.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
---
The tests are also amended in line with the new string.
builtin/commit.c | 8 ++++++--
t/t7500-commit-template-squash-signoff.sh | 4 ++--
t/t7502-commit-porcelain.sh | 4 ++--
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 815b408002..b681dcc83c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -918,7 +918,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
status_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);
- status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
+ if (!allow_empty_message) {
+ 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)
@@ -926,7 +928,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
} 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);
+ if (!allow_empty_message) {
+ status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
+ }
}
/*
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 7d02f79c0d..812ca45043 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -497,8 +497,8 @@ test_expect_success 'invalid message options when using --fixup' '
cat >expected-template <<EOF
-# Please enter the commit message for your changes. Lines starting
-# with '#' will be ignored, and an empty message aborts the commit.
+# Please enter the commit message for your changes.
+# Lines starting with '#' will be ignored.
#
# Author: A U Thor <author@example.com>
#
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 38a532d81c..a5217872ca 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -608,8 +608,8 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '
echo "sample
-# Please enter the commit message for your changes. Lines starting
-# with '#' will be ignored, and an empty message aborts the commit." >expect
+# Please enter the commit message for your changes.
+# Lines starting with '#' will be ignored." >expect
test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '
test_cmp expect actual
--
2.32.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* RE: [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-07 16:23 ` [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
@ 2021-07-07 17:42 ` Felipe Contreras
0 siblings, 0 replies; 20+ messages in thread
From: Felipe Contreras @ 2021-07-07 17:42 UTC (permalink / raw)
To: Hu Jialun, git; +Cc: gitster, Hu Jialun
Hu Jialun wrote:
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -918,7 +918,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {
> status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);
> status_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);
> - status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> + if (!allow_empty_message) {
> + status_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);
> + }
In git the style is to avoid braces if the content of the condition is a
single line.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-07 16:23 ` Hu Jialun
2021-07-07 16:23 ` [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings Hu Jialun
2021-07-07 16:23 ` [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
@ 2021-07-08 15:19 ` Hu Jialun
2021-07-08 16:05 ` Đoàn Trần Công Danh
2021-07-09 18:07 ` [PATCH v3 1/2] commit: reorganise commit hint strings Hu Jialun
2021-07-09 18:07 ` [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
4 siblings, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-08 15:19 UTC (permalink / raw)
To: git; +Cc: gitster, congdanhqx, felipe.contreras
Junio C Hamano wrote:
> char *hint_cleanup_all =
> _("Please enter the ... , and an empty message aborts the commit.\n");
> char *hint_cleanup_space =
> _("Please enter the ... if you want to.\n"
> "An empty message aborts the commit.\n");
>
> if (allow_empty_message) {
> hint_cleanup_all = _("...");
> hint_cleanup_space = _("...");
> }
>
> ... the if/elseif cascade in which calls to status_printf() are made
> ... using these variables
Would it be better this way or just using the ternary operator in-line
instead? If the latter, should it still be separated into another
variable or just embedded in the status_printf call? Using the ternary
operator does require to separate checks of allow_empty_message, but
might as well save us an `if` construct to reassign the variable.
In other words, which of the following 3 is the most acceptable?
1. As Junio suggested, quoted above.
2.
status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?
_("...") :
_("...."), comment_line_char);
3.
const char *hint_foo = allow_empty_message ?
_("...") :
_("....");
......
status_printf(s, GIT_COLOR_NORMAL, hint_foo, comment_line_char);
--------------------------------------------------------------------
Felipe Contreras wrote:
> In git the style is to avoid braces if the content of the condition is a
> single line.
Đoàn Trần Công Danh wrote:
> In Git project, it's enforced to have -Wdeclaration-after-statement,
> IOW, move all declaration before statement.
Noted with thanks!
> 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.
I did change test cases accordingly in the second patch (excerpt below), and
both tests did pass afterwards. Was there something wrong with it?
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 7d02f79c0d..812ca45043 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -497,8 +497,8 @@ test_expect_success 'invalid message options when using --fixup' '
>
> cat >expected-template <<EOF
>
> -# Please enter the commit message for your changes. Lines starting
> -# with '#' will be ignored, and an empty message aborts the commit.
> +# Please enter the commit message for your changes.
> +# Lines starting with '#' will be ignored.
> #
> # Author: A U Thor <author@example.com>
> #
> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index 38a532d81c..a5217872ca 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -608,8 +608,8 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '
>
> echo "sample
>
> -# Please enter the commit message for your changes. Lines starting
> -# with '#' will be ignored, and an empty message aborts the commit." >expect
> +# Please enter the commit message for your changes.
> +# Lines starting with '#' will be ignored." >expect
>
> test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '
> test_cmp expect actual
--------------------------------------------------------------------
And some perhaps rather noob questions below, as an (overly) curious
newcomer,
- Why is the "lego" style breakdown of translation strings unrecommended?
I suppose it might be in consideration of possibly different linguistic
sequences across languages but I'm not so sure.
- What is the rationale behind prohibiting braces around single line
constructs? It seems somewhat error-prone since somebody else could
later be adding statements into the body without putting the curly
braces.
- When replying to multiple comments in multiple emails (like in this very
email), would it be better to send multiple emails as replies to individual
comments or do it in one email? If the latter, which previous message should
the single reply be In-Reply-To?
Thanks in advance,
Hu Jialun
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-08 15:19 ` [PATCH] " Hu Jialun
@ 2021-07-08 16:05 ` Đoàn Trần Công Danh
2021-07-08 18:26 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Đoàn Trần Công Danh @ 2021-07-08 16:05 UTC (permalink / raw)
To: Hu Jialun; +Cc: git, gitster, felipe.contreras
On 2021-07-08 23:19:11+0800, Hu Jialun <hujialun@comp.nus.edu.sg> wrote:
> Junio C Hamano wrote:
> > char *hint_cleanup_all =
> > _("Please enter the ... , and an empty message aborts the commit.\n");
> > char *hint_cleanup_space =
> > _("Please enter the ... if you want to.\n"
> > "An empty message aborts the commit.\n");
> >
> > if (allow_empty_message) {
> > hint_cleanup_all = _("...");
> > hint_cleanup_space = _("...");
> > }
> >
> > ... the if/elseif cascade in which calls to status_printf() are made
> > ... using these variables
>
> Would it be better this way or just using the ternary operator in-line
> instead? If the latter, should it still be separated into another
> variable or just embedded in the status_printf call? Using the ternary
> operator does require to separate checks of allow_empty_message, but
> might as well save us an `if` construct to reassign the variable.
>
> In other words, which of the following 3 is the most acceptable?
>
> 1. As Junio suggested, quoted above.
I think this approach is the most expensive one, _() needs to query
the gettext infrastructure, which is usually costly.
However, I think that cost doesn't matter much since we're about to
open an editor soon.
> 2.
> status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?
> _("...") :
> _("...."), comment_line_char);
install_branch_config() uses this style.
>
> 3.
> const char *hint_foo = allow_empty_message ?
> _("...") :
> _("....");
builtin/remote.c:show_local_info_item() writes:
const char *msg;
if (condition)
msg = _("some message");
else
msg = _("other message");
So, I guess it's fine either way. And people will need to see the
patch to see which one is better.
> ......
> status_printf(s, GIT_COLOR_NORMAL, hint_foo, comment_line_char);
>
> --------------------------------------------------------------------
>
> Felipe Contreras wrote:
> > In git the style is to avoid braces if the content of the condition is a
> > single line.
>
> Đoàn Trần Công Danh wrote:
> > In Git project, it's enforced to have -Wdeclaration-after-statement,
> > IOW, move all declaration before statement.
>
> Noted with thanks!
>
> > 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.
>
> I did change test cases accordingly in the second patch (excerpt below), and
> both tests did pass afterwards. Was there something wrong with it?
Yes, when apply both 2 patches, the test passed, however, the test
doesn't pass with only 1/2 applied. Let's imagine in a near future,
some developers need to bisect some problems with Git with automation
scripts, and git-bisect stops at 1/2, since the tests report failure,
"git bisect run" will mark this change as "bad commit", thus render
git-bisect hard to use. We should make sure all tests pass on all
commit.
> And some perhaps rather noob questions below, as an (overly) curious
> newcomer,
>
> - Why is the "lego" style breakdown of translation strings unrecommended?
> I suppose it might be in consideration of possibly different linguistic
> sequences across languages but I'm not so sure.
Let's imagine an artificial language which have 2 words "linos" and
"linas" which is both translated to English as "lines", translators
need full context to decide which word should be chosen. Things maybe
complicated with language with gender, word-cases, etc...
There're some problems reported on and off this list [1]
> - What is the rationale behind prohibiting braces around single line
> constructs? It seems somewhat error-prone since somebody else could
> later be adding statements into the body without putting the curly
> braces.
Documentation/CodingGuidelines said so ;)
I don't think somebody adding random statements is a valid concern for
brace, I think it's expected to analyse the code context before doing
real-work on project. Furthermore, -Wmisleading-indentation is your
friends.
1: https://lore.kernel.org/git/20210509215250.33215-1-alexhenrie24@gmail.com/
--
Danh
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-08 16:05 ` Đoàn Trần Công Danh
@ 2021-07-08 18:26 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2021-07-08 18:26 UTC (permalink / raw)
To: Đoàn Trần Công Danh; +Cc: Hu Jialun, git, felipe.contreras
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
>> In other words, which of the following 3 is the most acceptable?
>>
>> 1. As Junio suggested, quoted above.
>
> I think this approach is the most expensive one, _() needs to query
> the gettext infrastructure, which is usually costly.
> However, I think that cost doesn't matter much since we're about to
> open an editor soon.
See note below.
>> 2.
>> status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?
>> _("...") :
>> _("...."), comment_line_char);
>
> install_branch_config() uses this style.
>
>>
>> 3.
>> const char *hint_foo = allow_empty_message ?
>> _("...") :
>> _("....");
>
> builtin/remote.c:show_local_info_item() writes:
>
> const char *msg;
> if (condition)
> msg = _("some message");
> else
> msg = _("other message");
>
> So, I guess it's fine either way. And people will need to see the
> patch to see which one is better.
Yeah, #1 and #3 are better than the patch posted or #2 in that by
extracting the large message body out-of-line from the code that do
use the messages, they make it simpler to follow the logic that uses
these messages. That is
if (cleanup_mode == CLEANUP_ALL)
status_printf(..., hint_cleanup_all);
else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
...;
else /* all the rest */
status_printf(..., hint_cleanup_space, comment_line_char);
would be far easier to follow than
if (cleanup_mode == CLEANUP_ALL)
status_printf(...,
condition
? large-large-message-1
: large-large-message-1-plus-note-about-empty-message);
else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
...;
else /* all the rest */
status_printf(...,
condition
? large-large-message-2
: large-large-message-2-plus-note-about-empty-message,
comment_line_char);
as the overall structure is easier to follow without the minute
detail of using slightly different messages depending on the
allow-empty setting.
By the way, if you want to avoid calling _() twice with the approach
#1, you can do
hint_cleanup_all = N_("<cleanup and note about empty message>");
...
if (condition) {
hint_cleanup_all = N_("<cleanup without note>");
...
}
and use _(hint_cleanup_all) at the site that uses the message.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] commit: reorganise commit hint strings
2021-07-07 16:23 ` Hu Jialun
` (2 preceding siblings ...)
2021-07-08 15:19 ` [PATCH] " Hu Jialun
@ 2021-07-09 18:07 ` Hu Jialun
2021-07-09 19:14 ` Junio C Hamano
2021-07-09 18:07 ` [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
4 siblings, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-09 18:07 UTC (permalink / raw)
To: git
Cc: Hu Jialun, Junio C Hamano,
Đoàn Trần Công Danh, Felipe Contreras
Strings of hint messages inserted into editor on interactive commit was
scattered in-line, rendering the code harder to understand at first
glance.
Extract those messages out into separate variables to make the code
outline easier to follow.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
---
builtin/commit.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43..e68d139dee 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -889,6 +889,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
int ident_shown = 0;
int saved_color_setting;
struct ident_split ci, ai;
+ const char *hint_cleanup_all = _("Please enter the commit message for your changes."
+ " Lines starting\nwith '%c' will be ignored,"
+ " and an empty message aborts the commit.\n");
+ const char *hint_cleanup_space = _("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");
if (whence != FROM_COMMIT) {
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
@@ -911,20 +919,12 @@ 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);
+ status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char);
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);
+ status_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char);
/*
* These should never fail because they come from our own
--
2.32.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] commit: reorganise commit hint strings
2021-07-09 18:07 ` [PATCH v3 1/2] commit: reorganise commit hint strings Hu Jialun
@ 2021-07-09 19:14 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2021-07-09 19:14 UTC (permalink / raw)
To: Hu Jialun; +Cc: git, Đoàn Trần Công Danh, Felipe Contreras
Hu Jialun <hujialun@comp.nus.edu.sg> writes:
> Strings of hint messages inserted into editor on interactive commit was
> scattered in-line, rendering the code harder to understand at first
> glance.
>
> Extract those messages out into separate variables to make the code
> outline easier to follow.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
> ---
> builtin/commit.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 190d215d43..e68d139dee 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -889,6 +889,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> int ident_shown = 0;
> int saved_color_setting;
> struct ident_split ci, ai;
> + const char *hint_cleanup_all = _("Please enter the commit message for your changes."
> + " Lines starting\nwith '%c' will be ignored,"
> + " and an empty message aborts the commit.\n");
> + const char *hint_cleanup_space = _("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");
That would easily make lines that are overly long. Perhaps fold
them like so?
const char *hint_cleanup_all =
_("Please enter the commit message for your changes."
" Lines starting\nwith '%c' will be ignored,"
" and an empty message aborts the commit.\n");
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-07 16:23 ` Hu Jialun
` (3 preceding siblings ...)
2021-07-09 18:07 ` [PATCH v3 1/2] commit: reorganise commit hint strings Hu Jialun
@ 2021-07-09 18:07 ` Hu Jialun
2021-07-09 19:14 ` Junio C Hamano
4 siblings, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-09 18:07 UTC (permalink / raw)
To: git
Cc: Hu Jialun, Junio C Hamano,
Đoàn Trần Công Danh, Felipe Contreras
Even when the `--allow-empty-message` option is given, "git commit"
offers an interactive editor session with prefilled message that says
the commit will be aborted if the buffer is emptied, which is wrong.
Remove the "an empty message aborts" part from the message when the
option is given to fix it.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Helped-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg>
---
builtin/commit.c | 25 +++++++++++++++--------
t/t7500-commit-template-squash-signoff.sh | 2 +-
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e68d139dee..cfbc83751a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -889,15 +889,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
int ident_shown = 0;
int saved_color_setting;
struct ident_split ci, ai;
- const char *hint_cleanup_all = _("Please enter the commit message for your changes."
- " Lines starting\nwith '%c' will be ignored,"
- " and an empty message aborts the commit.\n");
- const char *hint_cleanup_space = _("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");
-
+ const char *hint_cleanup_all = allow_empty_message ?
+ _("Please enter the commit message for your changes."
+ " Lines starting\nwith '%c' will be ignored.\n") :
+ _("Please enter the commit message for your changes."
+ " Lines starting\nwith '%c' will be ignored, and an empty"
+ " message aborts the commit.\n");
+ const char *hint_cleanup_space = allow_empty_message ?
+ _("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") :
+ _("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");
if (whence != FROM_COMMIT) {
if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
!merge_contains_scissors)
diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
index 7d02f79c0d..54c2082acb 100755
--- a/t/t7500-commit-template-squash-signoff.sh
+++ b/t/t7500-commit-template-squash-signoff.sh
@@ -498,7 +498,7 @@ test_expect_success 'invalid message options when using --fixup' '
cat >expected-template <<EOF
# Please enter the commit message for your changes. Lines starting
-# with '#' will be ignored, and an empty message aborts the commit.
+# with '#' will be ignored.
#
# Author: A U Thor <author@example.com>
#
--
2.32.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-09 18:07 ` [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message` Hu Jialun
@ 2021-07-09 19:14 ` Junio C Hamano
2021-07-10 17:26 ` Hu Jialun
2021-07-22 18:33 ` Hu Jialun
0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2021-07-09 19:14 UTC (permalink / raw)
To: Hu Jialun; +Cc: git, Đoàn Trần Công Danh, Felipe Contreras
Hu Jialun <hujialun@comp.nus.edu.sg> writes:
> Even when the `--allow-empty-message` option is given, "git commit"
> offers an interactive editor session with prefilled message that says
> the commit will be aborted if the buffer is emptied, which is wrong.
>
> Remove the "an empty message aborts" part from the message when the
> option is given to fix it.
The updated log message is much better than the old iteration, where
the "empty message aborts" was called "irrelevant" when it is actively
"wrong". Now we call it "wrong".
Let's update the title of the commit to match, e.g.
commit: tweak cleanup hint when --allow-empty-message is in effect
or something along that line.
> + const char *hint_cleanup_all = allow_empty_message ?
> + _("Please enter the commit message for your changes."
> + " Lines starting\nwith '%c' will be ignored.\n") :
> + _("Please enter the commit message for your changes."
> + " Lines starting\nwith '%c' will be ignored, and an empty"
> + " message aborts the commit.\n");
> + const char *hint_cleanup_space = allow_empty_message ?
> + _("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") :
> + _("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");
Local convention in this file seems to be that multi-line ?:
expressions are folded with ? and : operators at the beginning, not
at the end, of the lines, e.g. I see this construct in the same file.
reflog_msg = is_from_cherry_pick(whence)
? "commit (cherry-pick)"
: is_from_rebase(whence)
? "commit (rebase)"
: "commit";
So, let's mimick and make the above more like this:
const char *hint_cleanup_all = allow_empty_message
? _("Please enter the commit message for your changes."
" Lines starting\nwith '%c' will be ignored.\n")
: _("Please enter the commit message for your changes."
" Lines starting\nwith '%c' will be ignored, and an empty"
" message aborts the commit.\n");
> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh
> index 7d02f79c0d..54c2082acb 100755
> --- a/t/t7500-commit-template-squash-signoff.sh
> +++ b/t/t7500-commit-template-squash-signoff.sh
> @@ -498,7 +498,7 @@ test_expect_success 'invalid message options when using --fixup' '
> cat >expected-template <<EOF
>
> # Please enter the commit message for your changes. Lines starting
> -# with '#' will be ignored, and an empty message aborts the commit.
> +# with '#' will be ignored.
> #
> # Author: A U Thor <author@example.com>
> #
Perfect.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-09 19:14 ` Junio C Hamano
@ 2021-07-10 17:26 ` Hu Jialun
2021-07-22 18:33 ` Hu Jialun
1 sibling, 0 replies; 20+ messages in thread
From: Hu Jialun @ 2021-07-10 17:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano wrote:
> > + const char *hint_cleanup_all = allow_empty_message ?
> > + _("Please enter the commit message for your changes."
> > + " Lines starting\nwith '%c' will be ignored.\n") :
> > + _("Please enter the commit message for your changes."
> > + " Lines starting\nwith '%c' will be ignored, and an empty"
> > + " message aborts the commit.\n");
> > + const char *hint_cleanup_space = allow_empty_message ?
> > + _("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") :
> > + _("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");
>
> Local convention in this file seems to be that multi-line ?:
> expressions are folded with ? and : operators at the beginning, not
> at the end, of the lines, e.g. I see this construct in the same file.
>
> reflog_msg = is_from_cherry_pick(whence)
> ? "commit (cherry-pick)"
> : is_from_rebase(whence)
> ? "commit (rebase)"
> : "commit";
>
> So, let's mimick and make the above more like this:
>
> const char *hint_cleanup_all = allow_empty_message
> ? _("Please enter the commit message for your changes."
> " Lines starting\nwith '%c' will be ignored.\n")
> : _("Please enter the commit message for your changes."
> " Lines starting\nwith '%c' will be ignored, and an empty"
> " message aborts the commit.\n");
Another statement several lines below, whose style I conformed to, reads,
status_printf_ln(
s, GIT_COLOR_NORMAL,
whence == FROM_MERGE ?
_("\n"
"It looks like you may be committing a merge.\n"
"If this is not correct, please run\n"
" git update-ref -d MERGE_HEAD\n"
"and try again.\n") :
_("\n"
"It looks like you may be committing a cherry-pick.\n"
"If this is not correct, please run\n"
" git update-ref -d CHERRY_PICK_HEAD\n"
"and try again.\n"));
So I am a little confused which local style convention to follow... :S
$ grep -nE ':$' builtin/commit.c
904: "and try again.\n") :
$ grep -nE '^\s*:' builtin/commit.c
1754: : is_from_rebase(whence)
1756: : "commit";
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`
2021-07-09 19:14 ` Junio C Hamano
2021-07-10 17:26 ` Hu Jialun
@ 2021-07-22 18:33 ` Hu Jialun
2021-07-22 21:18 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Hu Jialun @ 2021-07-22 18:33 UTC (permalink / raw)
To: gitster; +Cc: git
Sorry for bothering again; just checking if anything went wrong with
my previous email about ternary operator conventions :')
https://lore.kernel.org/git/20210710172655.2731007-1-hujialun@comp.nus.edu.sg/
^ permalink raw reply [flat|nested] 20+ messages in thread