| Message ID | 20191008184727.14337-2-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 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 >
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)
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)