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

Brian Lyles [off-list ref] writes:

+	if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) {
+		/*
+		 * Check to see if this is an unborn branch
+		 */
+		head_name = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, &head_oid, NULL);
+		if (!head_name || !starts_with(head_name, "refs/heads/") || !is_null_oid(&head_oid))
+			return error(_("could not resolve HEAD commit"));
+		head_tree_oid = the_hash_algo->empty_tree;
+	} else {
+		head_commit = lookup_commit(r, &head_oid);
 
-	head_commit = lookup_commit(r, &head_oid);
+		/*
+		 * If head_commit is NULL, check_commit, called from
+		 * lookup_commit, would have indicated that head_commit is not
+		 * a commit object already.  repo_parse_commit() will return failure
+		 * without further complaints in such a case.  Otherwise, if
+		 * the commit is invalid, repo_parse_commit() will complain.  So
+		 * there is nothing for us to say here.  Just return failure.
+		 */
+		if (repo_parse_commit(r, head_commit))
+			return -1;

Not that I am qualified to do a review of your changes but I am in a
situation where I am trying to understand Git code in general and
(perhaps normal for this situation) wondering about varying styles of
commenting code -- could be that I am just too new to the code base and
do not yet understand the obvious things that don't need comments.

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.

So, my intention is by no means to trigger another spin of this series
-- it is just a view from someone trying to understand not just this
code ;-)

Dirk

[1] https://lore.kernel.org/git/xmqqh6hcu2tg.fsf@gitster.g/ (local)