From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 22 Nov 2019 11:09:49 GMT
Subject: Re: [PATCH 1/1] sequencer: fix empty commit check when amending
Message-ID: <b314c0b1-0e01-c8ea-bef2-7d1191b9080a@gmail.com>
In-Reply-To: <xmqqblt44d3h.fsf@gitster-ct.c.googlers.com>

Hi Junio

(sorry dscho I forgot to CC you on this patch)

On 22/11/2019 06:52, Junio C Hamano wrote:
> "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.

But current_head may be NULL so we'd end up with the equivalent of the 
ternary expression up here or an if/else to handle it. I thought it was 
clearer to find the parent we want to use and then get the tree from it.

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

I agree that's a clearer way of writing it. I'll re-roll with that

Best Wishes

Phillip

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