[RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags

[RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags

[Lucas Oshiro]

This series of patches fixes the flags --edit and --no-edit. Currently,
--no-edit has no effect.

These patches implement the following behaviour:

- when --edit is provided, the editor will always be opened;

- when --no-edit is provided, the editor will not be opened (if possible),
  otherwise an error message will be displayed;

- when neither --edit nor --no-edit are provided, the editor is opened only if
  a message is not provided and there isn't a previous tag message.

In the future, the fix of these flags and the code factoring done in this
patchset will be used on the implementation of a new flag --amend, as discussed
on the mail thread started on
https://public-inbox.org/git/CAHd499BM6M+=zRE1WFVXr7b+VhJHFeDind5xLqXcwZLv7QeDvw@mail.gmail.com/.

Lucas Oshiro (3):
  tag: factor out tag reading from write_tag_body()
  tag: factor out prepare tag template code
  tag: add full support for --edit and --no-edit

 builtin/tag.c  | 123 ++++++++++++++++++++++++++++++++++---------------
 t/t7004-tag.sh |   4 +-
 2 files changed, 88 insertions(+), 39 deletions(-)

-- 
2.23.0

[RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()

[Lucas Oshiro]

Improve code readability by moving tag reading to a new function called
get_tag_body, which will be used in the implementation of the git tag
--amend functionality.

Add warning in write_tag_body() in case the tag body is not successfully
recovered.

Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>
Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Helped-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/tag.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e0a4c25382..e1e3549af9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -170,26 +170,52 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	return git_color_default_config(var, value, cb);
 }
 
-static void write_tag_body(int fd, const struct object_id *oid)
+/* 
+ * Returns the tag body of the given oid or NULL, in case of error. If size is
+ * not NULL it is assigned the body size in bytes (excluding the '\0').
+ */
+static char *get_tag_body(const struct object_id *oid, size_t *size) 
 {
-	unsigned long size;
+	unsigned long buf_size;
 	enum object_type type;
-	char *buf, *sp;
+	char *buf, *sp, *tag_body;
+	size_t tag_body_size, signature_offset;
 
-	buf = read_object_file(oid, &type, &size);
+	buf = read_object_file(oid, &type, &buf_size);
 	if (!buf)
-		return;
+		return NULL;
 	/* skip header */
 	sp = strstr(buf, "\n\n");
 
-	if (!sp || !size || type != OBJ_TAG) {
+	if (!sp || !buf_size || type != OBJ_TAG) {
 		free(buf);
-		return;
+		return NULL;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	signature_offset = parse_signature(sp, buf + buf_size - sp);
+	sp[signature_offset] = '\0';
 
+	/* detach sp from buf */
+	tag_body_size = strlen(sp) + 1;
+	tag_body = xmalloc(tag_body_size);
+	xsnprintf(tag_body, tag_body_size, "%s", sp);
 	free(buf);
+	if (size)
+		*size = tag_body_size - 1; /* exclude '\0' */
+	return tag_body;
+}
+
+static void write_tag_body(int fd, const struct object_id *oid)
+{
+	size_t size;
+	const char *tag_body = get_tag_body(oid, &size);
+
+	if (!tag_body) {
+		warning("failed to get tag body for %s", oid->hash);
+		return;
+	}
+	printf("tag_body: <%s>\n", tag_body);
+	write_or_die(fd, tag_body, size);
 }
 
 static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
-- 
2.23.0

[RFC WIP PATCH 2/3] tag: factor out prepare tag template code

[Lucas Oshiro]

Improve code readability by moving tag body reading to a new function called
get_tag_body. This function will be used in the following patches to fix the
--no-edit flag.

Enhance legibility by encapsulating code that loads previous tag message
(if any) in new function prepare_tag_template. This code refactoring is
part of a series of commits that intend to implement the git tag --amend
flag and fix the functionality of --no-edit.

Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
Helped-by: Matheus Tavares <matheus.bernardino@usp.br>
Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>
---
 builtin/tag.c | 65 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e1e3549af9..0322bdbdfb 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -244,6 +244,43 @@ static const char message_advice_nested_tag[] =
 	   "\n"
 	   "\tgit tag -f %s %s^{}");
 
+/*
+ * Write the tag template message with previous tag body (if any) to the given
+ * file.
+ */
+static void prepare_tag_template(struct strbuf *given_msg,
+				 struct create_tag_options *opt,
+				 struct object_id *prev, char *path,
+				 const char *tag)
+{
+       int fd;
+
+	fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+	if (fd < 0)
+		die_errno(_("could not create file '%s'"), path);
+
+	if (opt->message_given) {
+		write_or_die(fd, given_msg->buf, given_msg->len);
+		strbuf_reset(given_msg);
+	} else if (!is_null_oid(prev)) {
+		write_tag_body(fd, prev);
+	} else {
+		struct strbuf template = STRBUF_INIT;
+		strbuf_addch(&template, '\n');
+		if (opt->cleanup_mode == CLEANUP_ALL) {
+			strbuf_commented_addf(&template, _(tag_template), tag,
+					      comment_line_char);
+		} else {
+			strbuf_commented_addf(&template,
+					      _(tag_template_nocleanup), tag,
+					      comment_line_char);
+		}
+		write_or_die(fd, template.buf, template.len);
+		strbuf_release(&template);
+	}
+	close(fd);
+}
+
 static void create_tag(const struct object_id *object, const char *object_ref,
 		       const char *tag,
 		       struct strbuf *buf, struct create_tag_options *opt,
@@ -251,7 +288,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 {
 	enum object_type type;
 	struct strbuf header = STRBUF_INIT;
-	char *path = NULL;
+	char *path = git_pathdup("TAG_EDITMSG");
 
 	type = oid_object_info(the_repository, object, NULL);
 	if (type <= OBJ_NONE)
@@ -271,31 +308,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 		    git_committer_info(IDENT_STRICT));
 
 	if (!opt->message_given || opt->use_editor) {
-		int fd;
-
-		/* write the template message before editing: */
-		path = git_pathdup("TAG_EDITMSG");
-		fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-		if (fd < 0)
-			die_errno(_("could not create file '%s'"), path);
-
-		if (opt->message_given) {
-			write_or_die(fd, buf->buf, buf->len);
-			strbuf_reset(buf);
-		} else if (!is_null_oid(prev)) {
-			write_tag_body(fd, prev);
-		} else {
-			struct strbuf buf = STRBUF_INIT;
-			strbuf_addch(&buf, '\n');
-			if (opt->cleanup_mode == CLEANUP_ALL)
-				strbuf_commented_addf(&buf, _(tag_template), tag, comment_line_char);
-			else
-				strbuf_commented_addf(&buf, _(tag_template_nocleanup), tag, comment_line_char);
-			write_or_die(fd, buf.buf, buf.len);
-			strbuf_release(&buf);
-		}
-		close(fd);
-
+		prepare_tag_template(buf, opt, prev, path, tag);
 		if (launch_editor(path, buf, NULL)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
-- 
2.23.0

[RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit

[Lucas Oshiro]

git tag --edit and --no-edit flags are not currently fully supported.
This patch fixes the functionality that allows the editor to be opened
on demand.

Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>
---
 builtin/tag.c  | 16 +++++++++++++---
 t/t7004-tag.sh |  4 ++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 0322bdbdfb..7dff61d45a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -230,6 +230,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu
 struct create_tag_options {
 	unsigned int message_given:1;
 	unsigned int use_editor:1;
+	unsigned int force_editor:1;
 	unsigned int sign;
 	enum {
 		CLEANUP_NONE,
@@ -307,13 +308,21 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 		    tag,
 		    git_committer_info(IDENT_STRICT));
 
-	if (!opt->message_given || opt->use_editor) {
+	if (opt->force_editor && !opt->message_given && is_null_oid(prev) &&
+	    !opt->use_editor) {
+		die(_("no tag message?"));
+	} else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))
+		  || (opt->force_editor && opt->use_editor)) {
+		/* Editor must be opened */
 		prepare_tag_template(buf, opt, prev, path, tag);
 		if (launch_editor(path, buf, NULL)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
 		}
+	} else if (!opt->message_given) {
+		/* Tag already exists and user doesn't want to change it */
+		strbuf_addstr(buf, get_tag_body(prev, NULL));
 	}
 
 	if (opt->cleanup_mode != CLEANUP_NONE)
@@ -436,7 +445,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	struct ref_format format = REF_FORMAT_INIT;
 	int icase = 0;
-	int edit_flag = 0;
+	int edit_flag = -1;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -592,7 +601,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("tag '%s' already exists"), tag);
 
 	opt.message_given = msg.given || msgfile;
-	opt.use_editor = edit_flag;
+	opt.force_editor = edit_flag >= 0;
+	opt.use_editor = opt.force_editor ? edit_flag : 0;
 
 	if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
 		opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 80eb13d94e..bf43d2c750 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \
 	'reediting a signed tag body omits signature' '
 	echo "rfc1991" >gpghome/gpg.conf &&
 	echo "RFC1991 signed tag" >expect &&
-	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
+	GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
 	test_cmp expect actual
 '
 
@@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \
 test_expect_success GPG,RFC1991 \
 	'reediting a signed tag body omits signature' '
 	echo "RFC1991 signed tag" >expect &&
-	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
+	GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
 	test_cmp expect actual
 '
 
-- 
2.23.0

Re: [RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()

[Matheus Tavares Bernardino]

On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:
>
> Improve code readability by moving tag reading to a new function called
> get_tag_body, which will be used in the implementation of the git tag
> --amend functionality.

This function is also used in your third patch, to fix git-tag's
--no-edit implementation, right? Maybe you could mention this here,
instead of referencing the --amend feature, as the former is already
present in this series.

> Add warning in write_tag_body() in case the tag body is not successfully
> recovered.
>
> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>
> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
> Helped-by: Matheus Tavares <matheus.bernardino@usp.br>

These tags usually come in chronological order. So I think the
Helped-by should come first as I helped you, then you and Bárbara
co-authored the patch and then, lastly, you sent it.

> ---
>  builtin/tag.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index e0a4c25382..e1e3549af9 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -170,26 +170,52 @@ static int git_tag_config(const char *var, const char *value, void *cb)
>         return git_color_default_config(var, value, cb);
>  }
>
> -static void write_tag_body(int fd, const struct object_id *oid)
> +/*
> + * Returns the tag body of the given oid or NULL, in case of error. If size is
> + * not NULL it is assigned the body size in bytes (excluding the '\0').
> + */
> +static char *get_tag_body(const struct object_id *oid, size_t *size)
>  {
> -       unsigned long size;
> +       unsigned long buf_size;
>         enum object_type type;
> -       char *buf, *sp;
> +       char *buf, *sp, *tag_body;
> +       size_t tag_body_size, signature_offset;
>
> -       buf = read_object_file(oid, &type, &size);
> +       buf = read_object_file(oid, &type, &buf_size);
>         if (!buf)
> -               return;
> +               return NULL;
>         /* skip header */
>         sp = strstr(buf, "\n\n");
>
> -       if (!sp || !size || type != OBJ_TAG) {
> +       if (!sp || !buf_size || type != OBJ_TAG) {
>                 free(buf);
> -               return;
> +               return NULL;
>         }
>         sp += 2; /* skip the 2 LFs */
> -       write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
> +       signature_offset = parse_signature(sp, buf + buf_size - sp);
> +       sp[signature_offset] = '\0';
>
> +       /* detach sp from buf */
> +       tag_body_size = strlen(sp) + 1;
> +       tag_body = xmalloc(tag_body_size);
> +       xsnprintf(tag_body, tag_body_size, "%s", sp);
>         free(buf);
> +       if (size)
> +               *size = tag_body_size - 1; /* exclude '\0' */
> +       return tag_body;
> +}
> +
> +static void write_tag_body(int fd, const struct object_id *oid)
> +{
> +       size_t size;
> +       const char *tag_body = get_tag_body(oid, &size);
> +
> +       if (!tag_body) {
> +               warning("failed to get tag body for %s", oid->hash);
> +               return;
> +       }
> +       printf("tag_body: <%s>\n", tag_body);

Hm, is this addition right or perhaps was it added in the patch by mistake?

> +       write_or_die(fd, tag_body, size);
>  }
>
>  static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
> --
> 2.23.0
>

Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code

[Matheus Tavares Bernardino]

On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:
>
> Improve code readability by moving tag body reading to a new function called
> get_tag_body. This function will be used in the following patches to fix the
> --no-edit flag.

This seems to be accidentally duplicated from patch 1/3.

> Enhance legibility by encapsulating code that loads previous tag message
> (if any) in new function prepare_tag_template. This code refactoring is
> part of a series of commits that intend to implement the git tag --amend
> flag and fix the functionality of --no-edit.
>
> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
> Helped-by: Matheus Tavares <matheus.bernardino@usp.br>
> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>

These tags can be re-ordered as I mentioned in patch 1/3, to follow a
chronological order: probably the Helped-by first followed by the
Co-authored-by, Barbara's S-o-B and then your S-o-B.

> ---
>  builtin/tag.c | 65 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index e1e3549af9..0322bdbdfb 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -244,6 +244,43 @@ static const char message_advice_nested_tag[] =
>            "\n"
>            "\tgit tag -f %s %s^{}");
>
> +/*
> + * Write the tag template message with previous tag body (if any) to the given
> + * file.
> + */

Maybe mention that the function creates the file at the given path?

> +static void prepare_tag_template(struct strbuf *given_msg,
> +                                struct create_tag_options *opt,
> +                                struct object_id *prev, char *path,
> +                                const char *tag)

I'm wondering if we could simplify this signature. Maybe we could
resolve whether a message was given at CLI, and if a previous tag
already exists, before getting to this function. This way, 'given_msg'
and 'prev' could be collapsed into a single 'struct strbuf *tag_body'
(and we could replace checking 'opt->message_given' and
'is_null_oid(prev)' by checking if 'tag_body' is not NULL). Then, we
could also pass just the 'cleanup_mode" instead of the whole
'create_tag_options'. Does this makes sense?

> +{
> +       int fd;
> +
> +       fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> +       if (fd < 0)
> +               die_errno(_("could not create file '%s'"), path);
> +
> +       if (opt->message_given) {
> +               write_or_die(fd, given_msg->buf, given_msg->len);
> +               strbuf_reset(given_msg);

I think keeping this reset at create_tag() (right before
lauch_editor() is called and only if 'opt->message_given') makes it
easier to understand what's happening (because there, we are cleaning
the given 'buf' to use it in lauch_editor()). Calling it here may be
misleading as 'given_msg' is not used in this function anymore.

> +       } else if (!is_null_oid(prev)) {
> +               write_tag_body(fd, prev);
> +       } else {
> +               struct strbuf template = STRBUF_INIT;
> +               strbuf_addch(&template, '\n');
> +               if (opt->cleanup_mode == CLEANUP_ALL) {
> +                       strbuf_commented_addf(&template, _(tag_template), tag,
> +                                             comment_line_char);
> +               } else {
> +                       strbuf_commented_addf(&template,
> +                                             _(tag_template_nocleanup), tag,
> +                                             comment_line_char);
> +               }
> +               write_or_die(fd, template.buf, template.len);
> +               strbuf_release(&template);
> +       }
> +       close(fd);
> +}
> +
>  static void create_tag(const struct object_id *object, const char *object_ref,
>                        const char *tag,
>                        struct strbuf *buf, struct create_tag_options *opt,
> @@ -251,7 +288,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>  {
>         enum object_type type;
>         struct strbuf header = STRBUF_INIT;
> -       char *path = NULL;
> +       char *path = git_pathdup("TAG_EDITMSG");
>
>         type = oid_object_info(the_repository, object, NULL);
>         if (type <= OBJ_NONE)
> @@ -271,31 +308,7 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>                     git_committer_info(IDENT_STRICT));
>
>         if (!opt->message_given || opt->use_editor) {
> -               int fd;
> -
> -               /* write the template message before editing: */
> -               path = git_pathdup("TAG_EDITMSG");
> -               fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> -               if (fd < 0)
> -                       die_errno(_("could not create file '%s'"), path);
> -
> -               if (opt->message_given) {
> -                       write_or_die(fd, buf->buf, buf->len);
> -                       strbuf_reset(buf);
> -               } else if (!is_null_oid(prev)) {
> -                       write_tag_body(fd, prev);
> -               } else {
> -                       struct strbuf buf = STRBUF_INIT;
> -                       strbuf_addch(&buf, '\n');
> -                       if (opt->cleanup_mode == CLEANUP_ALL)
> -                               strbuf_commented_addf(&buf, _(tag_template), tag, comment_line_char);
> -                       else
> -                               strbuf_commented_addf(&buf, _(tag_template_nocleanup), tag, comment_line_char);
> -                       write_or_die(fd, buf.buf, buf.len);
> -                       strbuf_release(&buf);
> -               }
> -               close(fd);
> -
> +               prepare_tag_template(buf, opt, prev, path, tag);
>                 if (launch_editor(path, buf, NULL)) {
>                         fprintf(stderr,
>                         _("Please supply the message using either -m or -F option.\n"));
> --
> 2.23.0
>

Re: [RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit

[Matheus Tavares Bernardino]

On Tue, Oct 8, 2019 at 3:47 PM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:
>
> git tag --edit and --no-edit flags are not currently fully supported.
> This patch fixes the functionality that allows the editor to be opened
> on demand.
>
> Co-authored-by: Bárbara Fernandes <barbara.dcf@gmail.com>
> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: Bárbara Fernandes <barbara.dcf@gmail.com>
> ---
>  builtin/tag.c  | 16 +++++++++++++---
>  t/t7004-tag.sh |  4 ++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 0322bdbdfb..7dff61d45a 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -230,6 +230,7 @@ static int build_tag_object(struct strbuf *buf, int sign, struct object_id *resu
>  struct create_tag_options {
>         unsigned int message_given:1;
>         unsigned int use_editor:1;
> +       unsigned int force_editor:1;

What if we turn 'use_editor' into a tri-state variable (--edit,
--no-edit and "nothing given") instead of adding this field? Maybe it
would simplify some condition checks at create_tag().

>         unsigned int sign;
>         enum {
>                 CLEANUP_NONE,
> @@ -307,13 +308,21 @@ static void create_tag(const struct object_id *object, const char *object_ref,
>                     tag,
>                     git_committer_info(IDENT_STRICT));
>
> -       if (!opt->message_given || opt->use_editor) {
> +       if (opt->force_editor && !opt->message_given && is_null_oid(prev) &&
> +           !opt->use_editor) {
> +               die(_("no tag message?"));
> +       } else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))
> +                 || (opt->force_editor && opt->use_editor)) {
> +               /* Editor must be opened */
>                 prepare_tag_template(buf, opt, prev, path, tag);
>                 if (launch_editor(path, buf, NULL)) {
>                         fprintf(stderr,
>                         _("Please supply the message using either -m or -F option.\n"));
>                         exit(1);
>                 }
> +       } else if (!opt->message_given) {
> +               /* Tag already exists and user doesn't want to change it */
> +               strbuf_addstr(buf, get_tag_body(prev, NULL));
>         }
>
>         if (opt->cleanup_mode != CLEANUP_NONE)
> @@ -436,7 +445,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>         static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>         struct ref_format format = REF_FORMAT_INIT;
>         int icase = 0;
> -       int edit_flag = 0;
> +       int edit_flag = -1;
>         struct option options[] = {
>                 OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>                 { OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
> @@ -592,7 +601,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>                 die(_("tag '%s' already exists"), tag);
>
>         opt.message_given = msg.given || msgfile;
> -       opt.use_editor = edit_flag;
> +       opt.force_editor = edit_flag >= 0;
> +       opt.use_editor = opt.force_editor ? edit_flag : 0;
>
>         if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
>                 opt.cleanup_mode = CLEANUP_ALL;
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 80eb13d94e..bf43d2c750 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \
>         'reediting a signed tag body omits signature' '
>         echo "rfc1991" >gpghome/gpg.conf &&
>         echo "RFC1991 signed tag" >expect &&
> -       GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
> +       GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
>         test_cmp expect actual
>  '
>
> @@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \
>  test_expect_success GPG,RFC1991 \
>         'reediting a signed tag body omits signature' '
>         echo "RFC1991 signed tag" >expect &&
> -       GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
> +       GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
>         test_cmp expect actual
>  '
>
> --
> 2.23.0
>

Re: [RFC WIP PATCH 0/3] tag: fix --edit and --no-edit flags

[Junio C Hamano]

Lucas Oshiro <lucasseikioshiro@gmail.com> writes:

> This series of patches fixes the flags --edit and --no-edit. Currently,
> --no-edit has no effect.
>
> These patches implement the following behaviour:
>
> - when --edit is provided, the editor will always be opened;
>
> - when --no-edit is provided, the editor will not be opened (if possible),
>   otherwise an error message will be displayed;
>
> - when neither --edit nor --no-edit are provided, the editor is opened only if
>   a message is not provided and there isn't a previous tag message.

Another thing that must be done is

 - when both --edit and --no-edit are given, the last one wins.

I do not know if you have implemented it in the three patches, though.

> In the future, the fix of these flags and the code factoring done in this
> patchset will be used on the implementation of a new flag --amend, as discussed
> on the mail thread started on
> https://public-inbox.org/git/CAHd499BM6M+=zRE1WFVXr7b+VhJHFeDind5xLqXcwZLv7QeDvw@mail.gmail.com/.
>
> Lucas Oshiro (3):
>   tag: factor out tag reading from write_tag_body()
>   tag: factor out prepare tag template code
>   tag: add full support for --edit and --no-edit
>
>  builtin/tag.c  | 123 ++++++++++++++++++++++++++++++++++---------------
>  t/t7004-tag.sh |   4 +-
>  2 files changed, 88 insertions(+), 39 deletions(-)

Re: [RFC WIP PATCH 1/3] tag: factor out tag reading from write_tag_body()

[Junio C Hamano]

Lucas Oshiro <lucasseikioshiro@gmail.com> writes:

> +/* 
> + * Returns the tag body of the given oid or NULL, in case of error. If size is
> + * not NULL it is assigned the body size in bytes (excluding the '\0').
> + */
> +static char *get_tag_body(const struct object_id *oid, size_t *size) 
>  {
> +	unsigned long buf_size;
>  	enum object_type type;
> +	char *buf, *sp, *tag_body;
> +	size_t tag_body_size, signature_offset;
>  
> +	buf = read_object_file(oid, &type, &buf_size);
>  	if (!buf)
> +		return NULL;
>  	/* skip header */
>  	sp = strstr(buf, "\n\n");
>  
> +	if (!sp || !buf_size || type != OBJ_TAG) {
>  		free(buf);
> +		return NULL;
>  	}

Returning early when !buf_size before even attempting to strstr
would be cleaner to read, i.e.

	buf = read_object_file(...);
	if (!buf || !buf_size) {
		free(buf);
		return NULL;
	}
	body = strstr(buf, "\n\n");

FWIW, the type check that is done after this point could also be a
part of the early return, as there is no point scanning for the end
of object header part if the object is not a tag (e.g. if it were a
blob, there is no "header part" and scanning for a blank line is
meaningless).
	
>  	sp += 2; /* skip the 2 LFs */
> +	signature_offset = parse_signature(sp, buf + buf_size - sp);
> +	sp[signature_offset] = '\0';
>  
> +	/* detach sp from buf */
> +	tag_body_size = strlen(sp) + 1;
> +	tag_body = xmalloc(tag_body_size);
> +	xsnprintf(tag_body, tag_body_size, "%s", sp);

Isn't this essentially

	tag_body = xstrdup(sp);
        tag_body_size = signature_offset;

(my arith may be off by one or two, but does a separate
tag_body_size need to exist?)

>  	free(buf);
> +	if (size)
> +		*size = tag_body_size - 1; /* exclude '\0' */
> +	return tag_body;
> +}
> +
> +static void write_tag_body(int fd, const struct object_id *oid)
> +{
> +	size_t size;
> +	const char *tag_body = get_tag_body(oid, &size);
> +
> +	if (!tag_body) {
> +		warning("failed to get tag body for %s", oid->hash);

I do not think the original gives any such warning.

 - Do we want to be unconditionally noisy this way?
 - Should this be a fatal error?  If not, why?
 - Should the message be translatable?

As an interface, is it sensible to force any and all callers of
get_tag_body() to supply a pointer to &size?  Is the returned value
always a NUL-terminated string?  I suspect that people would find it
a more natural interface if its were like:

	const char *body = get_tag_body(oid);

	if (!body)		
		...;

	if (this caller needs size) {
		size_t body_size = strlen(body);
		... use both body and body_size ...
		write_or_die(fd, body, body_size);
	} else {
		... just use body ...
		printf("%s", body);
	}
	
> +		return;
> +	}
> +	printf("tag_body: <%s>\n", tag_body);
> +	write_or_die(fd, tag_body, size);

WTH is this double writing?

>  }
>  
>  static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)

Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code

[Junio C Hamano]

Lucas Oshiro <lucasseikioshiro@gmail.com> writes:

> Improve code readability by moving tag body reading to a new function called
> get_tag_body.

Quite honestly, I think the result of this splitting is harder to
follow than the original.

For example, the value of opt->message_given and the validity of
given_msg is very closely related, so if you made the helper
function receive non-NULL given_msg when !opt->message_given, the
helper could only check !given_msg without having to worry about
opt->message_given; with such a change, I could buy that the split
improves code readability, but I do not see any such change in the
patch.

> Enhance legibility by encapsulating code that loads previous tag message
> (if any) in new function prepare_tag_template....

The helper seems to be used to _write_ into path, and not load or
read any message from either an object or a file on disk.

Re: [RFC WIP PATCH 3/3] tag: add full support for --edit and --no-edit

[Junio C Hamano]

Lucas Oshiro <lucasseikioshiro@gmail.com> writes:

>  struct create_tag_options {
>  	unsigned int message_given:1;
>  	unsigned int use_editor:1;
> +	unsigned int force_editor:1;
>  	unsigned int sign;

> -	if (!opt->message_given || opt->use_editor) {
> +	if (opt->force_editor && !opt->message_given && is_null_oid(prev) &&
> +	    !opt->use_editor) {
> +		die(_("no tag message?"));

If we didn't get a message from command line, and there is no
previous tag object to read the message from, there is nowhere but
editor to grab the message to be used, so !use_editor would trigger
an error if force_editor (i.e. the command line explicitly said
either --edit or --no-edit).  Makes sense, but I needed to cheat and
look at cmd_tag() to see how "force" is set to understand what is
going on.  I have to say "force" is not a good name for this field;
is this use similar to what we typically use an additional _given
field?

> +	} else if ((!opt->force_editor && !opt->message_given && is_null_oid(prev))
> +		  || (opt->force_editor && opt->use_editor)) {
> +		/* Editor must be opened */

If there is no --[no-]edit and there is no preexisting message, we
need to use the editor.  If the command line explicitly said --edit,
we also would use the editor.  OK.

But it starts to make me wonder if you rather want to replace the
single bit use_editor field with an enum with three possible values
(enum { EDITOR_UNSPECIFIED, EDITOR_YES, EDITOR_NO } use_editor).

>  		prepare_tag_template(buf, opt, prev, path, tag);
>  		if (launch_editor(path, buf, NULL)) {
>  			fprintf(stderr,
>  			_("Please supply the message using either -m or -F option.\n"));
>  			exit(1);
>  		}
> +	} else if (!opt->message_given) {
> +		/* Tag already exists and user doesn't want to change it */

Are we certain at this point in if/else cascade that prev is a valid
tag?  How?

> +		strbuf_addstr(buf, get_tag_body(prev, NULL));

This NULL tells us something about what I mentioned in my review on 1/3.


> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 80eb13d94e..bf43d2c750 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1313,7 +1313,7 @@ test_expect_success GPG,RFC1991 \
>  	'reediting a signed tag body omits signature' '
>  	echo "rfc1991" >gpghome/gpg.conf &&
>  	echo "RFC1991 signed tag" >expect &&
> -	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
> +	GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
>  	test_cmp expect actual
>  '
>  
> @@ -1356,7 +1356,7 @@ test_expect_success GPG,RFC1991 \
>  test_expect_success GPG,RFC1991 \
>  	'reediting a signed tag body omits signature' '
>  	echo "RFC1991 signed tag" >expect &&
> -	GIT_EDITOR=./fakeeditor git tag -f -s rfc1991-signed-tag $commit &&
> +	GIT_EDITOR=./fakeeditor git tag -f --edit -s rfc1991-signed-tag $commit &&
>  	test_cmp expect actual
>  '

Why do these two need explicit --edit option to invoke the editor?
That smells like an unnecessary backward incompatible change.

Thanks.

Re: [RFC WIP PATCH 2/3] tag: factor out prepare tag template code

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Lucas Oshiro <lucasseikioshiro@gmail.com> writes:
>
>> Improve code readability by moving tag body reading to a new function called
>> get_tag_body.
>
> Quite honestly, I think the result of this splitting is harder to
> follow than the original.
>
> For example, the value of opt->message_given and the validity of
> given_msg is very closely related, so if you made the helper
> function receive non-NULL given_msg when !opt->message_given, the
> helper could only check !given_msg without having to worry about
> opt->message_given; with such a change, I could buy that the split
> improves code readability, but I do not see any such change in the
> patch.

Just to avoid misunderstanding, I am not suggesting to change the
interface into the helper you introduced to be like this

	prepare_tag_template(opt->message_given ? buf, NULL,
			     opt, prev, path, tag);

That still needs to pass the entire opt structure into the helper
and forces the helper to look at opt->cleanup_mode and behave
differently.  If a restructuring of the code can be done in such a
way that the helper no longer need to look at opt (hence no need to
pass the entire opt structure into it), I can see that the change
improves the readability of the resulting code, but I am not all
that hopeful.

On the other hand (and this is the more important hand among the two
;-), I do not mind splitting a helper function out of an existing
codepath and make the existing codepath call the new helper, so that
the same helper can be reused from another codepath later.  Not at
all.  What I do mind is to mislabel a change that is done for such a
later code reuse as one that improves readability, when it does not.

Thanks.

>> Enhance legibility by encapsulating code that loads previous tag message
>> (if any) in new function prepare_tag_template....
>
> The helper seems to be used to _write_ into path, and not load or
> read any message from either an object or a file on disk.