| Message ID | 20210706022438.580374-1-hujialun@comp.nus.edu.sg (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: remove irrelavent prompt on `--allow-empty-message` | expand |
Hu Jialun <hujialun@comp.nus.edu.sg> writes: > Currently, COMMIT_EDITMSG contains "...and an empty message aborts the > commit", regardless of whether the --allow-empty-message option is > specified or not. This is deemed confusing and unintended. I have to wonder what reason a user have to use that option, other than wanting to record a commit without any message. In other words, wouldn't it be a bug for this command $ git commit --allow-empty-message to open an editor to allow editing the message, instead of just committing with an empty message? If we fix that, then the user wouldn't have to see "edit this file in your editor. making it empty will abort the commit" message in the first place, so all the message changes do not have to be done. If we still want to open an editor when --allow-empty-message is given, then I would say the "making it empty will abort" part of the message is *not* "confusing and unintended". It is a confused and wrong message. The resulting code looks correct, even though it is somewhat too repetitious. The existing code may want to see a preliminary clean-up patch (PATCH 1/2) to move these messages to a set of variables, so that the fix (PATCH 2/2) can swap the contents of these variables based on the value of allow_empty_message, if it makes the resulting code easier to follow (I haven't tried it, so please tell me if that improved the code or not after trying to do so ;-)). As to the proposed log message, let's not start it with "Currently", and spell out the approach to fix the issue explicitly, perhaps like. 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. Thanks. > Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg> > --- > builtin/commit.c | 41 ++++++++++++++++++++++++++++------------- > 1 file changed, 28 insertions(+), 13 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 190d215d43..61e3382db9 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -910,21 +910,36 @@ 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); > - else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { > + if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) { > + if (allow_empty_message) { > + status_printf(s, GIT_COLOR_NORMAL, > + _("Please enter the commit message for your changes." > + " Lines starting\nwith '%c' will be ignored.\n"), comment_line_char); > + } else { > + 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); > + } > + } 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. */ > + if (allow_empty_message) { > + 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"), comment_line_char); > + } else { > + 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); > + } > + } > > /* > * These should never fail because they come from our own
Junio C Hamano wrote: > Hu Jialun <hujialun@comp.nus.edu.sg> writes: > > > Currently, COMMIT_EDITMSG contains "...and an empty message aborts the > > commit", regardless of whether the --allow-empty-message option is > > specified or not. This is deemed confusing and unintended. > > I have to wonder what reason a user have to use that option, other > than wanting to record a commit without any message. In other words, > wouldn't it be a bug for this command > > $ git commit --allow-empty-message > > to open an editor to allow editing the message, instead of just > committing with an empty message? To translate this question into more programmatical terms. Should this commit an empty message? % git commit --allow-empty-message -m "$msg" I say the answer is no. We need to know what the message is.
I agree on this. If for whatever reason a user just wants to have something handy that can bypass the empty message check globally, they may no longer be able to do so via a trivial alias if an editor is not launched. Also, I guess it might be counter-intuitive for an option with the name --allow-... to force something rather than merely permitting it. I am trying to prepare the two patches and will send out once it is ready. :P
> The existing code may want to see a preliminary > clean-up patch (PATCH 1/2) to move these messages to a set of > variables, so that the fix (PATCH 2/2) can swap the contents of > these variables based on the value of allow_empty_message, if it > makes the resulting code easier to follow (I haven't tried it, so > please tell me if that improved the code or not after trying to do > so ;-)). Tried to do this and the code does seem more maintainable. I'm not exactly sure if I did it the right way, though, so please do feel free to point out where I have done improperly. Also, sorry about the previous email -- I forgot to put one newline and got the quoted text truncated. I am really new to mailing lists and might make some silly mistakes :E Hu Jialun (2): commit: reorganise duplicate commit prompt strings commit: remove irrelavent prompt on `--allow-empty-message` builtin/commit.c | 31 ++++++++++++++--------- t/t7500-commit-template-squash-signoff.sh | 4 +-- t/t7502-commit-porcelain.sh | 4 +-- 3 files changed, 23 insertions(+), 16 deletions(-)
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
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/
Đ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.
diff --git a/builtin/commit.c b/builtin/commit.c index 190d215d43..61e3382db9 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -910,21 +910,36 @@ 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); - else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) { + if (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) { + if (allow_empty_message) { + status_printf(s, GIT_COLOR_NORMAL, + _("Please enter the commit message for your changes." + " Lines starting\nwith '%c' will be ignored.\n"), comment_line_char); + } else { + 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); + } + } 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. */ + if (allow_empty_message) { + 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"), comment_line_char); + } else { + 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); + } + } /* * These should never fail because they come from our own
Currently, COMMIT_EDITMSG contains "...and an empty message aborts the commit", regardless of whether the --allow-empty-message option is specified or not. This is deemed confusing and unintended. Signed-off-by: Hu Jialun <hujialun@comp.nus.edu.sg> --- builtin/commit.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-)