{"thread":{"id":"47737","subject":"[PATCHv2] tag: add --edit option","startedAt":"2018-02-01T17:21:09Z","lastAt":"2018-02-04T15:57:22Z","messageCount":7,"participants":["Nicolas Morey-Chaisemartin","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"338032","messageId":"e99947cf-93ba-9376-f059-7f6a369d3ad5@suse.com","threadId":"47737","inReplyTo":null,"subject":"[PATCHv2] tag: add --edit option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2018-02-01T17:21:03Z","receivedAt":"2018-02-01T17:21:09Z","isPatch":false,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Add a --edit option whichs allows modifying the messages provided by -m or -F,\nthe same way git commit --edit does.\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n\nChanges since v1:\n- Fix usage string\n- Use write_script to generate editor\n- Rename editor to fakeeditor to match the other tests in the testsuite\n- I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails\n\n Documentation/git-tag.txt | 6 ++++++\n builtin/tag.c | 11 +++++++++--\n t/t7004-tag.sh | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 45 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 956fc019f984..b9e5a993bea0 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.\n \tImplies `-a` if none of `-a`, `-s`, or `-u `\n \tis given.\n \n+-e::\n+--edit::\n+\tThe message taken from file with `-F` and command line with\n+\t`-m` are usually used as the tag message unmodified.\n+\tThis option lets you further edit the message taken from these sources.\n+\n --cleanup=::\n \tThis option sets how the tag message is cleaned up.\n \tThe '' can be one of 'verbatim', 'whitespace' and 'strip'. The\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a7e6a5b0f234..ce5cac3dd23f 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu\n \n struct create_tag_options {\n \tunsigned int message_given:1;\n+\tunsigned int use_editor:1;\n \tunsigned int sign;\n \tenum {\n \t\tCLEANUP_NONE,\n@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, const char *tag,\n \t\t tag,\n \t\t git_committer_info(IDENT_STRICT));\n \n-\tif (!opt->message_given) {\n+\tif (!opt->message_given || opt->use_editor) {\n \t\tint fd;\n \n \t\t/* write the template message before editing: */\n@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, const char *tag,\n \t\tif (fd < 0)\n \t\t\tdie_errno(_(\"could not create file '%s'\"), path);\n \n-\t\tif (!is_null_oid(prev)) {\n+\t\tif (opt->message_given) {\n+\t\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t\t\tstrbuf_reset(buf);\n+\t\t} else if (!is_null_oid(prev)) {\n \t\t\twrite_tag_body(fd, prev);\n \t\t} else {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \tint icase = 0;\n+\tint edit_flag = 0;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n \t\t{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK('m', \"message\", &msg, N_(\"message\"),\n \t\t\t N_(\"tag message\"), parse_msg_arg),\n \t\tOPT_FILENAME('F', \"file\", &msgfile, N_(\"read message from file\")),\n+\t\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of tag message\")),\n \t\tOPT_BOOL('s', \"sign\", &opt.sign, N_(\"annotated and GPG-signed tag\")),\n \t\tOPT_STRING(0, \"cleanup\", &cleanup_arg, N_(\"mode\"),\n \t\t\tN_(\"how to strip spaces and #comments from message\")),\n@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"tag '%s' already exists\"), tag);\n \n \topt.message_given = msg.given || msgfile;\n+\topt.use_editor = edit_flag;\n \n \tif (!cleanup_arg || !strcmp(cleanup_arg, \"strip\"))\n \t\topt.cleanup_mode = CLEANUP_ALL;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex a9af2de9960b..063996ddc05c 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -452,6 +452,21 @@ test_expect_success \\\n \ttest_cmp expect actual\n '\n \n+get_tag_header annotated-tag-edit $commit commit $time >expect\n+echo \"An edited message\" >>expect\n+test_expect_success 'set up editor' '\n+\twrite_script fakeeditor <<-\\EOF\n+\tsed -e \"s/A message/An edited message/g\" <\"$1\" >\"$1-\"\n+\tmv \"$1-\" \"$1\"\n+\tEOF\n+'\n+test_expect_success \\\n+\t'creating an annotated tag with -m message --edit should succeed' '\n+\tEDITOR=./fakeeditor\tgit tag -m \"A message\" --edit annotated-tag-edit &&\n+\tget_tag_msg annotated-tag-edit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >msgfile <expect\n+sed -e \"s/Another message/Another edited message/g\" msgfile >>expect\n+test_expect_success 'set up editor' '\n+\twrite_script fakeeditor <<-\\EOF\n+\tsed -e \"s/Another message/Another edited message/g\" <\"$1\" >\"$1-\"\n+\tmv \"$1-\" \"$1\"\n+\tEOF\n+'\n+test_expect_success \\\n+\t'creating an annotated tag with -F messagefile --edit should succeed' '\n+\tEDITOR=./fakeeditor\tgit tag -F msgfile --edit file-annotated-tag-edit &&\n+\tget_tag_msg file-annotated-tag-edit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >inputmsg < wrote:\n> Add a --edit option whichs allows modifying the messages provided by -m or -F,\n> the same way git commit --edit does.\n>\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> Changes since v1:\n> - Fix usage string\n> - Use write_script to generate editor\n> - Rename editor to fakeeditor to match the other tests in the testsuite\n\nThanks for explaining what changed since the previous attempt. It is\nalso helpful for reviewers if you include a reference to the previous\niteration, like this:\nhttps://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7f7c@suse.com/T/#u\n\nCc:'ing reviewers of previous iterations is also good etiquette when\nsubmitting a new version.\n\n> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails\n\nTypically, it's easier on Junio, from a patch management standpoint,\nif you submit all these related patches as a single series.\nAlternately, if you do want to submit those changes separately, before\nthe current patch lands in \"master\", be sure to mention atop which\npatch (this one) the additional patch(es) should live. Thanks.\n\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.\n> +-e::\n> +--edit::\n> + The message taken from file with `-F` and command line with\n> + `-m` are usually used as the tag message unmodified.\n> + This option lets you further edit the message taken from these sources.\n\nYou probably ought to add this new option to the command synopsis. In\nthe git-commit man page, the synopsis mentions only '-e' (not --edit),\nso perhaps this man page could mirror that one. (Sorry for not\nnoticing this earlier.)\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> @@ -452,6 +452,21 @@ test_expect_success \\\n> +get_tag_header annotated-tag-edit $commit commit $time >expect\n> +echo \"An edited message\" >>expect\n\nModern practice is to perform these \"expect\" setup actions (and all\nother actions) within tests themselves rather than outside of tests.\nHowever, consistency also has value, and since this test script is\nfilled with this sort of stylized \"expect\" setup already, this may be\nfine, and probably not worth a re-roll. (A \"modernization\" patch by\nsomeone can come later if warranted.)\n\n> +test_expect_success 'set up editor' '\n> + write_script fakeeditor <<-\\EOF\n> + sed -e \"s/A message/An edited message/g\" <\"$1\" >\"$1-\"\n> + mv \"$1-\" \"$1\"\n> + EOF\n> +'\n"},{"id":"338084","messageId":"fa3f512a-bd77-80c7-4fec-071639f62d26@suse.com","threadId":"47737","inReplyTo":"CAPig+cT8vKyhq6DvFMz-0CPRO-Y7R4EE_JhN6yuiSUNXW8-Yww@mail.gmail.com","subject":"Re: [PATCHv2] tag: add --edit option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2018-02-02T07:15:28Z","receivedAt":"2018-02-02T09:32:59Z","isPatch":false,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 02/02/2018 à 02:29, Eric Sunshine a écrit :\n> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin\n> wrote:\n>> Add a --edit option whichs allows modifying the messages provided by -m or -F,\n>> the same way git commit --edit does.\n>>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> Changes since v1:\n>> - Fix usage string\n>> - Use write_script to generate editor\n>> - Rename editor to fakeeditor to match the other tests in the testsuite\n> Thanks for explaining what changed since the previous attempt. It is\n> also helpful for reviewers if you include a reference to the previous\n> iteration, like this:\n> https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7f7c@suse.com/T/#u\n>\n> Cc:'ing reviewers of previous iterations is also good etiquette when\n> submitting a new version.\n\nI thought I did. My script might be glitchy. Sorry for that.\n\n>\n>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails\n> Typically, it's easier on Junio, from a patch management standpoint,\n> if you submit all these related patches as a single series.\n> Alternately, if you do want to submit those changes separately, before\n> the current patch lands in \"master\", be sure to mention atop which\n> patch (this one) the additional patch(es) should live. Thanks.\n\nWell this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.\nPlus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),\nand I don't want this patch to get stuck in the pipe for that :)\n\n>\n>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n>> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without annotation lines.\n>> +-e::\n>> +--edit::\n>> + The message taken from file with `-F` and command line with\n>> + `-m` are usually used as the tag message unmodified.\n>> + This option lets you further edit the message taken from these sources.\n> You probably ought to add this new option to the command synopsis. In\n> the git-commit man page, the synopsis mentions only '-e' (not --edit),\n> so perhaps this man page could mirror that one. (Sorry for not\n> noticing this earlier.)\n\nYep makes sense.\n\n>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>> @@ -452,6 +452,21 @@ test_expect_success \\\n>> +get_tag_header annotated-tag-edit $commit commit $time >expect\n>> +echo \"An edited message\" >>expect\n> Modern practice is to perform these \"expect\" setup actions (and all\n> other actions) within tests themselves rather than outside of tests.\n> However, consistency also has value, and since this test script is\n> filled with this sort of stylized \"expect\" setup already, this may be\n> fine, and probably not worth a re-roll. (A \"modernization\" patch by\n> someone can come later if warranted.)\n>\n>> +test_expect_success 'set up editor' '\n>> + write_script fakeeditor <<-\\EOF\n>> + sed -e \"s/A message/An edited message/g\" <\"$1\" >\"$1-\"\n>> + mv \"$1-\" \"$1\"\n>> + EOF\n>> +'\n\nIt's probably worth doing a whole cleanup of these (and switch to write script) in a dedicated patch series.\n\nNicolas\n"},{"id":"338090","messageId":"CAPig+cTDHsBSPZ+o+jh9bDvJ7NcZ3DGe+penppPwyupCJzmhAA@mail.gmail.com","threadId":"47737","inReplyTo":"fa3f512a-bd77-80c7-4fec-071639f62d26@suse.com","subject":"Re: [PATCHv2] tag: add --edit option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T09:57:18Z","receivedAt":"2018-02-02T09:57:24Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin\n wrote:\n> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :\n>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin\n>> wrote:\n>>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails\n>> Typically, it's easier on Junio, from a patch management standpoint,\n>> if you submit all these related patches as a single series.\n>> Alternately, if you do want to submit those changes separately, before\n>> the current patch lands in \"master\", be sure to mention atop which\n>> patch (this one) the additional patch(es) should live. Thanks.\n>\n> Well this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.\n\nYup, that's a reasonable way to look at it. I see them as related\n(thus a potential patch series) simply because the existing error\nmessage in tag.c is fine, as is, until the --edit option is\nintroduced, after which it becomes a bit iffy. Not a big deal, though.\n\n> Plus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),\n\nAs long as you just change the content of double-quoted string, you\nshouldn't have to worry about localization. The localization folks\nwill handle the .po files and whatnot.\n"},{"id":"338100","messageId":"52737deb-a5dc-27d6-3c0c-0d8b8de991c5@suse.com","threadId":"47737","inReplyTo":"CAPig+cTDHsBSPZ+o+jh9bDvJ7NcZ3DGe+penppPwyupCJzmhAA@mail.gmail.com","subject":"Re: [PATCHv2] tag: add --edit option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2018-02-02T16:48:29Z","receivedAt":"2018-02-02T16:48:37Z","isPatch":false,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 02/02/2018 à 10:57, Eric Sunshine a écrit :\n> On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin\n> wrote:\n>> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :\n>>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin\n>>> wrote:\n>>>> - I'll post another series to fix the misleading messages in both commit.c and tag.c when launch_editor fails\n>>> Typically, it's easier on Junio, from a patch management standpoint,\n>>> if you submit all these related patches as a single series.\n>>> Alternately, if you do want to submit those changes separately, before\n>>> the current patch lands in \"master\", be sure to mention atop which\n>>> patch (this one) the additional patch(es) should live. Thanks.\n>> Well this patch does not touch any of the line concerned by fixing the error message. So both should be able to land in any order.\n> Yup, that's a reasonable way to look at it. I see them as related\n> (thus a potential patch series) simply because the existing error\n> message in tag.c is fine, as is, until the --edit option is\n> introduced, after which it becomes a bit iffy. Not a big deal, though.\n\nOK ok I'll do it :)\nWhat message do you suggest ?\nAs I said in a previous mail, a simple \"Editor failure, cancelling {commit, tag}\" should be enough as launch_editor already outputs error messages describing what issue the editor had.\n\nI don't think suggesting moving to --no-edit || -m || -F is that helpful.\nIt's basically saying your setup is broken, but you can workaround by setting those options (and not saying that you're going to have some more issues later one).\n\n>> Plus I've never had to look into localization yet so I'm going to screw up on the first few submissions (not counting on people that disagree or would prefer another message),\n> As long as you just change the content of double-quoted string, you\n> shouldn't have to worry about localization. The localization folks\n> will handle the .po files and whatnot.\n\nSounds easy enough :)\n\nNicolas\n"},{"id":"338111","messageId":"CAPig+cQF2HzmtVdHqtQcOf0B-yA8Kpj-CZbPmdQotGwtdYpxvw@mail.gmail.com","threadId":"47737","inReplyTo":"52737deb-a5dc-27d6-3c0c-0d8b8de991c5@suse.com","subject":"Re: [PATCHv2] tag: add --edit option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-02T19:16:04Z","receivedAt":"2018-02-02T19:16:10Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin\n wrote:\n> What message do you suggest ? As I said in a previous mail, a\n> simple \"Editor failure, cancelling {commit, tag}\" should be enough\n> as launch_editor already outputs error messages describing what\n> issue the editor had.\n>\n> I don't think suggesting moving to --no-edit || -m || -F is that\n> helpful. It's basically saying your setup is broken, but you can\n> workaround by setting those options (and not saying that you're\n> going to have some more issues later one).\n\nIf it's the case the launch_editor() indeed outputs an appropriate\nerror message, then the existing error message from tag.c is already\nappropriate when --edit is not specified. It's only the --edit case\nthat the tag.c's additional message is somewhat weird. And, in fact,\nsuppressing tag.c's message might be the correct thing to do in the\n--edit case:\n\n static void create_tag(...) {\n ...\nif (launch_editor(...)) {\n if (!opt->use_editor)\n fprintf(stderr, _(\"... use either -m or -F ...\"));\n exit(1);\n}\n\nI don't feel strongly about it either way and am fine with just\npunting on the issue until someone actually complains about it.\n"},{"id":"338167","messageId":"670db556-59f4-8236-be2f-e0c901e8df68@suse.com","threadId":"47737","inReplyTo":"CAPig+cQF2HzmtVdHqtQcOf0B-yA8Kpj-CZbPmdQotGwtdYpxvw@mail.gmail.com","subject":"Re: [PATCHv2] tag: add --edit option","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nmoreychaisemartin@suse.com","sentAt":"2018-02-04T15:57:12Z","receivedAt":"2018-02-04T15:57:22Z","isPatch":false,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 02/02/2018 à 20:16, Eric Sunshine a écrit :\n> On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin\n> wrote:\n>> What message do you suggest ? As I said in a previous mail, a\n>> simple \"Editor failure, cancelling {commit, tag}\" should be enough\n>> as launch_editor already outputs error messages describing what\n>> issue the editor had.\n>>\n>> I don't think suggesting moving to --no-edit || -m || -F is that\n>> helpful. It's basically saying your setup is broken, but you can\n>> workaround by setting those options (and not saying that you're\n>> going to have some more issues later one).\n> If it's the case the launch_editor() indeed outputs an appropriate\n> error message, then the existing error message from tag.c is already\n> appropriate when --edit is not specified.\n\nI don't fully agree with the current message. The right thing to do is to fix the editor, not to hide the issue.\nA better message would be \"Editor failed. Fix it, or supply the message using either...\"\nAt least we suggest the right way to do it first.\n\n> It's only the --edit case\n> that the tag.c's additional message is somewhat weird. And, in fact,\n> suppressing tag.c's message might be the correct thing to do in the\n> --edit case:\n>\n> static void create_tag(...) {\n> ...\n> if (launch_editor(...)) {\n> if (!opt->use_editor)\n> fprintf(stderr, _(\"... use either -m or -F ...\"));\n> exit(1);\n> }\n>\n> I don't feel strongly about it either way and am fine with just\n> punting on the issue until someone actually complains about it.\n\nThe test should be opt->message_given && opt->use_editor.\nIf just --edit is provided but no -m/-F, --edit does not have any effect and it should be the same error message as when no option is given.\n\nNicolas\n"}]}