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