Re: [PATCH 1/1] sequencer: fix empty commit check when amending

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2019, 06:52 UTC
Message-ID: <xmqqblt44d3h.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <7d34c0ff805b8637b23d0ef0045370dfc931a3af.1574345181.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (!(flags & ALLOW_EMPTY)) {
> +		struct commit *first_parent = current_head;

I would have used first_parent_tree variable instead here. That way, you did not have to have an overlong ternary expression down there, I expect.

Show 13 quoted lines

> +		if (flags & AMEND_MSG) {
> +			if (current_head->parents) {
> +				first_parent = current_head->parents->item;
> +				if (repo_parse_commit(r, first_parent)) {
> +					res = error(_("could not parse HEAD commit"));
> +					goto out;
> +				}
> +			} else {
> +				first_parent = NULL;
> +			}
> +		}
> +		if (oideq(first_parent ? get_commit_tree_oid(first_parent) :
> +					 the_hash_algo->empty_tree, &tree)) {

Style. It often makes the code much easier to read when you strive to break lines at the boundary of larger syntactic units. In this (A ? B : C, D) parameter list, ternary A ? B : C binds much tighter than the comma after it, so if you are breaking line inside it, you should break line after the comma, too, i.e.

	oideq(first_parent
	      ? get_commit_tree_oid(first_parent)
	      : the_hash_algo->empty_tree,
	      &tree)

to avoid appearing if C and D have closer relationship than B and C, which your version gives.

But I hope that it becomes a moot point, if we computed first_parent_tree inside the new "if (flags & AMEND_MSG)" block to leave this oideq() just

	if (oideq(first_parent_tree, &tree)) {