Re: [PATCH v4 4/7] sequencer: handle unborn branch with `--allow-empty`

[Dirk Gouders <dirk@gouders.net>]

Junio C Hamano <gitster@pobox.com> writes:

> Dirk Gouders <dirk@gouders.net> writes:
>
>> Brian Lyles <brianmlyles@gmail.com> writes:
>>
>>> +	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
>>> +		/*
>>> +		 * Check to see if this is an unborn branch
>>> +		 */
>> In the above example, there is a short but outstanding comment that
>> announces a check (and if I understood correctly by [1] it is a kind of
>> trick that could deserve some more information) and it does _not_
>> comment on the result.  Of course, I have an idea where the correct
>> place for a comment /* This is an unborn branch -- handle it as if... */
>> could be, but I'm not sure.
>
> You mean "Check to see if this is an unborn branch, and if so, use
> an empty tree to compare against, instead of the tree of the HEAD
> that does not yet exist"?
>
> I think that is possible, but the use of the_hash_algo->empty_tree
> indicates that clearly enough.  But we need to stop somewhere and
> what we see above may be a reasonable place to do so.
>
> If anything, we may want to say why we want to continue as if we had
> an empty tree (as opposed to fail and return with an error()), or
> the tree to compare with is computed here for what purpose.  But the
> name of the function may tell what this whole computation and
> comparison is for, so it probably is not needed, either.

Thank you for the reply.

I guess, the hidden question in my comment was: "Do experienced
Git developers understand the code as something obvious?".
And I read your answer as a "Yes, no problem.".

Now, I can put this subject aside and later, after more reading, check
if my understanding improved sufficiently to now understand that code
without additional comments, as you already do.

Dirk