Re: [PATCH 1/1] sequencer: fix empty commit check when amending
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 22, 2019, 11:09 UTC
- 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:
Show 8 quoted lines
> "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.
Show 25 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)I agree that's a clearer way of writing it. I'll re-roll with that
Best Wishes
Phillip
Show 9 quoted lines
> 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)) {
>