| Message ID | 20191008184727.14337-3-lucasseikioshiro@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tag: fix --edit and --no-edit flags | expand |
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 >
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 <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.
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"));