Fix potential segfault on cloning invalid tag

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 717

Style. Remove SP around commit_lookup_helper on both sides.

foo_helper() is not particularly a meaningful name. Let's read on and see if we can come up with a better name.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 719

This initialization is not strictly needed, no? I'd rather see us leave it uninitialized.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 720

Excess SP before NULL.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 733

OK, so tip points at the location that has the object name of the object that is planned to be at the tip of the branch that is checked out to the working tree.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 735

Style. No SP after '(' before the condition.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 736

Why double asterisk here, unlike the early return we saw earlier? Let's lose the extra one.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 737

s/a object id/an object name/;

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 740

The design to punt on a non-existing object at this point in the callflow may be sound, but is !lookup_object() a reliable indication that the object name refers to a non-existing object?

In other words, I do not see what guarantees that we have already saw and parsed the named object and placed it in the hashtable the_repository->parsed_objects->obj_hash[].

Is there a reason we should avoid using parse_object() here instead?

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 749

OK, the reasoning here is that we checked if tip refers to non-existing object before we tried lookup_commit_reference_gently(), so a failure to find a commit here must mean the object does exist but is not a committish. And in such a case, we decided to abort the calling update_head() and create an unborn branch ourselves.

Perhaps makes sense, I need to think about it a bit more to be certain, though.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 751

Style. Wrong placement of a comma before 'msg'.

Post v2.29 codebase should ask what our default first branch name should be.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 753

It is customary to use the returned value from error(), like so

		*status = error(_("..."));

that would save you one line. Taking all the nits above together, perhaps

	if (!tip_commit) {
		char *ref = xstrfmt("refs/heads/%s", git_default_branch_name());
		create_symref("HEAD", ref, msg);
		*err = error(_("..."));
		free(ref);
		return NULL;
	}

HOWEVER, I think it is much cleaner to lose the "we first check if the object exists" part and directly see if tip points at a committish first. Only when the tip does not point at a committish, we need to tell the two cases apart. So after figuring out into which ref the variable "tip" should point, we'd do something like:

	tip_commit = lookup_commit_refernece_gently(... tip, 1);
	if (tip_commit)
		return tip_commit;

	/*
	 * Now, "tip" is not a committish.  Does it even exist?
	 */
	if (!lookup_object(... tip))
		/* let update_ref() handle this error condition */
		return NULL;

	/*
	 * Give an error, but leave the default initial branch
	 * unborn, instead of failing the whole operation to cause
	 * the directory removed.
	 */
	... do the create_symref() thing ...
	return NULL;

Wouldn't it be cleaner?

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 760

If you did not do unnecessary rewrapping, the patch noise in the function definition here would have been avoided, no?

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 765

Style. Lose SP between asterisk and c; in our codebase the asterisk sticks to the identifier (not type).

Have a blank line here, between the end of decl and the first stmt in the block.

builtin/clone.c static void update_remote_refs(const struct ref *refs, · line 769

Style. No {} around a single stmt.

More importantly, without a name that is more meaningful than "commit_lookup_helper()" and without any comment near its definition to describe what the function is supposed to compute and what it considers an error, the reader of this callsite cannot tell what we are calling the "helper" for, and under what condition we are told by the helper to return an error to our callers.

With the analysis and rewrite of that _helper() function, I think a more descriptive name for the helper should contain words like "check/ensure" and "commit" in it. And the "err" variables, both in the caller and in the helper function, are somewhat misnamed. It is a signal from the callee to the caller that updating the branch head has already been done by the callee, so the caller does not have to do anything further. Perhaps

	int skip_update_ref = 0;
	const struct commit *c;

	c = check_tip_commit(our, remote, msg, &skip_update_ref);
	if (skip_update_ref)
		return -1; /* left a fallback unborn branch */

	... and then "c" is a commit (already peeled) usable for
	... pointing HEAD to at this point.

I'd suggest giving the "c" variable, whose lifespan is now much longer than in the original code, a more meaningful name, though.

builtin/clone.c static void update_head(const struct ref *our, const struct ref *remote, · line 791

Can't this cause discrepancy when the helper picked remote->old_oid as the "tip" there? We called lookup_commit_reference_gently() on that oid, and decided to check out the resulting commit and point HEAD at it, so we are certain that your "c" is a commit object, but remote->old_oid may name an annotated tag that points at the commit object. Don't we want to use &c->object.old here as well?

If that is the case (you need to think about it yourself---I didn't spend more than 5 minutes on this particular point myself and the above might be total nonsense), this part would become

	if (our && our is in our local branch namespace) {
		... do the local thing ...
	} else if (our || remote) {
		update_ref(msg, "HEAD", &c->object.oid, NULL, ...);
	}

as the body for 'our' and 'remote' are the same.

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1427

The logic looks OK. We avoid checking out an unborn if we left HEAD unborn because there is no better choice. And by not calling checkout() and overwriting the err variable, we make sure that the entire command still reports a failure.

I am not absolutely sure if reusing 'err' like the above two hunks is the best thing to do, or the code becomes easier to follow if we introduced another variable to receive the returned value from update_head(). Perhaps what we have above is good enough. I dunno.

t/t5609-clone-branch.sh test_expect_success 'setup' ' · line 25

Style. Do not leave SP between redirection operator and its target, i.e.

	echo $blob >.git/refs/heads/...

Didn't you mean to use refs/tags/broken-tag for the first one, though? Otherwise, I do not see how anybody would expect the two tests check different things.

Thanks.