Re: [PATCH v10] 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:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> If you want to fork your topic branch from the very latest of the
> tip of a branch your remote has, you would do:
>
>     git fetch origin some-branch
>     git checkout -b new_branch --track origin/some-branch
>
> Extend the "--track" option of "git checkout" and allow users to
> write
>
>     git checkout -b new_branch --track=fetch origin/some-branch
>
> to (1) fetch 'some-branch' from the remote 'origin', updating the
> remote-tracking branch 'origin/some-branch', (2) arrange subsequent
> 'git pull' on 'new_branch' to interact with 'origin/some-branch' and
> (3) fork 'new_branch' from it.
>
> In the value of the '--track' option, 'fetch' can be combined with
> the existing 'direct' (default) and 'inherit' modes via a
> comma-separated list. Examples:
>
>     git checkout -b new_branch --track=fetch,inherit some_local_branch
>     git switch -c new_branch --track=fetch origin/some-branch
>
> When "fetch" is requested and <start-point> is in <remote>/<branch>
> form, run "git fetch <remote> <branch>" before resolving the ref, so
> that other remote-tracking branches are left untouched. If
> <start-point> is a bare remote name like "origin" (which resolves to
> that remote's default branch), "git fetch <remote>" is run instead,
> since the target branch is not known up front. Abort the checkout if
> the fetch fails.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>     checkout: --track=fetch
>     
>     Rebased to fix merge conflict with master.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2281%2FHaraldNordgren%2Fcheckout-fetch-start-point-v10
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2281/HaraldNordgren/checkout-fetch-start-point-v10
> Pull-Request: https://github.com/git/git/pull/2281
>
> Range-diff vs v9:
>
>  1:  021375e4cc ! 1:  a773fb6bdf checkout: extend --track with a "fetch" mode to refresh start-point
>      @@ Documentation/git-checkout.adoc: of it").
>        the refspec configured for the corresponding remote, and then stripping
>       
>        ## Documentation/git-switch.adoc ##
>      -@@ Documentation/git-switch.adoc: should result in deletion of the path).
>      +@@ Documentation/git-switch.adoc: variable.
>        	attached to a terminal, regardless of `--quiet`.
>        
>        `-t`::
>      @@ builtin/checkout.c
>        #include "resolve-undo.h"
>        #include "revision.h"
>       +#include "run-command.h"
>      + #include "sequencer.h"
>        #include "setup.h"
>        #include "strvec.h"
>      - #include "submodule.h"
>       @@ builtin/checkout.c: struct checkout_opts {
>        	int count_checkout_paths;
>        	int overlay_mode;
>
>
>  Documentation/git-checkout.adoc |  13 ++-
>  Documentation/git-switch.adoc   |  13 ++-
>  builtin/checkout.c              | 168 +++++++++++++++++++++++++++++++-
>  t/t7201-co.sh                   | 144 +++++++++++++++++++++++++++
>  4 files changed, 332 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-checkout.adoc b/Documentation/git-checkout.adoc
> index a8b3b8c2e2..ec63434159 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. If the fetch fails and the
> +corresponding remote-tracking ref already exists, a warning is printed
> +and the checkout proceeds from the existing tip; otherwise the checkout
> +is aborted.
> ++
>  If no `-b` option is given, the name of the new branch will be
>  derived from the remote-tracking branch, by looking at the local part of
>  the refspec configured for the corresponding remote, and then stripping
> diff --git a/Documentation/git-switch.adoc b/Documentation/git-switch.adoc
> index d6c4f229a5..b5e79435cd 100644
> --- a/Documentation/git-switch.adoc
> +++ b/Documentation/git-switch.adoc
> @@ -155,11 +155,22 @@ variable.
>  	attached to a terminal, regardless of `--quiet`.
>  
>  `-t`::
> -`--track[ (direct|inherit)]`::
> +`--track[=(direct|inherit|fetch)[,...]]`::
>  	When creating a new branch, set up "upstream" configuration.
>  	`-c` is implied. See `--track` in linkgit:git-branch[1] for
>  	details.
>  +
> +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. If the fetch fails and the
> +corresponding remote-tracking ref already exists, a warning is printed
> +and the switch proceeds from the existing tip; otherwise the switch is
> +aborted.
> ++
>  If no `-c` option is given, the name of the new branch will be derived
>  from the remote-tracking branch, by looking at the local part of the
>  refspec configured for the corresponding remote, and then stripping
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 1345e8574a..fc58456546 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -25,10 +25,12 @@
>  #include "preload-index.h"
>  #include "read-cache.h"
>  #include "refs.h"
> +#include "refspec.h"
>  #include "remote.h"
>  #include "repo-settings.h"
>  #include "resolve-undo.h"
>  #include "revision.h"
> +#include "run-command.h"
>  #include "sequencer.h"
>  #include "setup.h"
>  #include "strvec.h"
> @@ -62,6 +64,7 @@ struct checkout_opts {
>  	int count_checkout_paths;
>  	int overlay_mode;
>  	int dwim_new_local_branch;
> +	int fetch;
>  	int discard_changes;
>  	int accept_ref;
>  	int accept_pathspec;
> @@ -115,6 +118,158 @@ struct branch_info {
>  	char *checkout;
>  };
>  
> +static int resolve_fetch_target(const char *arg, char **remote_out,
> +				char **src_ref_out, char **existing_ref_out)
> +{
> +	const char *slash;
> +	char *remote_name = NULL;
> +	struct remote *remote = NULL;
> +	struct refspec_item query = { 0 };
> +	struct strbuf dst = STRBUF_INIT;
> +	struct object_id oid;
> +	const char *rest = NULL;
> +	const char *head_target = NULL;
> +	const char *short_target;
> +
> +	*remote_out = NULL;
> +	*src_ref_out = NULL;
> +	*existing_ref_out = NULL;
> +
> +	if (!arg || !*arg || *arg == '/')
> +		return -1;
> +
> +	slash = arg + strlen(arg);
> +	while (1) {
> +		free(remote_name);
> +		remote_name = xstrndup(arg, slash - arg);
> +		remote = remote_get(remote_name);
> +		if (remote && remote_is_configured(remote, 1))
> +			break;
> +		while (slash > arg && *--slash != '/')
> +			;
> +		if (slash == arg) {
> +			free(remote_name);
> +			return -1;
> +		}
> +	}

OK.  So the caller gives "foo/bar/baz" when "foo/bar" is the name of
the remote that uses "refs/remotes/foo/bar" to store remote-tracking
branches from there.  You check "foo/bar/baz", which fails to be a
remote, then "foo/bar", which is a configured remote and break.

> +
> +	if (*slash == '/' && slash[1])
> +		rest = slash + 1;

And "baz" becomes the "rest"; if the user gave us "foo/bar" and the
abobve loop found it as the name of the remote, we would want to use
"refs/remotes/foo/bar/HEAD", which is the "if (!rest)" below is
about.

> +	if (!rest) {
> +		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);
> +		if (head_target) {
> +			*existing_ref_out = xstrdup(dst.buf);
> +			if (skip_prefix(head_target, "refs/remotes/", &short_target) &&
> +			    skip_prefix(short_target, remote_name, &short_target) &&
> +			    *short_target == '/')
> +				rest = short_target + 1;
> +		}
> +		strbuf_reset(&dst);
> +	}

So we may have been given "foo/bar" and after resolving HEAD there,
have "baz" in rest.  Or "foo/bar/baz" was given and we may have
figured out that that is "baz" in "foo/bar".

> +	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);
> +		}
> +		if (!*existing_ref_out) {
> +			strbuf_reset(&dst);
> +			strbuf_addf(&dst, "refs/remotes/%s", arg);
> +			if (!refs_read_ref(get_main_ref_store(the_repository),
> +					   dst.buf, &oid))
> +				*existing_ref_out = xstrdup(dst.buf);
> +		}
> +	}

