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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stefan,

On Mon, 23 Apr 2018, Stefan Beller wrote:

> 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())

Yes, you are right!

> 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(...);
>     }

I went for the easier route: call error_errno() before close(fd), and then
return -1 after close(cmd.out). Since error_errno() always returns -1, the
result is pretty much the same (I do not think that we want the caller of
import_object() to rely on the 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?

Yep.

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

I even saved one variable ;-)

> > @@ -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?

Oy vey. How many more mistakes can I introduce in one commit...

> > @@ -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?

Absolutely.

Thank you for helping me improve these patches,
Dscho