Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> - if (export_object(&old_oid, type, raw, tmpfile))
> - return -1;
> - if (launch_editor(tmpfile, NULL, NULL) < 0)
> - return error("editing object file failed");
> - if (import_object(&new_oid, type, raw, tmpfile))
> + tmpfile = git_pathdup("REPLACE_EDITOBJ");
> + if (export_object(&old_oid, type, raw, tmpfile) ||
> + (launch_editor(tmpfile, NULL, NULL) < 0 &&
> + error("editing object file failed")) ||
> + import_object(&new_oid, type, raw, tmpfile)) {
> + free(tmpfile);
> return -1;
> -
> + }
I know the above is to avoid leaking tmpfile, but a single if ()
condition that makes multiple calls to functions primarily for their
side effects is too ugly to live. Perhaps something like
if (export_object(...))
goto clean_fail;
if (launch_editor(...)) {
error("editing object file failed");
goto clean_fail;
}
if (import_object(...)) {
clean_fail:
free(tmpfile);
return -1;
}
would keep the ease-of-reading of the original while plugging the
leak. It would even be cleaner to move the body of clean_fail:
completely out of line, instead of having it in the last of three
steps like the above.
Other than that, looked cleanly done. Thanks for a pleasant read.
Will queue.
