{"thread":{"id":"51997","subject":"[RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags","startedAt":"2019-10-08T18:47:47Z","lastAt":"2019-10-10T04:29:57Z","messageCount":12,"participants":["Lucas Oshiro","Matheus Tavares Bernardino","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"383698","messageId":"20191008184727.14337-1-lucasseikioshiro@gmail.com","threadId":"51997","inReplyTo":null,"subject":"[RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags","fromName":"Lucas Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2019-10-08T18:47:24Z","receivedAt":"2019-10-08T18:47:47Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"This series of patches fixes the flags --edit and --no-edit. Currently,\n--no-edit has no effect.\n\nThese patches implement the following behaviour:\n\n- when --edit is provided, the editor will always be opened;\n\n- when --no-edit is provided, the editor will not be opened (if possible),\n  otherwise an error message will be displayed;\n\n- when neither --edit nor --no-edit are provided, the editor is opened only if\n  a message is not provided and there isn't a previous tag message.\n\nIn the future, the fix of these flags and the code factoring done in this\npatchset will be used on the implementation of a new flag --amend, as discussed\non the mail thread started on\nhttps://public-inbox.org/git/CAHd499BM6M+=zRE1WFVXr7b+VhJHFeDind5xLqXcwZLv7QeDvw@mail.gmail.com/.\n\nLucas Oshiro (3):\n  tag: factor out tag reading from write_tag_body()\n  tag: factor out prepare tag template code\n  tag: add full support for --edit and --no-edit\n\n builtin/tag.c  | 123 ++++++++++++++++++++++++++++++++++---------------\n t/t7004-tag.sh |   4 +-\n 2 files changed, 88 insertions(+), 39 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"383699","messageId":"20191008184727.14337-2-lucasseikioshiro@gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-1-lucasseikioshiro@gmail.com","subject":"[RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()","fromName":"Lucas Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2019-10-08T18:47:25Z","receivedAt":"2019-10-08T18:47:47Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Improve code readability by moving tag reading to a new function called\nget_tag_body, which will be used in the implementation of the git tag\n--amend functionality.\n\nAdd warning in write_tag_body() in case the tag body is not successfully\nrecovered.\n\nCo-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\nSigned-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\nSigned-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\nHelped-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/tag.c | 42 ++++++++++++++++++++++++++++++++++--------\n 1 file changed, 34 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e0a4c25382..e1e3549af9 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -170,26 +170,52 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \treturn git_color_default_config(var, value, cb);\n }\n \n-static void write_tag_body(int fd, const struct object_id *oid)\n+/* \n+ * Returns the tag body of the given oid or NULL, in case of error. If size is\n+ * not NULL it is assigned the body size in bytes (excluding the '\\0').\n+ */\n+static char *get_tag_body(const struct object_id *oid, size_t *size) \n {\n-\tunsigned long size;\n+\tunsigned long buf_size;\n \tenum object_type type;\n-\tchar *buf, *sp;\n+\tchar *buf, *sp, *tag_body;\n+\tsize_t tag_body_size, signature_offset;\n \n-\tbuf = read_object_file(oid, &type, &size);\n+\tbuf = read_object_file(oid, &type, &buf_size);\n \tif (!buf)\n-\t\treturn;\n+\t\treturn NULL;\n \t/* skip header */\n \tsp = strstr(buf, \"\\n\\n\");\n \n-\tif (!sp || !size || type != OBJ_TAG) {\n+\tif (!sp || !buf_size || type != OBJ_TAG) {\n \t\tfree(buf);\n-\t\treturn;\n+\t\treturn NULL;\n \t}\n \tsp += 2; /* skip the 2 LFs */\n-\twrite_or_die(fd, sp, parse_signature(sp, buf + size - sp));\n+\tsignature_offset = parse_signature(sp, buf + buf_size - sp);\n+\tsp[signature_offset] = '\\0';\n \n+\t/* detach sp from buf */\n+\ttag_body_size = strlen(sp) + 1;\n+\ttag_body = xmalloc(tag_body_size);\n+\txsnprintf(tag_body, tag_body_size, \"%s\", sp);\n \tfree(buf);\n+\tif (size)\n+\t\t*size = tag_body_size - 1; /* exclude '\\0' */\n+\treturn tag_body;\n+}\n+\n+static void write_tag_body(int fd, const struct object_id *oid)\n+{\n+\tsize_t size;\n+\tconst char *tag_body = get_tag_body(oid, &size);\n+\n+\tif (!tag_body) {\n+\t\twarning(\"failed to get tag body for %s\", oid->hash);\n+\t\treturn;\n+\t}\n+\tprintf(\"tag_body: <%s>\\n\", tag_body);\n+\twrite_or_die(fd, tag_body, size);\n }\n \n static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)\n-- \n2.23.0\n\n"},{"id":"383700","messageId":"20191008184727.14337-3-lucasseikioshiro@gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-1-lucasseikioshiro@gmail.com","subject":"[RFC WIP PATCH 2/3] tag: factor out prepare tag template code","fromName":"Lucas Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2019-10-08T18:47:26Z","receivedAt":"2019-10-08T18:47:50Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"Improve code readability by moving tag body reading to a new function called\nget_tag_body. This function will be used in the following patches to fix the\n--no-edit flag.\n\nEnhance legibility by encapsulating code that loads previous tag message\n(if any) in new function prepare_tag_template. This code refactoring is\npart of a series of commits that intend to implement the git tag --amend\nflag and fix the functionality of --no-edit.\n\nCo-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\nHelped-by: Matheus Tavares <matheus.bernardino@usp.br>\nSigned-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\nSigned-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n---\n builtin/tag.c | 65 ++++++++++++++++++++++++++++++---------------------\n 1 file changed, 39 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e1e3549af9..0322bdbdfb 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -244,6 +244,43 @@ static const char message_advice_nested_tag[] =\n \t   \"\\n\"\n \t   \"\\tgit tag -f %s %s^{}\");\n \n+/*\n+ * Write the tag template message with previous tag body (if any) to the given\n+ * file.\n+ */\n+static void prepare_tag_template(struct strbuf *given_msg,\n+\t\t\t\t struct create_tag_options *opt,\n+\t\t\t\t struct object_id *prev, char *path,\n+\t\t\t\t const char *tag)\n+{\n+       int fd;\n+\n+\tfd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"could not create file '%s'\"), path);\n+\n+\tif (opt->message_given) {\n+\t\twrite_or_die(fd, given_msg->buf, given_msg->len);\n+\t\tstrbuf_reset(given_msg);\n+\t} else if (!is_null_oid(prev)) {\n+\t\twrite_tag_body(fd, prev);\n+\t} else {\n+\t\tstruct strbuf template = STRBUF_INIT;\n+\t\tstrbuf_addch(&template, '\\n');\n+\t\tif (opt->cleanup_mode == CLEANUP_ALL) {\n+\t\t\tstrbuf_commented_addf(&template, _(tag_template), tag,\n+\t\t\t\t\t      comment_line_char);\n+\t\t} else {\n+\t\t\tstrbuf_commented_addf(&template,\n+\t\t\t\t\t      _(tag_template_nocleanup), tag,\n+\t\t\t\t\t      comment_line_char);\n+\t\t}\n+\t\twrite_or_die(fd, template.buf, template.len);\n+\t\tstrbuf_release(&template);\n+\t}\n+\tclose(fd);\n+}\n+\n static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t       const char *tag,\n \t\t       struct strbuf *buf, struct create_tag_options *opt,\n@@ -251,7 +288,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n {\n \tenum object_type type;\n \tstruct strbuf header = STRBUF_INIT;\n-\tchar *path = NULL;\n+\tchar *path = git_pathdup(\"TAG_EDITMSG\");\n \n \ttype = oid_object_info(the_repository, object, NULL);\n \tif (type <= OBJ_NONE)\n@@ -271,31 +308,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t    git_committer_info(IDENT_STRICT));\n \n \tif (!opt->message_given || opt->use_editor) {\n-\t\tint fd;\n-\n-\t\t/* write the template message before editing: */\n-\t\tpath = git_pathdup(\"TAG_EDITMSG\");\n-\t\tfd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"could not create file '%s'\"), path);\n-\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-\t\t\tstrbuf_addch(&buf, '\\n');\n-\t\t\tif (opt->cleanup_mode == CLEANUP_ALL)\n-\t\t\t\tstrbuf_commented_addf(&buf, _(tag_template), tag, comment_line_char);\n-\t\t\telse\n-\t\t\t\tstrbuf_commented_addf(&buf, _(tag_template_nocleanup), tag, comment_line_char);\n-\t\t\twrite_or_die(fd, buf.buf, buf.len);\n-\t\t\tstrbuf_release(&buf);\n-\t\t}\n-\t\tclose(fd);\n-\n+\t\tprepare_tag_template(buf, opt, prev, path, tag);\n \t\tif (launch_editor(path, buf, NULL)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n-- \n2.23.0\n\n"},{"id":"383701","messageId":"20191008184727.14337-4-lucasseikioshiro@gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-1-lucasseikioshiro@gmail.com","subject":"[RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit","fromName":"Lucas Oshiro","fromEmail":"lucasseikioshiro@gmail.com","sentAt":"2019-10-08T18:47:27Z","receivedAt":"2019-10-08T18:47:53Z","isPatch":true,"sender":{"key":"lucasseikioshiro@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701580?v=4"},"body":"git tag --edit and --no-edit flags are not currently fully supported.\nThis patch fixes the functionality that allows the editor to be opened\non demand.\n\nCo-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\nSigned-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\nSigned-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n---\n builtin/tag.c  | 16 +++++++++++++---\n t/t7004-tag.sh |  4 ++--\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 0322bdbdfb..7dff61d45a 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -230,6 +230,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu\n struct create_tag_options {\n \tunsigned int message_given:1;\n \tunsigned int use_editor:1;\n+\tunsigned int force_editor:1;\n \tunsigned int sign;\n \tenum {\n \t\tCLEANUP_NONE,\n@@ -307,13 +308,21 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t    tag,\n \t\t    git_committer_info(IDENT_STRICT));\n \n-\tif (!opt->message_given || opt->use_editor) {\n+\tif (opt->force_editor && !opt->message_given && is_null_oid(prev) &&\n+\t    !opt->use_editor) {\n+\t\tdie(_(\"no tag message?\"));\n+\t} else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))\n+\t\t  || (opt->force_editor && opt->use_editor)) {\n+\t\t/* Editor must be opened */\n \t\tprepare_tag_template(buf, opt, prev, path, tag);\n \t\tif (launch_editor(path, buf, NULL)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\n \t\t}\n+\t} else if (!opt->message_given) {\n+\t\t/* Tag already exists and user doesn't want to change it */\n+\t\tstrbuf_addstr(buf, get_tag_body(prev, NULL));\n \t}\n \n \tif (opt->cleanup_mode != CLEANUP_NONE)\n@@ -436,7 +445,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+\tint edit_flag = -1;\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@@ -592,7 +601,8 @@ 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+\topt.force_editor = edit_flag >= 0;\n+\topt.use_editor = opt.force_editor ? edit_flag : 0;\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 80eb13d94e..bf43d2c750 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \\\n \t'reediting a signed tag body omits signature' '\n \techo \"rfc1991\" >gpghome/gpg.conf &&\n \techo \"RFC1991 signed tag\" >expect &&\n-\tGIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n+\tGIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n \ttest_cmp expect actual\n '\n \n@@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \\\n test_expect_success GPG,RFC1991 \\\n \t'reediting a signed tag body omits signature' '\n \techo \"RFC1991 signed tag\" >expect &&\n-\tGIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n+\tGIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n \ttest_cmp expect actual\n '\n \n-- \n2.23.0\n\n"},{"id":"383724","messageId":"CAHd-oW7HXzvjxzV=79yRAMHc=msB2rWmWCVEYuB4N1LnOMwDyQ@mail.gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-2-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-09T01:48:43Z","receivedAt":"2019-10-09T01:48:57Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:\n>\n> Improve code readability by moving tag reading to a new function called\n> get_tag_body, which will be used in the implementation of the git tag\n> --amend functionality.\n\nThis function is also used in your third patch, to fix git-tag's\n--no-edit implementation, right? Maybe you could mention this here,\ninstead of referencing the --amend feature, as the former is already\npresent in this series.\n\n> Add warning in write_tag_body() in case the tag body is not successfully\n> recovered.\n>\n> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\n> Helped-by: Matheus Tavares <matheus.bernardino@usp.br>\n\nThese tags usually come in chronological order. So I think the\nHelped-by should come first as I helped you, then you and Bárbara\nco-authored the patch and then, lastly, you sent it.\n\n> ---\n>  builtin/tag.c | 42 ++++++++++++++++++++++++++++++++++--------\n>  1 file changed, 34 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index e0a4c25382..e1e3549af9 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -170,26 +170,52 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n>         return git_color_default_config(var, value, cb);\n>  }\n>\n> -static void write_tag_body(int fd, const struct object_id *oid)\n> +/*\n> + * Returns the tag body of the given oid or NULL, in case of error. If size is\n> + * not NULL it is assigned the body size in bytes (excluding the '\\0').\n> + */\n> +static char *get_tag_body(const struct object_id *oid, size_t *size)\n>  {\n> -       unsigned long size;\n> +       unsigned long buf_size;\n>         enum object_type type;\n> -       char *buf, *sp;\n> +       char *buf, *sp, *tag_body;\n> +       size_t tag_body_size, signature_offset;\n>\n> -       buf = read_object_file(oid, &type, &size);\n> +       buf = read_object_file(oid, &type, &buf_size);\n>         if (!buf)\n> -               return;\n> +               return NULL;\n>         /* skip header */\n>         sp = strstr(buf, \"\\n\\n\");\n>\n> -       if (!sp || !size || type != OBJ_TAG) {\n> +       if (!sp || !buf_size || type != OBJ_TAG) {\n>                 free(buf);\n> -               return;\n> +               return NULL;\n>         }\n>         sp += 2; /* skip the 2 LFs */\n> -       write_or_die(fd, sp, parse_signature(sp, buf + size - sp));\n> +       signature_offset = parse_signature(sp, buf + buf_size - sp);\n> +       sp[signature_offset] = '\\0';\n>\n> +       /* detach sp from buf */\n> +       tag_body_size = strlen(sp) + 1;\n> +       tag_body = xmalloc(tag_body_size);\n> +       xsnprintf(tag_body, tag_body_size, \"%s\", sp);\n>         free(buf);\n> +       if (size)\n> +               *size = tag_body_size - 1; /* exclude '\\0' */\n> +       return tag_body;\n> +}\n> +\n> +static void write_tag_body(int fd, const struct object_id *oid)\n> +{\n> +       size_t size;\n> +       const char *tag_body = get_tag_body(oid, &size);\n> +\n> +       if (!tag_body) {\n> +               warning(\"failed to get tag body for %s\", oid->hash);\n> +               return;\n> +       }\n> +       printf(\"tag_body: <%s>\\n\", tag_body);\n\nHm, is this addition right or perhaps was it added in the patch by mistake?\n\n> +       write_or_die(fd, tag_body, size);\n>  }\n>\n>  static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)\n> --\n> 2.23.0\n>\n"},{"id":"383729","messageId":"CAHd-oW6mK1ZSCu-igT5PSLop6gDEtKmrsg6sBsGij2NMxgy25g@mail.gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-3-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-09T03:02:08Z","receivedAt":"2019-10-09T03:02:44Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:\n>\n> Improve code readability by moving tag body reading to a new function called\n> get_tag_body. This function will be used in the following patches to fix the\n> --no-edit flag.\n\nThis seems to be accidentally duplicated from patch 1/3.\n\n> Enhance legibility by encapsulating code that loads previous tag message\n> (if any) in new function prepare_tag_template. This code refactoring is\n> part of a series of commits that intend to implement the git tag --amend\n> flag and fix the functionality of --no-edit.\n>\n> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n> Helped-by: Matheus Tavares <matheus.bernardino@usp.br>\n> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\n> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n\nThese tags can be re-ordered as I mentioned in patch 1/3, to follow a\nchronological order: probably the Helped-by first followed by the\nCo-authored-by, Barbara's S-o-B and then your S-o-B.\n\n> ---\n>  builtin/tag.c | 65 ++++++++++++++++++++++++++++++---------------------\n>  1 file changed, 39 insertions(+), 26 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index e1e3549af9..0322bdbdfb 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -244,6 +244,43 @@ static const char message_advice_nested_tag[] =\n>            \"\\n\"\n>            \"\\tgit tag -f %s %s^{}\");\n>\n> +/*\n> + * Write the tag template message with previous tag body (if any) to the given\n> + * file.\n> + */\n\nMaybe mention that the function creates the file at the given path?\n\n> +static void prepare_tag_template(struct strbuf *given_msg,\n> +                                struct create_tag_options *opt,\n> +                                struct object_id *prev, char *path,\n> +                                const char *tag)\n\nI'm wondering if we could simplify this signature. Maybe we could\nresolve whether a message was given at CLI, and if a previous tag\nalready exists, before getting to this function. This way, 'given_msg'\nand 'prev' could be collapsed into a single 'struct strbuf *tag_body'\n(and we could replace checking 'opt->message_given' and\n'is_null_oid(prev)' by checking if 'tag_body' is not NULL). Then, we\ncould also pass just the 'cleanup_mode\" instead of the whole\n'create_tag_options'. Does this makes sense?\n\n> +{\n> +       int fd;\n> +\n> +       fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> +       if (fd < 0)\n> +               die_errno(_(\"could not create file '%s'\"), path);\n> +\n> +       if (opt->message_given) {\n> +               write_or_die(fd, given_msg->buf, given_msg->len);\n> +               strbuf_reset(given_msg);\n\nI think keeping this reset at create_tag() (right before\nlauch_editor() is called and only if 'opt->message_given') makes it\neasier to understand what's happening (because there, we are cleaning\nthe given 'buf' to use it in lauch_editor()). Calling it here may be\nmisleading as 'given_msg' is not used in this function anymore.\n\n> +       } else if (!is_null_oid(prev)) {\n> +               write_tag_body(fd, prev);\n> +       } else {\n> +               struct strbuf template = STRBUF_INIT;\n> +               strbuf_addch(&template, '\\n');\n> +               if (opt->cleanup_mode == CLEANUP_ALL) {\n> +                       strbuf_commented_addf(&template, _(tag_template), tag,\n> +                                             comment_line_char);\n> +               } else {\n> +                       strbuf_commented_addf(&template,\n> +                                             _(tag_template_nocleanup), tag,\n> +                                             comment_line_char);\n> +               }\n> +               write_or_die(fd, template.buf, template.len);\n> +               strbuf_release(&template);\n> +       }\n> +       close(fd);\n> +}\n> +\n>  static void create_tag(const struct object_id *object, const char *object_ref,\n>                        const char *tag,\n>                        struct strbuf *buf, struct create_tag_options *opt,\n> @@ -251,7 +288,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n>  {\n>         enum object_type type;\n>         struct strbuf header = STRBUF_INIT;\n> -       char *path = NULL;\n> +       char *path = git_pathdup(\"TAG_EDITMSG\");\n>\n>         type = oid_object_info(the_repository, object, NULL);\n>         if (type <= OBJ_NONE)\n> @@ -271,31 +308,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n>                     git_committer_info(IDENT_STRICT));\n>\n>         if (!opt->message_given || opt->use_editor) {\n> -               int fd;\n> -\n> -               /* write the template message before editing: */\n> -               path = git_pathdup(\"TAG_EDITMSG\");\n> -               fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> -               if (fd < 0)\n> -                       die_errno(_(\"could not create file '%s'\"), path);\n> -\n> -               if (opt->message_given) {\n> -                       write_or_die(fd, buf->buf, buf->len);\n> -                       strbuf_reset(buf);\n> -               } else if (!is_null_oid(prev)) {\n> -                       write_tag_body(fd, prev);\n> -               } else {\n> -                       struct strbuf buf = STRBUF_INIT;\n> -                       strbuf_addch(&buf, '\\n');\n> -                       if (opt->cleanup_mode == CLEANUP_ALL)\n> -                               strbuf_commented_addf(&buf, _(tag_template), tag, comment_line_char);\n> -                       else\n> -                               strbuf_commented_addf(&buf, _(tag_template_nocleanup), tag, comment_line_char);\n> -                       write_or_die(fd, buf.buf, buf.len);\n> -                       strbuf_release(&buf);\n> -               }\n> -               close(fd);\n> -\n> +               prepare_tag_template(buf, opt, prev, path, tag);\n>                 if (launch_editor(path, buf, NULL)) {\n>                         fprintf(stderr,\n>                         _(\"Please supply the message using either -m or -F option.\\n\"));\n> --\n> 2.23.0\n>\n"},{"id":"383739","messageId":"CAHd-oW4RrzhhiCSbfVYFdCGuCrsi=cJfpZJGMBqU53oPDk1QgA@mail.gmail.com","threadId":"51997","inReplyTo":"20191008184727.14337-4-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-09T09:19:02Z","receivedAt":"2019-10-09T09:19:17Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:\n>\n> git tag --edit and --no-edit flags are not currently fully supported.\n> This patch fixes the functionality that allows the editor to be opened\n> on demand.\n>\n> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>\n> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>\n> ---\n>  builtin/tag.c  | 16 +++++++++++++---\n>  t/t7004-tag.sh |  4 ++--\n>  2 files changed, 15 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 0322bdbdfb..7dff61d45a 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -230,6 +230,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu\n>  struct create_tag_options {\n>         unsigned int message_given:1;\n>         unsigned int use_editor:1;\n> +       unsigned int force_editor:1;\n\nWhat if we turn 'use_editor' into a tri-state variable (--edit,\n--no-edit and \"nothing given\") instead of adding this field? Maybe it\nwould simplify some condition checks at create_tag().\n\n>         unsigned int sign;\n>         enum {\n>                 CLEANUP_NONE,\n> @@ -307,13 +308,21 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n>                     tag,\n>                     git_committer_info(IDENT_STRICT));\n>\n> -       if (!opt->message_given || opt->use_editor) {\n> +       if (opt->force_editor && !opt->message_given && is_null_oid(prev) &&\n> +           !opt->use_editor) {\n> +               die(_(\"no tag message?\"));\n> +       } else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))\n> +                 || (opt->force_editor && opt->use_editor)) {\n> +               /* Editor must be opened */\n>                 prepare_tag_template(buf, opt, prev, path, tag);\n>                 if (launch_editor(path, buf, NULL)) {\n>                         fprintf(stderr,\n>                         _(\"Please supply the message using either -m or -F option.\\n\"));\n>                         exit(1);\n>                 }\n> +       } else if (!opt->message_given) {\n> +               /* Tag already exists and user doesn't want to change it */\n> +               strbuf_addstr(buf, get_tag_body(prev, NULL));\n>         }\n>\n>         if (opt->cleanup_mode != CLEANUP_NONE)\n> @@ -436,7 +445,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>         static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n>         struct ref_format format = REF_FORMAT_INIT;\n>         int icase = 0;\n> -       int edit_flag = 0;\n> +       int edit_flag = -1;\n>         struct option options[] = {\n>                 OPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list tag names\"), 'l'),\n>                 { OPTION_INTEGER, 'n', NULL, &filter.lines, N_(\"n\"),\n> @@ -592,7 +601,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>                 die(_(\"tag '%s' already exists\"), tag);\n>\n>         opt.message_given = msg.given || msgfile;\n> -       opt.use_editor = edit_flag;\n> +       opt.force_editor = edit_flag >= 0;\n> +       opt.use_editor = opt.force_editor ? edit_flag : 0;\n>\n>         if (!cleanup_arg || !strcmp(cleanup_arg, \"strip\"))\n>                 opt.cleanup_mode = CLEANUP_ALL;\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 80eb13d94e..bf43d2c750 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \\\n>         'reediting a signed tag body omits signature' '\n>         echo \"rfc1991\" >gpghome/gpg.conf &&\n>         echo \"RFC1991 signed tag\" >expect &&\n> -       GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n> +       GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n>         test_cmp expect actual\n>  '\n>\n> @@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \\\n>  test_expect_success GPG,RFC1991 \\\n>         'reediting a signed tag body omits signature' '\n>         echo \"RFC1991 signed tag\" >expect &&\n> -       GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n> +       GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n>         test_cmp expect actual\n>  '\n>\n> --\n> 2.23.0\n>\n"},{"id":"383777","messageId":"xmqqwoddjqk9.fsf@gitster-ct.c.googlers.com","threadId":"51997","inReplyTo":"20191008184727.14337-1-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T02:13:26Z","receivedAt":"2019-10-10T02:21:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Oshiro <lucasseikioshiro@gmail.com> writes:\n\n> This series of patches fixes the flags --edit and --no-edit. Currently,\n> --no-edit has no effect.\n>\n> These patches implement the following behaviour:\n>\n> - when --edit is provided, the editor will always be opened;\n>\n> - when --no-edit is provided, the editor will not be opened (if possible),\n>   otherwise an error message will be displayed;\n>\n> - when neither --edit nor --no-edit are provided, the editor is opened only if\n>   a message is not provided and there isn't a previous tag message.\n\nAnother thing that must be done is\n\n - when both --edit and --no-edit are given, the last one wins.\n\nI do not know if you have implemented it in the three patches, though.\n\n> In the future, the fix of these flags and the code factoring done in this\n> patchset will be used on the implementation of a new flag --amend, as discussed\n> on the mail thread started on\n> https://public-inbox.org/git/CAHd499BM6M+=zRE1WFVXr7b+VhJHFeDind5xLqXcwZLv7QeDvw@mail.gmail.com/.\n>\n> Lucas Oshiro (3):\n>   tag: factor out tag reading from write_tag_body()\n>   tag: factor out prepare tag template code\n>   tag: add full support for --edit and --no-edit\n>\n>  builtin/tag.c  | 123 ++++++++++++++++++++++++++++++++++---------------\n>  t/t7004-tag.sh |   4 +-\n>  2 files changed, 88 insertions(+), 39 deletions(-)\n"},{"id":"383778","messageId":"xmqqsgo1jp8h.fsf@gitster-ct.c.googlers.com","threadId":"51997","inReplyTo":"20191008184727.14337-2-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T02:42:06Z","receivedAt":"2019-10-10T02:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Oshiro <lucasseikioshiro@gmail.com> writes:\n\n> +/* \n> + * Returns the tag body of the given oid or NULL, in case of error. If size is\n> + * not NULL it is assigned the body size in bytes (excluding the '\\0').\n> + */\n> +static char *get_tag_body(const struct object_id *oid, size_t *size) \n>  {\n> +\tunsigned long buf_size;\n>  \tenum object_type type;\n> +\tchar *buf, *sp, *tag_body;\n> +\tsize_t tag_body_size, signature_offset;\n>  \n> +\tbuf = read_object_file(oid, &type, &buf_size);\n>  \tif (!buf)\n> +\t\treturn NULL;\n>  \t/* skip header */\n>  \tsp = strstr(buf, \"\\n\\n\");\n>  \n> +\tif (!sp || !buf_size || type != OBJ_TAG) {\n>  \t\tfree(buf);\n> +\t\treturn NULL;\n>  \t}\n\nReturning early when !buf_size before even attempting to strstr\nwould be cleaner to read, i.e.\n\n\tbuf = read_object_file(...);\n\tif (!buf || !buf_size) {\n\t\tfree(buf);\n\t\treturn NULL;\n\t}\n\tbody = strstr(buf, \"\\n\\n\");\n\nFWIW, the type check that is done after this point could also be a\npart of the early return, as there is no point scanning for the end\nof object header part if the object is not a tag (e.g. if it were a\nblob, there is no \"header part\" and scanning for a blank line is\nmeaningless).\n\t\n>  \tsp += 2; /* skip the 2 LFs */\n> +\tsignature_offset = parse_signature(sp, buf + buf_size - sp);\n> +\tsp[signature_offset] = '\\0';\n>  \n> +\t/* detach sp from buf */\n> +\ttag_body_size = strlen(sp) + 1;\n> +\ttag_body = xmalloc(tag_body_size);\n> +\txsnprintf(tag_body, tag_body_size, \"%s\", sp);\n\nIsn't this essentially\n\n\ttag_body = xstrdup(sp);\n        tag_body_size = signature_offset;\n\n(my arith may be off by one or two, but does a separate\ntag_body_size need to exist?)\n\n>  \tfree(buf);\n> +\tif (size)\n> +\t\t*size = tag_body_size - 1; /* exclude '\\0' */\n> +\treturn tag_body;\n> +}\n> +\n> +static void write_tag_body(int fd, const struct object_id *oid)\n> +{\n> +\tsize_t size;\n> +\tconst char *tag_body = get_tag_body(oid, &size);\n> +\n> +\tif (!tag_body) {\n> +\t\twarning(\"failed to get tag body for %s\", oid->hash);\n\nI do not think the original gives any such warning.\n\n - Do we want to be unconditionally noisy this way?\n - Should this be a fatal error?  If not, why?\n - Should the message be translatable?\n\nAs an interface, is it sensible to force any and all callers of\nget_tag_body() to supply a pointer to &size?  Is the returned value\nalways a NUL-terminated string?  I suspect that people would find it\na more natural interface if its were like:\n\n\tconst char *body = get_tag_body(oid);\n\n\tif (!body)\t\t\n\t\t...;\n\n\tif (this caller needs size) {\n\t\tsize_t body_size = strlen(body);\n\t\t... use both body and body_size ...\n\t\twrite_or_die(fd, body, body_size);\n\t} else {\n\t\t... just use body ...\n\t\tprintf(\"%s\", body);\n\t}\n\t\n> +\t\treturn;\n> +\t}\n> +\tprintf(\"tag_body: <%s>\\n\", tag_body);\n> +\twrite_or_die(fd, tag_body, size);\n\nWTH is this double writing?\n\n>  }\n>  \n>  static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)\n"},{"id":"383779","messageId":"xmqqo8ypjota.fsf@gitster-ct.c.googlers.com","threadId":"51997","inReplyTo":"20191008184727.14337-3-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T02:51:13Z","receivedAt":"2019-10-10T02:51:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Oshiro <lucasseikioshiro@gmail.com> writes:\n\n> Improve code readability by moving tag body reading to a new function called\n> get_tag_body.\n\nQuite honestly, I think the result of this splitting is harder to\nfollow than the original.\n\nFor example, the value of opt->message_given and the validity of\ngiven_msg is very closely related, so if you made the helper\nfunction receive non-NULL given_msg when !opt->message_given, the\nhelper could only check !given_msg without having to worry about\nopt->message_given; with such a change, I could buy that the split\nimproves code readability, but I do not see any such change in the\npatch.\n\n> Enhance legibility by encapsulating code that loads previous tag message\n> (if any) in new function prepare_tag_template....\n\nThe helper seems to be used to _write_ into path, and not load or\nread any message from either an object or a file on disk.\n"},{"id":"383780","messageId":"xmqqk19djmsr.fsf@gitster-ct.c.googlers.com","threadId":"51997","inReplyTo":"20191008184727.14337-4-lucasseikioshiro@gmail.com","subject":"Re: [RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T03:34:44Z","receivedAt":"2019-10-10T03:34:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas Oshiro <lucasseikioshiro@gmail.com> writes:\n\n>  struct create_tag_options {\n>  \tunsigned int message_given:1;\n>  \tunsigned int use_editor:1;\n> +\tunsigned int force_editor:1;\n>  \tunsigned int sign;\n\n> -\tif (!opt->message_given || opt->use_editor) {\n> +\tif (opt->force_editor && !opt->message_given && is_null_oid(prev) &&\n> +\t    !opt->use_editor) {\n> +\t\tdie(_(\"no tag message?\"));\n\nIf we didn't get a message from command line, and there is no\nprevious tag object to read the message from, there is nowhere but\neditor to grab the message to be used, so !use_editor would trigger\nan error if force_editor (i.e. the command line explicitly said\neither --edit or --no-edit).  Makes sense, but I needed to cheat and\nlook at cmd_tag() to see how \"force\" is set to understand what is\ngoing on.  I have to say \"force\" is not a good name for this field;\nis this use similar to what we typically use an additional _given\nfield?\n\n> +\t} else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))\n> +\t\t  || (opt->force_editor && opt->use_editor)) {\n> +\t\t/* Editor must be opened */\n\nIf there is no --[no-]edit and there is no preexisting message, we\nneed to use the editor.  If the command line explicitly said --edit,\nwe also would use the editor.  OK.\n\nBut it starts to make me wonder if you rather want to replace the\nsingle bit use_editor field with an enum with three possible values\n(enum { EDITOR_UNSPECIFIED, EDITOR_YES, EDITOR_NO } use_editor).\n\n>  \t\tprepare_tag_template(buf, opt, prev, path, tag);\n>  \t\tif (launch_editor(path, buf, NULL)) {\n>  \t\t\tfprintf(stderr,\n>  \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n>  \t\t\texit(1);\n>  \t\t}\n> +\t} else if (!opt->message_given) {\n> +\t\t/* Tag already exists and user doesn't want to change it */\n\nAre we certain at this point in if/else cascade that prev is a valid\ntag?  How?\n\n> +\t\tstrbuf_addstr(buf, get_tag_body(prev, NULL));\n\nThis NULL tells us something about what I mentioned in my review on 1/3.\n\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 80eb13d94e..bf43d2c750 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \\\n>  \t'reediting a signed tag body omits signature' '\n>  \techo \"rfc1991\" >gpghome/gpg.conf &&\n>  \techo \"RFC1991 signed tag\" >expect &&\n> -\tGIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n> +\tGIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n>  \ttest_cmp expect actual\n>  '\n>  \n> @@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \\\n>  test_expect_success GPG,RFC1991 \\\n>  \t'reediting a signed tag body omits signature' '\n>  \techo \"RFC1991 signed tag\" >expect &&\n> -\tGIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&\n> +\tGIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&\n>  \ttest_cmp expect actual\n>  '\n\nWhy do these two need explicit --edit option to invoke the editor?\nThat smells like an unnecessary backward incompatible change.\n\nThanks.\n"},{"id":"383781","messageId":"xmqqftk1jk90.fsf@gitster-ct.c.googlers.com","threadId":"51997","inReplyTo":"xmqqo8ypjota.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T04:29:47Z","receivedAt":"2019-10-10T04:29:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Lucas Oshiro <lucasseikioshiro@gmail.com> writes:\n>\n>> Improve code readability by moving tag body reading to a new function called\n>> get_tag_body.\n>\n> Quite honestly, I think the result of this splitting is harder to\n> follow than the original.\n>\n> For example, the value of opt->message_given and the validity of\n> given_msg is very closely related, so if you made the helper\n> function receive non-NULL given_msg when !opt->message_given, the\n> helper could only check !given_msg without having to worry about\n> opt->message_given; with such a change, I could buy that the split\n> improves code readability, but I do not see any such change in the\n> patch.\n\nJust to avoid misunderstanding, I am not suggesting to change the\ninterface into the helper you introduced to be like this\n\n\tprepare_tag_template(opt->message_given ? buf, NULL,\n\t\t\t     opt, prev, path, tag);\n\nThat still needs to pass the entire opt structure into the helper\nand forces the helper to look at opt->cleanup_mode and behave\ndifferently.  If a restructuring of the code can be done in such a\nway that the helper no longer need to look at opt (hence no need to\npass the entire opt structure into it), I can see that the change\nimproves the readability of the resulting code, but I am not all\nthat hopeful.\n\nOn the other hand (and this is the more important hand among the two\n;-), I do not mind splitting a helper function out of an existing\ncodepath and make the existing codepath call the new helper, so that\nthe same helper can be reused from another codepath later.  Not at\nall.  What I do mind is to mislabel a change that is done for such a\nlater code reuse as one that improves readability, when it does not.\n\nThanks.\n\n>> Enhance legibility by encapsulating code that loads previous tag message\n>> (if any) in new function prepare_tag_template....\n>\n> The helper seems to be used to _write_ into path, and not load or\n> read any message from either an object or a file on disk.\n"}]}