Re: [PATCH 07/34] commit: release strbuf on error return in commit_tree_extended()

[Stefan Beller <sbeller@xxxxxxxxxx>]

On Wed, Aug 30, 2017 at 10:49 AM, Rene Scharfe <l.s.r@xxxxxx> wrote:
> Signed-off-by: Rene Scharfe <l.s.r@xxxxxx>
> ---
>  commit.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 8b28415939..51f969fcbc 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1514,60 +1514,63 @@ N_("Warning: commit message did not conform to UTF-8.\n"
>  int commit_tree_extended(const char *msg, size_t msg_len,
>                          const unsigned char *tree,
>                          struct commit_list *parents, unsigned char *ret,
>                          const char *author, const char *sign_commit,
>                          struct commit_extra_header *extra)
>  {
>         int result;
>         int encoding_is_utf8;
>         struct strbuf buffer;
>
>         assert_sha1_type(tree, OBJ_TREE);
>
>         if (memchr(msg, '\0', msg_len))
>                 return error("a NUL byte in commit log message not allowed.");
>
>         /* Not having i18n.commitencoding is the same as having utf-8 */
>         encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
>
>         strbuf_init(&buffer, 8192); /* should avoid reallocs for the headers */
>         strbuf_addf(&buffer, "tree %s\n", sha1_to_hex(tree));
>
>         /*
>          * NOTE! This ordering means that the same exact tree merged with a
>          * different order of parents will be a _different_ changeset even
>          * if everything else stays the same.
>          */
>         while (parents) {
>                 struct commit *parent = pop_commit(&parents);
>                 strbuf_addf(&buffer, "parent %s\n",
>                             oid_to_hex(&parent->object.oid));
>         }
>
>         /* Person/date information */
>         if (!author)
>                 author = git_author_info(IDENT_STRICT);
>         strbuf_addf(&buffer, "author %s\n", author);
>         strbuf_addf(&buffer, "committer %s\n", git_committer_info(IDENT_STRICT));
>         if (!encoding_is_utf8)
>                 strbuf_addf(&buffer, "encoding %s\n", git_commit_encoding);
>
>         while (extra) {
>                 add_extra_header(&buffer, extra);
>                 extra = extra->next;
>         }
>         strbuf_addch(&buffer, '\n');
>
>         /* And add the comment */
>         strbuf_add(&buffer, msg, msg_len);
>
>         /* And check the encoding */
>         if (encoding_is_utf8 && !verify_utf8(&buffer))
>                 fprintf(stderr, _(commit_utf8_warn));
>
> -       if (sign_commit && do_sign_commit(&buffer, sign_commit))
> -               return -1;
> +       if (sign_commit && do_sign_commit(&buffer, sign_commit)) {
> +               result = -1;
> +               goto out;
> +       }

While this seems obviously correct (following the "goto cleanup" pattern),
I shortly wondered if it can be expressed more concisely, as we really skip
only one call:

    if (!sign_commit || !do_sign_commit(&buffer, sign_commit))
        result = write_sha1_file(...)
    else
        result = -1;

    strbuf_release(&buffer);
    return result;

Instead of an if, we could even inline it as

    result = (!sign_commit || !do_sign_commit(&buffer, sign_commit)) ?
        write_sha1_file(buffer.buf, buffer.len, commit_type, ret) : -1;

     strbuf_release(&buffer);

I am not sure if this is easier to read (maybe that is too dense?)
Just thought for food, the original patch looks good, too.

Thanks,
Stefan