Hi Stefan,
On Mon, 23 Apr 2018, Stefan Beller wrote:
> On Fri, Apr 20, 2018 at 3:21 PM, Johannes Schindelin
> <johannes.schindelin@xxxxxx> 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
