[RFC,WIP,2/3] tag: factor out prepare tag template code
diff mbox series

Message ID 20191008184727.14337-3-lucasseikioshiro@gmail.com
State New
Headers show
Series
  • tag: fix --edit and --no-edit flags
Related show

Commit Message

Lucas Oshiro Oct. 8, 2019, 6:47 p.m. UTC
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(-)

Comments

Matheus Tavares Bernardino Oct. 9, 2019, 3:02 a.m. UTC | #1
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
>
Junio C Hamano Oct. 10, 2019, 2:51 a.m. UTC | #2
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.
Junio C Hamano Oct. 10, 2019, 4:29 a.m. UTC | #3
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.

Patch
diff mbox series

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"));