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

[Junio C Hamano <gitster@pobox.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.

> +		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)) {