What happens if "HEAD" was not there, though.  If "refs/remotes/foo/bar/"
did not exist, we would have already returned -1 after trying to
find which part in arg is the remote name.  But if "refs/remotes/foo/bar"
is valid, the user gave us "foo/bar", and we cannot find HEAD, then
the above "if (rest)" is skipped.  We give "foo/bar" to *remote_out,
and return 0 without touching *src_ref_out or *existing_ref_out at
all.

> +	strbuf_release(&dst);
> +	*remote_out = remote_name;
> +	return 0;
> +}
> +
> +static void fetch_remote_for_start_point(const char *arg)
> +{
> +	char *remote_name = NULL;
> +	char *src_ref = NULL;
> +	char *existing_ref = NULL;
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	if (resolve_fetch_target(arg, &remote_name, &src_ref, &existing_ref))
> +		return;
> +
> +	strvec_pushl(&cmd.args, "fetch", remote_name, NULL);
> +	if (src_ref)
> +		strvec_push(&cmd.args, src_ref);

What should happen with this configuration

    [remote "origin"]
        fetch = refs/heads/*:refs/upstream/*

and the user says either of these two:

    $ git checkout --track=fetch upstream
    $ git checkout --track=fetch upstream/master

We fail to find in "where does the remote name ends and branch name
start?" loop that this request is about remote "origin" at all, no?
We may see in the former case that there is
refs/remotes/upstream/HEAD >that points at "master" in the same
hierarchy, but the code thinks "upstream" is the remote name, which
would mean you would "git fetch upstream", when the remote you need
to fetch from is "origin".

> +	cmd.git_cmd = 1;
> +	if (run_command(&cmd)) {
> +		if (existing_ref)
> +			warning(_("failed to fetch start-point '%s'; "
> +				  "using existing '%s'"),
> +				arg, existing_ref);
> +		else
> +			die(_("failed to fetch start-point '%s'"), arg);

If we failed to set *existing_ref_out, shouldn't we fail without
even attempting to call run_command() here, as we will have to die()
anyway even if "git fetch" succeeds.  For that matter, it may be
simpler and more correct for resolve_fetch_target() to fail (return
-1) when it happens, by making the lat "if (rest) {...}" to have a
corresponding "else { return -1 }" after it.

> +	}
> +
> +	free(remote_name);
> +	free(src_ref);
> +	free(existing_ref);
> +}

I'll stop here.