Re: [PATCH v4 04/11] replace: "libify" create_graft() and callees

[Stefan Beller <sbeller@google.com>]

Hi Johannes,

On Fri, Apr 20, 2018 at 3:21 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> @@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum object_type type,
> -               if (strbuf_read(&result, cmd.out, 41) < 0)
> -                       die_errno("unable to read from mktree");
> +               if (strbuf_read(&result, cmd.out, 41) < 0) {
> +                       close(fd);
> +                       close(cmd.out);
> +                       return error_errno("unable to read from mktree");

So before the errno is coming directly from strbuf_read,
which will set errno on error to the desired errno.
(It will come from an underlying read())

However close() may fail and clobber errno,
so I would think we'd need to

    if (strbuf_read(&result, cmd.out, 41) < 0) {
      int err =  errno; /* close shall not clobber errno */
      close(fd);
      close(cmd.out);
      errno = err;
      return error_errno(...);
    }

> -               if (fstat(fd, &st) < 0)
> -                       die_errno("unable to fstat %s", filename);
> +               if (fstat(fd, &st) < 0) {
> +                       close(fd);
> +                       return error_errno("unable to fstat %s", filename);
> +               }

Same here?

An alternative would be to do
    ret = error_errno(...)
    close (..)
    return ret;


> @@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
>         struct strbuf ref = STRBUF_INIT;
>
>         if (get_oid(object_ref, &old_oid) < 0)
> -               die("Not a valid object name: '%s'", object_ref);
> +               return error("Not a valid object name: '%s'", object_ref);
>
>         type = oid_object_info(&old_oid, NULL);
>         if (type < 0)
> -               die("unable to get object type for %s", oid_to_hex(&old_oid));
> +               return error("unable to get object type for %s",
> +                            oid_to_hex(&old_oid));
>
> -       check_ref_valid(&old_oid, &prev, &ref, force);
> +       if (check_ref_valid(&old_oid, &prev, &ref, force))
> +               return -1;
>         strbuf_release(&ref);
>
> -       export_object(&old_oid, type, raw, tmpfile);
> +       if (export_object(&old_oid, type, raw, tmpfile))
> +               return -1;
>         if (launch_editor(tmpfile, NULL, NULL) < 0)
> -               die("editing object file failed");
> -       import_object(&new_oid, type, raw, tmpfile);
> +               return error("editing object file failed");
> +       if (import_object(&new_oid, type, raw, tmpfile))
> +               return -1;
>
>         free(tmpfile);

Do we need to free tmpfile in previous returns?

> @@ -394,24 +422,29 @@ static int create_graft(int argc, const char **argv, int force)
>         unsigned long size;
>
>         if (get_oid(old_ref, &old_oid) < 0)
> -               die(_("Not a valid object name: '%s'"), old_ref);
> -       commit = lookup_commit_or_die(&old_oid, old_ref);
> +               return error(_("Not a valid object name: '%s'"), old_ref);
> +       commit = lookup_commit_reference(&old_oid);
> +       if (!commit)
> +               return error(_("could not parse %s"), old_ref);
>
>         buffer = get_commit_buffer(commit, &size);
>         strbuf_add(&buf, buffer, size);
>         unuse_commit_buffer(commit, buffer);
>
> -       replace_parents(&buf, argc - 1, &argv[1]);
> +       if (replace_parents(&buf, argc - 1, &argv[1]) < 0)
> +               return -1;
>
>         if (remove_signature(&buf)) {
>                 warning(_("the original commit '%s' has a gpg signature."), old_ref);
>                 warning(_("the signature will be removed in the replacement commit!"));
>         }
>
> -       check_mergetags(commit, argc, argv);
> +       if (check_mergetags(commit, argc, argv))
> +               return -1;
>
>         if (write_object_file(buf.buf, buf.len, commit_type, &new_oid))
> -               die(_("could not write replacement commit for: '%s'"), old_ref);
> +               return error(_("could not write replacement commit for: '%s'"),
> +                            old_ref);
>
>         strbuf_release(&buf);

Release &buf in the other return cases, too?

Thanks,
Stefan