Re: [PATCH] Segmentation Fault on non-commit --branch clone

[Junio C Hamano <gitster@pobox.com>]

Davide Berardi <berardi.dav@gmail.com> writes:

> +static int fallback_on_noncommit(const struct ref *check,
> +				 const struct ref *remote,
> +				 const char *msg)
> +{
> +	if (check == NULL)
> +		return 1;
> +	struct commit *c = lookup_commit_reference_gently(the_repository,
> +						   &check->old_oid, 1);

This is decl-after-stmt.  Can check be NULL, though?  IOW, the first
two lines in this function should go.

> +	if (c == NULL) {

We tend to say "if (!c) {" instead.

> +		/* Fallback HEAD to fallback refs */

You are falling back to just a single ref (i.e. s/refs/ref/) but
more importantly, what the code is doing is obvious enough without
this comment.  What we want commenting on is _why_ we do this.

	/*
	 * The ref specified to be checked out does not point at a
	 * commit so pointing HEAD at it will leave us a broken
         * repository.  But we need to have _something_ plausible
	 * in HEAD---otherwise the result would not be a repository.
	 */

would explain why we point HEAD to the default 'master' branch.

> +		warning(_("%s is not a valid commit object, HEAD will fallback to %s"),
> +			check->name, FALLBACK_REF);
> +		update_ref(msg, FALLBACK_REF, &remote->old_oid, NULL,
> +			   REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);

Having said that, I was hoping that this would use the help from the
guess_remote_head() like the case without "-b" option does, so that
we do not have to use a hardcoded default.

> +	}
> +
> +	return c == NULL;

Our API convention is 0 for success and negavive for failure.

> +}
> +
> static void update_head(const struct ref *our, const struct ref *remote,
> 			const char *msg)
> {
> 	const char *head;
> 	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> 		/* Local default branch link */
> 		if (create_symref("HEAD", our->name, NULL) < 0)
> 			die(_("unable to update HEAD"));
> @@ -718,12 +739,17 @@ static void update_head(const str

Here in the patch context is a "do this in a non-bare repository"
guard, and crucially, we do this:

> 			install_branch_config(0, head, option_origin, our->name);

That is, we add configuration for our->name (which is now
"refs/heads/master"), but I do not think we updated any of the other
field in *our to make the world a consistent place.  Is the object
pointed at by our local refs/heads/master in a reasonable
relationship with the object at the tip of the 'master' branch at
the remote site, or is can totally be unrelated because we made no
attempt to make our->old_oid or whatever field consistent with the
"corrected" reality?

Given the potential complication, and given that we are doing a
corretive action only so that we leave some repository the user can
fix manually, I am inclined to say that we should avoid falling into
this codepath.  I'll wonder about a totally different approach later
in this message that makes the fallback_on_noncommit() helper and
change to these existing parts of the update_head() function
unnecessary.

> 		}
> 	} else if (our) {
> +		if (fallback_on_noncommit(our, remote, msg) != 0)
> +			return;
> +		/* here commit is valid */
> 		struct commit *c = lookup_commit_reference(the_repository,
> 							   &our->old_oid);

What makes us certain that commit is valid?  our->old_oid is not
adjusted by the fallback_on_noncommit() function, but we did check
if it is a commit by doing the exact same lookup_commit_reference()
in there already, and we know it found a commit (otherwise the
function would have returned a non-zero to signal an error).

But it also means that we are making a redundant and unnecessary
call if the code is structured better.

This makes me wonder why we are not adding a single call to a helper
function at the beginning of the function, something like

	const struct oid *tip = NULL;
	struct commit *tip_commit = NULL;

	if (our)
		tip = &our->old_oid;
	else if (remote)
		tip = &remote->old_oid;

	if (!tip)
		return;

	tip_commit = lookup_commit_reference_gently(the_repository, tip);

Then, if !tip_commit, we know we need to fall back to something.  I
actually think it would probably be cleanest if we added

	if (!tip_commit) {
		/*
		 * The given non-commit cannot be checked out,
                 * so have a 'master' branch and leave it unborn.
                 */
		warning(_"non-commit cannot be checked out");
		create_symref("HEAD", "refs/heads/master", msg);
		return;
	}

That is, we always check out an unborn 'master' branch (which would
yield another warning at the beginning of checkout() function, which
is what we want) doing minimum damage, without even configuring any
remote tracking information.

The rest of the update_head() function could be left as-is, but with
the "see what we would be checking out first" approach, we probably
can lose some code (like the call to lookup_commit_reference() in
the "detached HEAD" case), without adding any extra logic.

> 	} else if (remote) {
> +		if (fallback_on_noncommit(remote, remote, msg) != 0)
> +			return;
> 		/*
> 		 * We know remote HEAD points to a non-branch, or
> 		 * HEAD points to a branch but we don't know which one.

Thanks.