Re: [PATCH v4 3/3] clone: respect remote unborn HEAD

From: Jeff King <peff@peff.net>
Date: Jan 21, 2021, 21:02 UTC
Message-ID: <YAnr8lBESOO+ACL/@coredump.intra.peff.net>
In-Reply-To: <e770fc46eb3b600d8475d69964badaf2c63a7364.1608673963.git.jonathantanmy@google.com>

On Tue, Dec 22, 2020 at 01:54:20PM -0800, Jonathan Tan wrote:

Show 22 quoted lines

> @@ -1323,10 +1325,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		remote_head = NULL;
>  		option_no_checkout = 1;
>  		if (!option_bare) {
> -			const char *branch = git_default_branch_name();
> -			char *ref = xstrfmt("refs/heads/%s", branch);
> +			const char *branch;
> +			char *ref;
> +
> +			if (unborn_head_target &&
> +			    skip_prefix(unborn_head_target, "refs/heads/", &branch)) {
> +				ref = unborn_head_target;
> +				unborn_head_target = NULL;
> +			} else {
> +				branch = git_default_branch_name();
> +				ref = xstrfmt("refs/heads/%s", branch);
> +			}
>  
>  			install_branch_config(0, branch, remote_name, ref);
> +			create_symref("HEAD", ref, "");
>  			free(ref);
>  		}

In the old code, we never called create_symref() at all. It makes sense that we'd do it now when unborn_head_target is not NULL. But what about in the "else" clause there? Now we're adding an extra create_symref() call. Who was setting up the HEAD symref before, and are we now doing it twice?

If we have a valid unborn head, then we alias it to "ref" and we set the original to NULL. And it gets cleaned up here via free(ref). Makes sense. It confused me for a moment with this hunk...

Show 5 quoted lines

> @@ -1373,6 +1385,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&key);
>  	junk_mode = JUNK_LEAVE_ALL;
>  
> +	free(unborn_head_target);

...since this line will almost always be free(NULL) as a result (either there was no unborn head, or we consumed the string already). But it is covering the case that somebody gave us an unborn_head_target but it didn't start with refs/heads/. So it's useful to have.

-Peff