Re: [PATCH v5 00/11] Deprecate .git/info/grafts

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> 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.