Re: [PATCH v8] checkout: extend --track with a "fetch" mode to refresh start-point

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

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Documentation/git-checkout.adoc b/Documentation/git-checkout.adoc
> index 43ccf47cf6..28f17f427e 100644
> --- a/Documentation/git-checkout.adoc
> +++ b/Documentation/git-checkout.adoc
> @@ -158,11 +158,22 @@ of it").
>  	resets _<branch>_ to the start point instead of failing.
>  
>  `-t`::
> -`--track[=(direct|inherit)]`::
> +`--track[=(direct|inherit|fetch)[,...]]`::
>  	When creating a new branch, set up "upstream" configuration. See
>  	`--track` in linkgit:git-branch[1] for details. As a convenience,
>  	--track without -b implies branch creation.
>  +
> +The argument is a comma-separated list. `direct` (the default) and
> +`inherit` select the tracking mode and are mutually exclusive. Adding
> +`fetch` requests that the remote be fetched before _<start-point>_ is
> +resolved, so the new branch starts from a fresh tip: when
> +_<start-point>_ is in _<remote>/<branch>_ form, only that branch is
> +updated; when _<start-point>_ is a bare remote name (e.g. `origin`),
> +only the remote's default branch is updated.

The latter is because "checkout -t -b new remote/origin" makes "new"
track "remote/origin/HEAD", which makes sense.  And of course this
fetch can fail.

> +static int resolve_fetch_target(const char *arg, char **remote_out,
> +				char **src_ref_out)
> +{
> +	const char *slash;
> +	char *remote_name;
> +	struct remote *remote;
> +	struct refspec_item query = { 0 };
> +	struct strbuf dst = STRBUF_INIT;
> +	const char *rest;
> +
> +	*remote_out = NULL;
> +	*src_ref_out = NULL;
> +
> +	if (!arg || !*arg)
> +		return -1;
> +
> +	slash = strchr(arg, '/');
> +	if (slash == arg)
> +		return -1;
> +	remote_name = slash ? xstrndup(arg, slash - arg) : xstrdup(arg);
> +
> +	remote = remote_get(remote_name);
> +	if (!remote || !remote_is_configured(remote, 1)) {
> +		free(remote_name);
> +		return -1;
> +	}
> +
> +	rest = (slash && slash[1]) ? slash + 1 : NULL;

There is no slash when asking for "origin", and the control goes
into the "if (!rest)" block.

> +	if (!rest) {
> +		struct object_id oid;
> +		const char *head_target;
> +		const char *short_target;
> +
> +		strbuf_addf(&dst, "refs/remotes/%s/HEAD", remote_name);
> +		head_target = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +						      dst.buf,
> +						      RESOLVE_REF_READING |
> +						      RESOLVE_REF_NO_RECURSE,
> +						      &oid, NULL);
> +		strbuf_reset(&dst);
> +		if (head_target &&
> +		    skip_prefix(head_target, "refs/remotes/", &short_target) &&
> +		    skip_prefix(short_target, remote_name, &short_target) &&
> +		    *short_target == '/')
> +			rest = short_target + 1;
> +	}

If refs/remotes/origin/HEAD points at refs/remotes/origin/main, rest
gets "main".  Otherwise (i.e. unexpected contents in HEAD or lacking
HEAD), rest remains NULL.  And then we have the "if (rest)" block.

> +	if (rest) {
> +		strbuf_addf(&dst, "refs/remotes/%s/%s", remote_name, rest);
> +		query.dst = dst.buf;
> +		if (!remote_find_tracking(remote, &query) && query.src) {
> +			*src_ref_out = xstrdup(query.src);
> +			free(query.src);
> +		} else {
> +			*src_ref_out = xstrdup(rest);
> +		}
> +	}
> +
> +	strbuf_release(&dst);
> +	*remote_out = remote_name;
> +	return 0;
> +}

It is not a new problem but I do not remember how we explicitly
forbid "hierarchical" remote names.  refs/remotes/a/b/c might be
their branch "b/c" at remote we call "a", or branch "c" at remote
"a/b".  Unless we forbid slashes in remote names, the "there is no
slash so we got only the name of the remote to mean its HEAD" logic
would not work well, so we may want to double check.

> +static void fetch_remote_for_start_point(const char *arg)
> +{
> +	char *remote_name = NULL;
> +	char *src_ref = NULL;
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +	struct strbuf dst_ref = STRBUF_INIT;
> +	int have_existing_ref = 0;
> +
> +	if (resolve_fetch_target(arg, &remote_name, &src_ref))
> +		return;
> +
> +	{
> +		struct object_id oid;
> +
> +		if (strchr(arg, '/'))
> +			strbuf_addf(&dst_ref, "refs/remotes/%s", arg);
> +		else
> +			strbuf_addf(&dst_ref, "refs/remotes/%s/HEAD", arg);
> +		if (!refs_read_ref(get_main_ref_store(the_repository),
> +				   dst_ref.buf, &oid))
> +			have_existing_ref = 1;
> +	}

I do not quite see the point of this extra block.  Can we do without
it (and move the def of oid up near the beginning of the function,
of course)?

Even better, as resolve_fetch_target() already looks at "arg" and
poked at the remote-tracking ref hierarchy, wouldn't it make more
sense to make that helper function responsible for finding out if
there already is a usable, albeit potentially stale, ref?  


> +	strvec_pushl(&cmd.args, "fetch", remote_name, NULL);
> +	if (src_ref)
> +		strvec_push(&cmd.args, src_ref);
> +	cmd.git_cmd = 1;
> +	if (run_command(&cmd)) {
> +		if (have_existing_ref)
> +			warning(_("failed to fetch start-point '%s'; "
> +				  "using existing '%s'"),
> +				arg, dst_ref.buf);
> +		else
> +			die(_("failed to fetch start-point '%s'"), arg);
> +	}
> +
> +	free(remote_name);
> +	free(src_ref);
> +	strbuf_release(&dst_ref);
> +}

> @@ -1244,7 +1398,6 @@ static int git_checkout_config(const char *var, const char *value,
>  		opts->dwim_new_local_branch = git_config_bool(var, value);
>  		return 0;
>  	}
> -
>  	if (starts_with(var, "submodule."))
>  		return git_default_submodule_config(var, value, NULL);

Unrelated patch noise?