Hi Johannes,
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())
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
