{"thread":{"id":"56052","subject":"[PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","startedAt":"2021-07-06T02:34:15Z","lastAt":"2021-07-22T21:19:02Z","messageCount":20,"participants":["Hu Jialun","Junio C Hamano","Felipe Contreras","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429274","messageId":"20210706022438.580374-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":null,"subject":"[PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-06T02:24:38Z","receivedAt":"2021-07-06T02:34:15Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Currently, COMMIT_EDITMSG contains \"...and an empty message aborts the\ncommit\", regardless of whether the --allow-empty-message option is\nspecified or not. This is deemed confusing and unintended.\n\nSigned-off-by: Hu Jialun \n---\n builtin/commit.c | 41 ++++++++++++++++++++++++++++-------------\n 1 file changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43..61e3382db9 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -910,21 +910,36 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t}\n \n \t\tfprintf(s->fp, \"\\n\");\n-\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n-\t\t\t\t \" message aborts the commit.\\n\"), comment_line_char);\n-\t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n+\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n+\t\t\tif (allow_empty_message) {\n+\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n+\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\" Lines starting\\nwith '%c' will be ignored.\\n\"), comment_line_char);\n+\t\t\t} else {\n+\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n+\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\" Lines starting\\nwith '%c' will be ignored, and an empty\"\n+\t\t\t\t\t\" message aborts the commit.\\n\"), comment_line_char);\n+\t\t\t}\n+\t\t} else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n \t\t\t\twt_status_add_cut_line(s->fp);\n-\t\t} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\n\"\n-\t\t\t\t \"with '%c' will be kept; you may remove them\"\n-\t\t\t\t \" yourself if you want to.\\n\"\n-\t\t\t\t \"An empty message aborts the commit.\\n\"), comment_line_char);\n+\t\t} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n+\t\t\tif (allow_empty_message) {\n+\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n+\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\t\"with '%c' will be kept; you may remove them\"\n+\t\t\t\t\t\" yourself if you want to.\\n\"), comment_line_char);\n+\t\t\t} else {\n+\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n+\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\" Lines starting\\n\"\n+\t\t\t\t\t\"with '%c' will be kept; you may remove them\"\n+\t\t\t\t\t\" yourself if you want to.\\n\"\n+\t\t\t\t\t\"An empty message aborts the commit.\\n\"), comment_line_char);\n+\t\t\t}\n+\t\t}\n \n \t\t/*\n \t\t * These should never fail because they come from our own\n-- \n2.32.0\n\n"},{"id":"429296","messageId":"xmqqlf6jla5o.fsf@gitster.g","threadId":"56052","inReplyTo":"20210706022438.580374-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-06T15:44:19Z","receivedAt":"2021-07-06T15:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hu Jialun writes:\n\n> Currently, COMMIT_EDITMSG contains \"...and an empty message aborts the\n> commit\", regardless of whether the --allow-empty-message option is\n> specified or not. This is deemed confusing and unintended.\n\nI have to wonder what reason a user have to use that option, other\nthan wanting to record a commit without any message. In other words,\nwouldn't it be a bug for this command \n\n $ git commit --allow-empty-message\n\nto open an editor to allow editing the message, instead of just\ncommitting with an empty message? If we fix that, then the user\nwouldn't have to see \"edit this file in your editor. making it empty\nwill abort the commit\" message in the first place, so all the\nmessage changes do not have to be done.\n\nIf we still want to open an editor when --allow-empty-message is\ngiven, then I would say the \"making it empty will abort\" part of the\nmessage is *not* \"confusing and unintended\". It is a confused and\nwrong message.\n\nThe resulting code looks correct, even though it is somewhat too\nrepetitious. The existing code may want to see a preliminary\nclean-up patch (PATCH 1/2) to move these messages to a set of\nvariables, so that the fix (PATCH 2/2) can swap the contents of\nthese variables based on the value of allow_empty_message, if it\nmakes the resulting code easier to follow (I haven't tried it, so\nplease tell me if that improved the code or not after trying to do\nso ;-)).\n\nAs to the proposed log message, let's not start it with \"Currently\",\nand spell out the approach to fix the issue explicitly, perhaps like.\n\n Even when the `--allow-empty-message` option is given, \"git\n commit\" offers an interactive editor session with prefilled\n message that says the commit will be aborted if the buffer is\n emptied, which is wrong.\n\n Remove the \"an empty message aborts\" part from the message when\n the option is given to fix it.\n\nThanks.\n\n\n> Signed-off-by: Hu Jialun \n> ---\n> builtin/commit.c | 41 ++++++++++++++++++++++++++++-------------\n> 1 file changed, 28 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43..61e3382db9 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -910,21 +910,36 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t}\n> \n> \t\tfprintf(s->fp, \"\\n\");\n> -\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)\n> -\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> -\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> -\t\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> -\t\t\t\t \" message aborts the commit.\\n\"), comment_line_char);\n> -\t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n> +\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n> +\t\t\tif (allow_empty_message) {\n> +\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> +\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\" Lines starting\\nwith '%c' will be ignored.\\n\"), comment_line_char);\n> +\t\t\t} else {\n> +\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> +\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> +\t\t\t\t\t\" message aborts the commit.\\n\"), comment_line_char);\n> +\t\t\t}\n> +\t\t} else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n> \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n> \t\t\t\twt_status_add_cut_line(s->fp);\n> -\t\t} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n> -\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> -\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> -\t\t\t\t \" Lines starting\\n\"\n> -\t\t\t\t \"with '%c' will be kept; you may remove them\"\n> -\t\t\t\t \" yourself if you want to.\\n\"\n> -\t\t\t\t \"An empty message aborts the commit.\\n\"), comment_line_char);\n> +\t\t} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n> +\t\t\tif (allow_empty_message) {\n> +\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> +\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\" Lines starting\\n\"\n> +\t\t\t\t\t\"with '%c' will be kept; you may remove them\"\n> +\t\t\t\t\t\" yourself if you want to.\\n\"), comment_line_char);\n> +\t\t\t} else {\n> +\t\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> +\t\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\" Lines starting\\n\"\n> +\t\t\t\t\t\"with '%c' will be kept; you may remove them\"\n> +\t\t\t\t\t\" yourself if you want to.\\n\"\n> +\t\t\t\t\t\"An empty message aborts the commit.\\n\"), comment_line_char);\n> +\t\t\t}\n> +\t\t}\n> \n> \t\t/*\n> \t\t * These should never fail because they come from our own\n"},{"id":"429402","messageId":"60e52f945507_2fe63d2088c@natae.notmuch","threadId":"56052","inReplyTo":"xmqqlf6jla5o.fsf@gitster.g","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-07T04:37:40Z","receivedAt":"2021-07-07T04:37:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Hu Jialun writes:\n> \n> > Currently, COMMIT_EDITMSG contains \"...and an empty message aborts the\n> > commit\", regardless of whether the --allow-empty-message option is\n> > specified or not. This is deemed confusing and unintended.\n> \n> I have to wonder what reason a user have to use that option, other\n> than wanting to record a commit without any message. In other words,\n> wouldn't it be a bug for this command \n> \n> $ git commit --allow-empty-message\n> \n> to open an editor to allow editing the message, instead of just\n> committing with an empty message?\n\nTo translate this question into more programmatical terms.\n\nShould this commit an empty message?\n\n % git commit --allow-empty-message -m \"$msg\"\n\nI say the answer is no.\n\nWe need to know what the message is.\n\n-- \nFelipe Contreras\n"},{"id":"429418","messageId":"20210707104342.783609-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"xmqqlf6jla5o.fsf@gitster.g","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-07T10:43:42Z","receivedAt":"2021-07-07T10:44:22Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"I agree on this. If for whatever reason a user just wants to have\nsomething handy that can bypass the empty message check globally, they\nmay no longer be able to do so via a trivial alias if an editor is not\nlaunched. Also, I guess it might be counter-intuitive for an option with\nthe name --allow-... to force something rather than merely permitting it.\n\nI am trying to prepare the two patches and will send out once it is ready. :P\n"},{"id":"429422","messageId":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210706022438.580374-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-07T16:23:06Z","receivedAt":"2021-07-07T16:24:09Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"> The existing code may want to see a preliminary\n> clean-up patch (PATCH 1/2) to move these messages to a set of\n> variables, so that the fix (PATCH 2/2) can swap the contents of\n> these variables based on the value of allow_empty_message, if it\n> makes the resulting code easier to follow (I haven't tried it, so\n> please tell me if that improved the code or not after trying to do\n> so ;-)).\n\nTried to do this and the code does seem more maintainable. I'm not\nexactly sure if I did it the right way, though, so please do feel free\nto point out where I have done improperly.\n\nAlso, sorry about the previous email -- I forgot to put one newline and got\nthe quoted text truncated. I am really new to mailing lists and might make\nsome silly mistakes :E\n\nHu Jialun (2):\n commit: reorganise duplicate commit prompt strings\n commit: remove irrelavent prompt on `--allow-empty-message`\n\n builtin/commit.c | 31 ++++++++++++++---------\n t/t7500-commit-template-squash-signoff.sh | 4 +--\n t/t7502-commit-porcelain.sh | 4 +--\n 3 files changed, 23 insertions(+), 16 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"429423","messageId":"20210707162308.2438170-2-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","subject":"[PATCH v2 1/2] commit: reorganise duplicate commit prompt strings","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-07T16:23:07Z","receivedAt":"2021-07-07T16:24:10Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"While the prefilled commit prompt is mostly the same for different\ncleanup modes, those are separately repeated, which violates the DRY\nprinciple and hinders maintainability.\n\nUnify and reorder identical substrings to improve.\n\nSigned-off-by: Hu Jialun \n---\n builtin/commit.c | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43..815b408002 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -910,21 +910,24 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t}\n \n \t\tfprintf(s->fp, \"\\n\");\n-\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n-\t\t\t\t \" message aborts the commit.\\n\"), comment_line_char);\n+\t\tconst char *msg_enter_prompt = _(\"Please enter the commit message for your changes.\");\n+\t\tconst char *keep_char_prompt = _(\"Lines starting with '%c' will be kept;\"\n+\t\t\t\t\t\t \" you may remove them yourself if you want to.\");\n+\t\tconst char *ignore_char_prompt = _(\"Lines starting with '%c' will be ignored.\");\n+\t\tconst char *empty_msg_abort_prompt = _(\"An empty message aborts the commit.\");\n+\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t}\n \t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n \t\t\t\twt_status_add_cut_line(s->fp);\n-\t\t} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\n\"\n-\t\t\t\t \"with '%c' will be kept; you may remove them\"\n-\t\t\t\t \" yourself if you want to.\\n\"\n-\t\t\t\t \"An empty message aborts the commit.\\n\"), comment_line_char);\n+\t\t} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, keep_char_prompt, comment_line_char);\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t}\n \n \t\t/*\n \t\t * These should never fail because they come from our own\n-- \n2.32.0\n\n"},{"id":"429424","messageId":"20210707162308.2438170-3-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","subject":"[PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-07T16:23:08Z","receivedAt":"2021-07-07T16:24:13Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Even when the `--allow-empty-message` option is given, \"git\ncommit\" offers an interactive editor session with prefilled\nmessage that says the commit will be aborted if the buffer is\nemptied, which is wrong.\n\nRemove the \"an empty message aborts\" part from the message when\nthe option is given to fix it.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Hu Jialun \n---\nThe tests are also amended in line with the new string.\n\n builtin/commit.c | 8 ++++++--\n t/t7500-commit-template-squash-signoff.sh | 4 ++--\n t/t7502-commit-porcelain.sh | 4 ++--\n 3 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 815b408002..b681dcc83c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -918,7 +918,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t\tif (!allow_empty_message) {\n+\t\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t\t}\n \t\t}\n \t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n@@ -926,7 +928,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, keep_char_prompt, comment_line_char);\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t\tif (!allow_empty_message) {\n+\t\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n+\t\t\t}\n \t\t}\n \n \t\t/*\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0d..812ca45043 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -497,8 +497,8 @@ test_expect_success 'invalid message options when using --fixup' '\n \n cat >expected-template <\n #\ndiff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\nindex 38a532d81c..a5217872ca 100755\n--- a/t/t7502-commit-porcelain.sh\n+++ b/t/t7502-commit-porcelain.sh\n@@ -608,8 +608,8 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '\n \n echo \"sample\n \n-# Please enter the commit message for your changes. Lines starting\n-# with '#' will be ignored, and an empty message aborts the commit.\" >expect\n+# Please enter the commit message for your changes.\n+# Lines starting with '#' will be ignored.\" >expect\n \n test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '\n \ttest_cmp expect actual\n-- \n2.32.0\n\n"},{"id":"429426","messageId":"YOXc+mT01vptY9Nx@danh.dev","threadId":"56052","inReplyTo":"20210707162308.2438170-2-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-07-07T16:57:30Z","receivedAt":"2021-07-07T16:57:36Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Hi Jialun,\n\nOn 2021-07-08 00:23:07+0800, Hu Jialun wrote:\n> While the prefilled commit prompt is mostly the same for different\n> cleanup modes, those are separately repeated, which violates the DRY\n> principle and hinders maintainability.\n> \n> Unify and reorder identical substrings to improve.\n> \n> Signed-off-by: Hu Jialun \n> ---\n> builtin/commit.c | 27 +++++++++++++++------------\n> 1 file changed, 15 insertions(+), 12 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43..815b408002 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -910,21 +910,24 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\t}\n> \n> \t\tfprintf(s->fp, \"\\n\");\n> -\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)\n> -\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> -\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> -\t\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> -\t\t\t\t \" message aborts the commit.\\n\"), comment_line_char);\n> +\t\tconst char *msg_enter_prompt = _(\"Please enter the commit message for your changes.\");\n> +\t\tconst char *keep_char_prompt = _(\"Lines starting with '%c' will be kept;\"\n> +\t\t\t\t\t\t \" you may remove them yourself if you want to.\");\n> +\t\tconst char *ignore_char_prompt = _(\"Lines starting with '%c' will be ignored.\");\n> +\t\tconst char *empty_msg_abort_prompt = _(\"An empty message aborts the commit.\");\n\nIn Git project, it's enforced to have -Wdeclaration-after-statement,\nIOW, move all declaration before statement.\n\n\n\n> +\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n\nbuiltin/commit.c:919:4: error: format not a string literal and no format arguments [-Werror=format-security]\n 919 | status_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n\nmsg_enter_prompt will come from translator and may have '%' inside it.\n\nWe can solve it by inserting \"%s\" there.\n\nHowever, I think we shouldn't take this route, because splitting likes this\nwill make a translation lego. I can't speak for Junio, but from my\nobservation, it's preferred to have 3 variables for 3 full-text, and\nwe will pick the suitable text in each if-leg.\n\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n> +\t\t}\n> \t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n> \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n> \t\t\t\twt_status_add_cut_line(s->fp);\n> -\t\t} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n> -\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n> -\t\t\t\t_(\"Please enter the commit message for your changes.\"\n> -\t\t\t\t \" Lines starting\\n\"\n> -\t\t\t\t \"with '%c' will be kept; you may remove them\"\n> -\t\t\t\t \" yourself if you want to.\\n\"\n> -\t\t\t\t \"An empty message aborts the commit.\\n\"), comment_line_char);\n> +\t\t} else { /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, keep_char_prompt, comment_line_char);\n> +\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n> +\t\t}\n\nAfter changing those texts, the tests should be updated, too.\nIt's a customary service for the next developer, who needs to bisect\nthis project to have all test-cases pass on each changes.\n\nWith this change, t7500.50 and t7502.37 runs into failures.\nPlease fix them here, instead of next change.\n> \n> \t\t/*\n> \t\t * These should never fail because they come from our own\n> -- \n> 2.32.0\n> \n\n-- \nDanh\n"},{"id":"429429","messageId":"60e5e7a2d2ddf_30143720855@natae.notmuch","threadId":"56052","inReplyTo":"20210707162308.2438170-3-hujialun@comp.nus.edu.sg","subject":"RE: [PATCH v2 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-07T17:42:58Z","receivedAt":"2021-07-07T17:43:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hu Jialun wrote:\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -918,7 +918,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL) {\n> \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, msg_enter_prompt);\n> \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, ignore_char_prompt, comment_line_char);\n> -\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n> +\t\t\tif (!allow_empty_message) {\n> +\t\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, empty_msg_abort_prompt);\n> +\t\t\t}\n\nIn git the style is to avoid braces if the content of the condition is a\nsingle line.\n\n-- \nFelipe Contreras\n"},{"id":"429430","messageId":"xmqqzguydnnl.fsf@gitster.g","threadId":"56052","inReplyTo":"YOXc+mT01vptY9Nx@danh.dev","subject":"Re: [PATCH v2 1/2] commit: reorganise duplicate commit prompt strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T17:44:30Z","receivedAt":"2021-07-07T17:44:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n> However, I think we shouldn't take this route, because splitting likes this\n> will make a translation lego. I can't speak for Junio, but from my\n> observation, it's preferred to have 3 variables for 3 full-text, and\n> we will pick the suitable text in each if-leg.\n\nYes, that is what I meant in my earlier suggestion. More like\n\n char *hint_cleanup_all =\n\t_(\"Please enter the ... , and an empty message aborts the commit.\\n\");\n char *hint_cleanup_space =\n\t_(\"Please enter the ... if you want to.\\n\"\n \"An empty message aborts the commit.\\n\");\n\n if (allow_empty_message) {\n hint_cleanup_all = _(\"...\");\n hint_cleanup_space = _(\"...\");\n }\n\n ... the if/elseif cascade in which calls to status_printf() are made\n ... using these variables\n\nThanks.\n"},{"id":"429493","messageId":"20210708151911.2524122-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-08T15:19:11Z","receivedAt":"2021-07-08T15:19:43Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Junio C Hamano wrote:\n> char *hint_cleanup_all =\n> _(\"Please enter the ... , and an empty message aborts the commit.\\n\");\n> char *hint_cleanup_space =\n> _(\"Please enter the ... if you want to.\\n\"\n> \"An empty message aborts the commit.\\n\");\n>\n> if (allow_empty_message) {\n> hint_cleanup_all = _(\"...\");\n> hint_cleanup_space = _(\"...\");\n> }\n>\n> ... the if/elseif cascade in which calls to status_printf() are made\n> ... using these variables\n\nWould it be better this way or just using the ternary operator in-line\ninstead? If the latter, should it still be separated into another\nvariable or just embedded in the status_printf call? Using the ternary\noperator does require to separate checks of allow_empty_message, but\nmight as well save us an `if` construct to reassign the variable.\n\nIn other words, which of the following 3 is the most acceptable?\n\n1. As Junio suggested, quoted above.\n\n2.\nstatus_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?\n _(\"...\") :\n\t\t\t\t _(\"....\"), comment_line_char);\n\n3.\nconst char *hint_foo = allow_empty_message ?\n _(\"...\") :\n\t\t _(\"....\");\n......\nstatus_printf(s, GIT_COLOR_NORMAL, hint_foo, comment_line_char);\n\n--------------------------------------------------------------------\n\nFelipe Contreras wrote:\n> In git the style is to avoid braces if the content of the condition is a\n> single line.\n\nĐoàn Trần Công Danh wrote:\n> In Git project, it's enforced to have -Wdeclaration-after-statement,\n> IOW, move all declaration before statement.\n\nNoted with thanks!\n\n> After changing those texts, the tests should be updated, too.\n> It's a customary service for the next developer, who needs to bisect\n> this project to have all test-cases pass on each changes.\n> \n> With this change, t7500.50 and t7502.37 runs into failures.\n> Please fix them here, instead of next change.\n\nI did change test cases accordingly in the second patch (excerpt below), and\nboth tests did pass afterwards. Was there something wrong with it?\n\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 7d02f79c0d..812ca45043 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -497,8 +497,8 @@ test_expect_success 'invalid message options when using --fixup' '\n> \n> cat >expected-template < \n> -# Please enter the commit message for your changes. Lines starting\n> -# with '#' will be ignored, and an empty message aborts the commit.\n> +# Please enter the commit message for your changes.\n> +# Lines starting with '#' will be ignored.\n> #\n> # Author: A U Thor \n> #\n> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh\n> index 38a532d81c..a5217872ca 100755\n> --- a/t/t7502-commit-porcelain.sh\n> +++ b/t/t7502-commit-porcelain.sh\n> @@ -608,8 +608,8 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '\n> \n> echo \"sample\n> \n> -# Please enter the commit message for your changes. Lines starting\n> -# with '#' will be ignored, and an empty message aborts the commit.\" >expect\n> +# Please enter the commit message for your changes.\n> +# Lines starting with '#' will be ignored.\" >expect\n> \n> test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '\n> \ttest_cmp expect actual\n\n--------------------------------------------------------------------\n\nAnd some perhaps rather noob questions below, as an (overly) curious\nnewcomer,\n\n- Why is the \"lego\" style breakdown of translation strings unrecommended?\n I suppose it might be in consideration of possibly different linguistic\n sequences across languages but I'm not so sure.\n\n- What is the rationale behind prohibiting braces around single line\n constructs? It seems somewhat error-prone since somebody else could\n later be adding statements into the body without putting the curly\n braces.\n\n- When replying to multiple comments in multiple emails (like in this very\n email), would it be better to send multiple emails as replies to individual\n comments or do it in one email? If the latter, which previous message should\n the single reply be In-Reply-To?\n\nThanks in advance,\nHu Jialun\n"},{"id":"429498","messageId":"YOciZUlWnF5ur5ec@danh.dev","threadId":"56052","inReplyTo":"20210708151911.2524122-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-07-08T16:05:57Z","receivedAt":"2021-07-08T16:06:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-07-08 23:19:11+0800, Hu Jialun wrote:\n> Junio C Hamano wrote:\n> > char *hint_cleanup_all =\n> > _(\"Please enter the ... , and an empty message aborts the commit.\\n\");\n> > char *hint_cleanup_space =\n> > _(\"Please enter the ... if you want to.\\n\"\n> > \"An empty message aborts the commit.\\n\");\n> >\n> > if (allow_empty_message) {\n> > hint_cleanup_all = _(\"...\");\n> > hint_cleanup_space = _(\"...\");\n> > }\n> >\n> > ... the if/elseif cascade in which calls to status_printf() are made\n> > ... using these variables\n> \n> Would it be better this way or just using the ternary operator in-line\n> instead? If the latter, should it still be separated into another\n> variable or just embedded in the status_printf call? Using the ternary\n> operator does require to separate checks of allow_empty_message, but\n> might as well save us an `if` construct to reassign the variable.\n> \n> In other words, which of the following 3 is the most acceptable?\n> \n> 1. As Junio suggested, quoted above.\n\nI think this approach is the most expensive one, _() needs to query\nthe gettext infrastructure, which is usually costly.\nHowever, I think that cost doesn't matter much since we're about to\nopen an editor soon.\n\n> 2.\n> status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?\n> _(\"...\") :\n> \t\t\t\t _(\"....\"), comment_line_char);\n\ninstall_branch_config() uses this style.\n\n> \n> 3.\n> const char *hint_foo = allow_empty_message ?\n> _(\"...\") :\n> \t\t _(\"....\");\n\nbuiltin/remote.c:show_local_info_item() writes:\n\n\tconst char *msg;\n\tif (condition)\n\t\tmsg = _(\"some message\");\n\telse\n\t\tmsg = _(\"other message\");\n\nSo, I guess it's fine either way. And people will need to see the\npatch to see which one is better.\n\n> ......\n> status_printf(s, GIT_COLOR_NORMAL, hint_foo, comment_line_char);\n> \n> --------------------------------------------------------------------\n> \n> Felipe Contreras wrote:\n> > In git the style is to avoid braces if the content of the condition is a\n> > single line.\n> \n> Đoàn Trần Công Danh wrote:\n> > In Git project, it's enforced to have -Wdeclaration-after-statement,\n> > IOW, move all declaration before statement.\n> \n> Noted with thanks!\n> \n> > After changing those texts, the tests should be updated, too.\n> > It's a customary service for the next developer, who needs to bisect\n> > this project to have all test-cases pass on each changes.\n> > \n> > With this change, t7500.50 and t7502.37 runs into failures.\n> > Please fix them here, instead of next change.\n> \n> I did change test cases accordingly in the second patch (excerpt below), and\n> both tests did pass afterwards. Was there something wrong with it?\n\nYes, when apply both 2 patches, the test passed, however, the test\ndoesn't pass with only 1/2 applied. Let's imagine in a near future,\nsome developers need to bisect some problems with Git with automation\nscripts, and git-bisect stops at 1/2, since the tests report failure,\n\"git bisect run\" will mark this change as \"bad commit\", thus render\ngit-bisect hard to use. We should make sure all tests pass on all\ncommit.\n\n> And some perhaps rather noob questions below, as an (overly) curious\n> newcomer,\n> \n> - Why is the \"lego\" style breakdown of translation strings unrecommended?\n> I suppose it might be in consideration of possibly different linguistic\n> sequences across languages but I'm not so sure.\n\nLet's imagine an artificial language which have 2 words \"linos\" and\n\"linas\" which is both translated to English as \"lines\", translators\nneed full context to decide which word should be chosen. Things maybe\ncomplicated with language with gender, word-cases, etc...\n\nThere're some problems reported on and off this list [1]\n\n> - What is the rationale behind prohibiting braces around single line\n> constructs? It seems somewhat error-prone since somebody else could\n> later be adding statements into the body without putting the curly\n> braces.\n\nDocumentation/CodingGuidelines said so ;)\nI don't think somebody adding random statements is a valid concern for\nbrace, I think it's expected to analyse the code context before doing\nreal-work on project. Furthermore, -Wmisleading-indentation is your\nfriends.\n\n\n1: https://lore.kernel.org/git/20210509215250.33215-1-alexhenrie24@gmail.com/\n\n-- \nDanh\n"},{"id":"429506","messageId":"xmqqfswobr1u.fsf@gitster.g","threadId":"56052","inReplyTo":"YOciZUlWnF5ur5ec@danh.dev","subject":"Re: [PATCH] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T18:26:21Z","receivedAt":"2021-07-08T18:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n>> In other words, which of the following 3 is the most acceptable?\n>> \n>> 1. As Junio suggested, quoted above.\n>\n> I think this approach is the most expensive one, _() needs to query\n> the gettext infrastructure, which is usually costly.\n> However, I think that cost doesn't matter much since we're about to\n> open an editor soon.\n\nSee note below.\n\n\n>> 2.\n>> status_printf(s, GIT_COLOR_NORMAL, allow_empty_message ?\n>> _(\"...\") :\n>> \t\t\t\t _(\"....\"), comment_line_char);\n>\n> install_branch_config() uses this style.\n>\n>> \n>> 3.\n>> const char *hint_foo = allow_empty_message ?\n>> _(\"...\") :\n>> \t\t _(\"....\");\n>\n> builtin/remote.c:show_local_info_item() writes:\n>\n> \tconst char *msg;\n> \tif (condition)\n> \t\tmsg = _(\"some message\");\n> \telse\n> \t\tmsg = _(\"other message\");\n>\n> So, I guess it's fine either way. And people will need to see the\n> patch to see which one is better.\n\nYeah, #1 and #3 are better than the patch posted or #2 in that by\nextracting the large message body out-of-line from the code that do\nuse the messages, they make it simpler to follow the logic that uses\nthese messages. That is\n\n\tif (cleanup_mode == CLEANUP_ALL)\n\t\tstatus_printf(..., hint_cleanup_all);\n\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n\t\t...;\n\telse /* all the rest */\n\t\tstatus_printf(..., hint_cleanup_space, comment_line_char);\n\nwould be far easier to follow than\n\n\tif (cleanup_mode == CLEANUP_ALL)\n\t\tstatus_printf(..., \n\t\tcondition \n\t\t? large-large-message-1\n\t\t: large-large-message-1-plus-note-about-empty-message);\n\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n\t\t...;\n\telse /* all the rest */\n\t\tstatus_printf(...,\n\t\tcondition\n\t\t? large-large-message-2\n\t\t: large-large-message-2-plus-note-about-empty-message,\n\t\tcomment_line_char);\n\nas the overall structure is easier to follow without the minute\ndetail of using slightly different messages depending on the\nallow-empty setting.\n\nBy the way, if you want to avoid calling _() twice with the approach\n#1, you can do\n\n\thint_cleanup_all = N_(\"\");\n \t...\n\n\tif (condition) {\n\t\thint_cleanup_all = N_(\"\");\n\t\t...\n\t}\n\nand use _(hint_cleanup_all) at the site that uses the message.\n"},{"id":"429573","messageId":"20210709180732.2678818-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","subject":"[PATCH v3 1/2] commit: reorganise commit hint strings","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-09T18:07:31Z","receivedAt":"2021-07-09T18:08:14Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Strings of hint messages inserted into editor on interactive commit was\nscattered in-line, rendering the code harder to understand at first\nglance.\n\nExtract those messages out into separate variables to make the code\noutline easier to follow.\n\nHelped-by: Junio C Hamano \nHelped-by: Đoàn Trần Công Danh \nSigned-off-by: Hu Jialun \n---\n builtin/commit.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 190d215d43..e68d139dee 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -889,6 +889,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tint ident_shown = 0;\n \t\tint saved_color_setting;\n \t\tstruct ident_split ci, ai;\n+\t\tconst char *hint_cleanup_all = _(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\t \" Lines starting\\nwith '%c' will be ignored,\"\n+\t\t\t\t\t\t \" and an empty message aborts the commit.\\n\");\n+\t\tconst char *hint_cleanup_space = _(\"Please enter the commit message for your changes.\"\n+\t\t\t\t\t\t \" Lines starting\\n\"\n+\t\t\t\t\t\t \"with '%c' will be kept; you may remove them\"\n+\t\t\t\t\t\t \" yourself if you want to.\\n\"\n+\t\t\t\t\t\t \"An empty message aborts the commit.\\n\");\n \n \t\tif (whence != FROM_COMMIT) {\n \t\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&\n@@ -911,20 +919,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \n \t\tfprintf(s->fp, \"\\n\");\n \t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_ALL)\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n-\t\t\t\t \" message aborts the commit.\\n\"), comment_line_char);\n+\t\t\tstatus_printf(s, GIT_COLOR_NORMAL, hint_cleanup_all, comment_line_char);\n \t\telse if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n \t\t\tif (whence == FROM_COMMIT && !merge_contains_scissors)\n \t\t\t\twt_status_add_cut_line(s->fp);\n \t\t} else /* COMMIT_MSG_CLEANUP_SPACE, that is. */\n-\t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n-\t\t\t\t_(\"Please enter the commit message for your changes.\"\n-\t\t\t\t \" Lines starting\\n\"\n-\t\t\t\t \"with '%c' will be kept; you may remove them\"\n-\t\t\t\t \" yourself if you want to.\\n\"\n-\t\t\t\t \"An empty message aborts the commit.\\n\"), comment_line_char);\n+\t\t\tstatus_printf(s, GIT_COLOR_NORMAL, hint_cleanup_space, comment_line_char);\n \n \t\t/*\n \t\t * These should never fail because they come from our own\n-- \n2.32.0\n\n"},{"id":"429574","messageId":"20210709180732.2678818-2-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"20210707162308.2438170-1-hujialun@comp.nus.edu.sg","subject":"[PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-09T18:07:32Z","receivedAt":"2021-07-09T18:08:17Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Even when the `--allow-empty-message` option is given, \"git commit\"\noffers an interactive editor session with prefilled message that says\nthe commit will be aborted if the buffer is emptied, which is wrong.\n\nRemove the \"an empty message aborts\" part from the message when the\noption is given to fix it.\n\nHelped-by: Junio C Hamano \nHelped-by: Đoàn Trần Công Danh \nHelped-by: Felipe Contreras \nSigned-off-by: Hu Jialun \n---\n builtin/commit.c | 25 +++++++++++++++--------\n t/t7500-commit-template-squash-signoff.sh | 2 +-\n 2 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e68d139dee..cfbc83751a 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -889,15 +889,22 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tint ident_shown = 0;\n \t\tint saved_color_setting;\n \t\tstruct ident_split ci, ai;\n-\t\tconst char *hint_cleanup_all = _(\"Please enter the commit message for your changes.\"\n-\t\t\t\t\t\t \" Lines starting\\nwith '%c' will be ignored,\"\n-\t\t\t\t\t\t \" and an empty message aborts the commit.\\n\");\n-\t\tconst char *hint_cleanup_space = _(\"Please enter the commit message for your changes.\"\n-\t\t\t\t\t\t \" Lines starting\\n\"\n-\t\t\t\t\t\t \"with '%c' will be kept; you may remove them\"\n-\t\t\t\t\t\t \" yourself if you want to.\\n\"\n-\t\t\t\t\t\t \"An empty message aborts the commit.\\n\");\n-\n+\t\tconst char *hint_cleanup_all = allow_empty_message ?\n+\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t \" Lines starting\\nwith '%c' will be ignored.\\n\") :\n+\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n+\t\t\t \" message aborts the commit.\\n\");\n+\t\tconst char *hint_cleanup_space = allow_empty_message ?\n+\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t \" Lines starting\\n\"\n+\t\t\t \"with '%c' will be kept; you may remove them\"\n+\t\t\t \" yourself if you want to.\\n\") :\n+\t\t\t_(\"Please enter the commit message for your changes.\"\n+\t\t\t \" Lines starting\\n\"\n+\t\t\t \"with '%c' will be kept; you may remove them\"\n+\t\t\t \" yourself if you want to.\\n\"\n+\t\t\t \"An empty message aborts the commit.\\n\");\n \t\tif (whence != FROM_COMMIT) {\n \t\t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&\n \t\t\t\t!merge_contains_scissors)\ndiff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\nindex 7d02f79c0d..54c2082acb 100755\n--- a/t/t7500-commit-template-squash-signoff.sh\n+++ b/t/t7500-commit-template-squash-signoff.sh\n@@ -498,7 +498,7 @@ test_expect_success 'invalid message options when using --fixup' '\n cat >expected-template <\n #\n-- \n2.32.0\n\n"},{"id":"429577","messageId":"xmqq7dhz711a.fsf@gitster.g","threadId":"56052","inReplyTo":"20210709180732.2678818-2-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T19:14:09Z","receivedAt":"2021-07-09T19:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hu Jialun writes:\n\n> Even when the `--allow-empty-message` option is given, \"git commit\"\n> offers an interactive editor session with prefilled message that says\n> the commit will be aborted if the buffer is emptied, which is wrong.\n>\n> Remove the \"an empty message aborts\" part from the message when the\n> option is given to fix it.\n\nThe updated log message is much better than the old iteration, where\nthe \"empty message aborts\" was called \"irrelevant\" when it is actively\n\"wrong\". Now we call it \"wrong\".\n\nLet's update the title of the commit to match, e.g.\n\n commit: tweak cleanup hint when --allow-empty-message is in effect\n\nor something along that line.\n\n> +\t\tconst char *hint_cleanup_all = allow_empty_message ?\n> +\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t \" Lines starting\\nwith '%c' will be ignored.\\n\") :\n> +\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> +\t\t\t \" message aborts the commit.\\n\");\n> +\t\tconst char *hint_cleanup_space = allow_empty_message ?\n> +\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t \" Lines starting\\n\"\n> +\t\t\t \"with '%c' will be kept; you may remove them\"\n> +\t\t\t \" yourself if you want to.\\n\") :\n> +\t\t\t_(\"Please enter the commit message for your changes.\"\n> +\t\t\t \" Lines starting\\n\"\n> +\t\t\t \"with '%c' will be kept; you may remove them\"\n> +\t\t\t \" yourself if you want to.\\n\"\n> +\t\t\t \"An empty message aborts the commit.\\n\");\n\nLocal convention in this file seems to be that multi-line ?:\nexpressions are folded with ? and : operators at the beginning, not\nat the end, of the lines, e.g. I see this construct in the same file.\n\n\t\t\treflog_msg = is_from_cherry_pick(whence)\n\t\t\t\t\t? \"commit (cherry-pick)\"\n\t\t\t\t\t: is_from_rebase(whence)\n\t\t\t\t\t? \"commit (rebase)\"\n\t\t\t\t\t: \"commit\";\n\nSo, let's mimick and make the above more like this:\n\n\t\tconst char *hint_cleanup_all = allow_empty_message\n\t\t\t? _(\"Please enter the commit message for your changes.\"\n\t\t\t \" Lines starting\\nwith '%c' will be ignored.\\n\")\n\t\t\t: _(\"Please enter the commit message for your changes.\"\n\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n\t\t\t \" message aborts the commit.\\n\");\n\n> diff --git a/t/t7500-commit-template-squash-signoff.sh b/t/t7500-commit-template-squash-signoff.sh\n> index 7d02f79c0d..54c2082acb 100755\n> --- a/t/t7500-commit-template-squash-signoff.sh\n> +++ b/t/t7500-commit-template-squash-signoff.sh\n> @@ -498,7 +498,7 @@ test_expect_success 'invalid message options when using --fixup' '\n> cat >expected-template < \n> # Please enter the commit message for your changes. Lines starting\n> -# with '#' will be ignored, and an empty message aborts the commit.\n> +# with '#' will be ignored.\n> #\n> # Author: A U Thor \n> #\n\nPerfect.\n"},{"id":"429578","messageId":"xmqqzguv5mg5.fsf@gitster.g","threadId":"56052","inReplyTo":"20210709180732.2678818-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH v3 1/2] commit: reorganise commit hint strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T19:14:34Z","receivedAt":"2021-07-09T19:14:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hu Jialun writes:\n\n> Strings of hint messages inserted into editor on interactive commit was\n> scattered in-line, rendering the code harder to understand at first\n> glance.\n>\n> Extract those messages out into separate variables to make the code\n> outline easier to follow.\n>\n> Helped-by: Junio C Hamano \n> Helped-by: Đoàn Trần Công Danh \n> Signed-off-by: Hu Jialun \n> ---\n> builtin/commit.c | 20 ++++++++++----------\n> 1 file changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 190d215d43..e68d139dee 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -889,6 +889,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\tint ident_shown = 0;\n> \t\tint saved_color_setting;\n> \t\tstruct ident_split ci, ai;\n> +\t\tconst char *hint_cleanup_all = _(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\t \" Lines starting\\nwith '%c' will be ignored,\"\n> +\t\t\t\t\t\t \" and an empty message aborts the commit.\\n\");\n> +\t\tconst char *hint_cleanup_space = _(\"Please enter the commit message for your changes.\"\n> +\t\t\t\t\t\t \" Lines starting\\n\"\n> +\t\t\t\t\t\t \"with '%c' will be kept; you may remove them\"\n> +\t\t\t\t\t\t \" yourself if you want to.\\n\"\n> +\t\t\t\t\t\t \"An empty message aborts the commit.\\n\");\n\nThat would easily make lines that are overly long. Perhaps fold\nthem like so?\n\n\t\tconst char *hint_cleanup_all =\n\t\t\t_(\"Please enter the commit message for your changes.\"\n\t\t\t \" Lines starting\\nwith '%c' will be ignored,\"\n\t\t\t \" and an empty message aborts the commit.\\n\");\n\n"},{"id":"429636","messageId":"20210710172655.2731007-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"xmqq7dhz711a.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-10T17:26:55Z","receivedAt":"2021-07-10T17:27:49Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Junio C Hamano wrote:\n> > +\t\tconst char *hint_cleanup_all = allow_empty_message ?\n> > +\t\t\t_(\"Please enter the commit message for your changes.\"\n> > +\t\t\t \" Lines starting\\nwith '%c' will be ignored.\\n\") :\n> > +\t\t\t_(\"Please enter the commit message for your changes.\"\n> > +\t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> > +\t\t\t \" message aborts the commit.\\n\");\n> > +\t\tconst char *hint_cleanup_space = allow_empty_message ?\n> > +\t\t\t_(\"Please enter the commit message for your changes.\"\n> > +\t\t\t \" Lines starting\\n\"\n> > +\t\t\t \"with '%c' will be kept; you may remove them\"\n> > +\t\t\t \" yourself if you want to.\\n\") :\n> > +\t\t\t_(\"Please enter the commit message for your changes.\"\n> > +\t\t\t \" Lines starting\\n\"\n> > +\t\t\t \"with '%c' will be kept; you may remove them\"\n> > +\t\t\t \" yourself if you want to.\\n\"\n> > +\t\t\t \"An empty message aborts the commit.\\n\");\n> \n> Local convention in this file seems to be that multi-line ?:\n> expressions are folded with ? and : operators at the beginning, not\n> at the end, of the lines, e.g. I see this construct in the same file.\n> \n> \t\t\treflog_msg = is_from_cherry_pick(whence)\n> \t\t\t\t\t? \"commit (cherry-pick)\"\n> \t\t\t\t\t: is_from_rebase(whence)\n> \t\t\t\t\t? \"commit (rebase)\"\n> \t\t\t\t\t: \"commit\";\n> \n> So, let's mimick and make the above more like this:\n> \n> \t\tconst char *hint_cleanup_all = allow_empty_message\n> \t\t\t? _(\"Please enter the commit message for your changes.\"\n> \t\t\t \" Lines starting\\nwith '%c' will be ignored.\\n\")\n> \t\t\t: _(\"Please enter the commit message for your changes.\"\n> \t\t\t \" Lines starting\\nwith '%c' will be ignored, and an empty\"\n> \t\t\t \" message aborts the commit.\\n\");\n\nAnother statement several lines below, whose style I conformed to, reads,\n\n\tstatus_printf_ln(\n\t\ts, GIT_COLOR_NORMAL,\n\t\twhence == FROM_MERGE ?\n\t\t\t _(\"\\n\"\n\t\t\t \"It looks like you may be committing a merge.\\n\"\n\t\t\t \"If this is not correct, please run\\n\"\n\t\t\t \"\tgit update-ref -d MERGE_HEAD\\n\"\n\t\t\t \"and try again.\\n\") :\n\t\t\t _(\"\\n\"\n\t\t\t \"It looks like you may be committing a cherry-pick.\\n\"\n\t\t\t \"If this is not correct, please run\\n\"\n\t\t\t \"\tgit update-ref -d CHERRY_PICK_HEAD\\n\"\n\t\t\t \"and try again.\\n\"));\n\nSo I am a little confused which local style convention to follow... :S\n\n\t$ grep -nE ':$' builtin/commit.c\n\t 904: \"and try again.\\n\") :\n\t$ grep -nE '^\\s*:' builtin/commit.c \n\t 1754: : is_from_rebase(whence)\n\t 1756: : \"commit\";\n"},{"id":"430943","messageId":"20210722183332.814139-1-hujialun@comp.nus.edu.sg","threadId":"56052","inReplyTo":"xmqq7dhz711a.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Hu Jialun","fromEmail":"hujialun@comp.nus.edu.sg","sentAt":"2021-07-22T18:33:32Z","receivedAt":"2021-07-22T18:34:04Z","isPatch":true,"sender":{"key":"hujialun@comp.nus.edu.sg","avatar":"https://avatars.githubusercontent.com/u/59951032?v=4"},"body":"Sorry for bothering again; just checking if anything went wrong with\nmy previous email about ternary operator conventions :')\n\nhttps://lore.kernel.org/git/20210710172655.2731007-1-hujialun@comp.nus.edu.sg/\n"},{"id":"430950","messageId":"xmqqsg0682tg.fsf@gitster.g","threadId":"56052","inReplyTo":"20210722183332.814139-1-hujialun@comp.nus.edu.sg","subject":"Re: [PATCH v3 2/2] commit: remove irrelavent prompt on `--allow-empty-message`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-22T21:18:51Z","receivedAt":"2021-07-22T21:19:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hu Jialun writes:\n\n> Sorry for bothering again; just checking if anything went wrong with\n> my previous email about ternary operator conventions :')\n>\n> https://lore.kernel.org/git/20210710172655.2731007-1-hujialun@comp.nus.edu.sg/\n\nI do not think anything went wrong.\n\nYou counted between wrapping after ?: vs before ?: and showed that\nlocally line-wrapping just before ?:, so that ? and : appears after\nthe indent, happens more often, no?\n\nIt was my understanding, after your finding above, that we have been\nwaiting for an updated patch to mimic the majority.\n\nThanks.\n"}]}