Re: [PATCH 08/15] scalar: implement the `clone` subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 1, 2021, 16:45 UTC
Message-ID: <xmqqsfyoqm6e.fsf@gitster.g>
In-Reply-To: <2cbf0b611133df5fa7eed1bf38460f9d119d2a6e.1630359290.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> +static char *remote_default_branch(const char *url)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +
> +	cp.git_cmd = 1;
> +	strvec_pushl(&cp.args, "ls-remote", "--symref", url, "HEAD", NULL);
> +	strbuf_addstr(&out, "-\n");

Is this a workaround for the problem that the first "ref:" line won't be found by looking for "\nref: " in the loop? Cute, but the extra "-" is a bit misleading.

Show 12 quoted lines

> +	if (!pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) {
> +		char *ref = out.buf;
> +
> +		while ((ref = strstr(ref + 1, "\nref: "))) {
> +			const char *p;
> +			char *head, *branch;
> +
> +			ref += strlen("\nref: ");
> +			head = strstr(ref, "\tHEAD");
> +
> +			if (!head || memchr(ref, '\n', head - ref))
> +				continue;

OK. We expect "ref: " <refname> "\t" <head> "\n" where <head> is "HEAD" for their .git/HEAD and refs/remotes/<nick>/HEAD for their remote-tracking branch for the remote they call <nick>, on a single line. We reject a line that is not of that shape, and we reject a line that is about remote-tracking branch by only looking for "\tHEAD". Makes sense.

The strstr() goes from "ref + 1", which feels sloppy. When we reject the line we found that begins with "ref :", I would have expected that the next scan would start at the beginning of the next line, not from the middle of this line at the first letter 'e' in 'refs/heads/' on the current line "ref: refs/heads/.....". As long as the current line is long enough, strstr() would not miss the beginning of the next line, so it might be OK.

Show 10 quoted lines

> +			if (skip_prefix(ref, "refs/heads/", &p)) {
> +				branch = xstrndup(p, head - p);
> +				strbuf_release(&out);
> +				return branch;
> +			}
> +
> +			error(_("remote HEAD is not a branch: '%.*s'"),
> +			      (int)(head - ref), ref);
> +			strbuf_release(&out);
> +			return NULL;

OK. Any symref whose basename is HEAD in their remote-tracking hierarchy would have been rejected earlier in the loop.

Is there a particular reason why we return early here, instead of breaking out of hte loop and let the generic "failed to get" code path below to handle this case?

Show 18 quoted lines

> +		}
> +	}
> +	warning(_("failed to get default branch name from remote; "
> +		  "using local default"));
> +	strbuf_reset(&out);
> +
> +	child_process_init(&cp);
> +	cp.git_cmd = 1;
> +	strvec_pushl(&cp.args, "symbolic-ref", "--short", "HEAD", NULL);
> +	if (!pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) {
> +		strbuf_trim(&out);
> +		return strbuf_detach(&out, NULL);
> +	}
> +
> +	strbuf_release(&out);
> +	error(_("failed to get default branch name"));
> +	return NULL;
> +}

Show 57 quoted lines

> +static int cmd_clone(int argc, const char **argv)
> +{
> +	const char *branch = NULL;
> +	int full_clone = 0;
> +	struct option clone_options[] = {
> +		OPT_STRING('b', "branch", &branch, N_("<branch>"),
> +			   N_("branch to checkout after clone")),
> +		OPT_BOOL(0, "full-clone", &full_clone,
> +			 N_("when cloning, create full working directory")),
> +		OPT_END(),
> +	};
> +	const char * const clone_usage[] = {
> +		N_("scalar clone [<options>] [--] <repo> [<dir>]"),
> +		NULL
> +	};
> +	const char *url;
> +	char *enlistment = NULL, *dir = NULL;
> +	struct strbuf buf = STRBUF_INIT;
> +	int res;
> +
> +	argc = parse_options(argc, argv, NULL, clone_options, clone_usage, 0);
> +
> +	if (argc == 2) {
> +		url = argv[0];
> +		enlistment = xstrdup(argv[1]);
> +	} else if (argc == 1) {
> +		url = argv[0];
> +
> +		strbuf_addstr(&buf, url);
> +		/* Strip trailing slashes, if any */
> +		while (buf.len > 0 && is_dir_sep(buf.buf[buf.len - 1]))
> +			strbuf_setlen(&buf, buf.len - 1);
> +		/* Strip suffix `.git`, if any */
> +		strbuf_strip_suffix(&buf, ".git");
> +
> +		enlistment = find_last_dir_sep(buf.buf);
> +		if (!enlistment) {
> +			die(_("cannot deduce worktree name from '%s'"), url);
> +		}
> +		enlistment = xstrdup(enlistment + 1);
> +	} else {
> +		usage_msg_opt(_("You must specify a repository to clone."),
> +			      clone_usage, clone_options);
> +	}
> +
> +	if (is_directory(enlistment))
> +		die(_("directory '%s' exists already"), enlistment);
> +
> +	dir = xstrfmt("%s/src", enlistment);
> +
> +	strbuf_reset(&buf);
> +	if (branch)
> +		strbuf_addf(&buf, "init.defaultBranch=%s", branch);
> +	else {
> +		char *b = repo_default_branch_name(the_repository, 1);
> +		strbuf_addf(&buf, "init.defaultBranch=%s", b);
> +		free(b);

Doesn't "git clone" already use their HEAD without having to make an extra "git ls-remote" roundtrip?

Ahh, you do not do "git clone"; you do "git init", set things up, and then "git fetch" and checkout, all manually.

Which is kind of shame.

I wonder if it is a cleaner implementation to give a new option to "git clone" that gives a command sequence (not necessarily have to be implemented as a shell script) that specifies necessary pre-configuration steps to be done before the command starts the transfer step.