Re: [PATCH v2 1/2] branch: suggest <remote>/<branch> on upstream slip

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

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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When setting the upstream of the current branch to the 'main' branch
> of the remote 'origin', i.e.,
>
>     $ git branch --set-upstream-to origin/main
>
> it is easy to mistakenly write
>
>     $ git branch --set-upstream-to origin main
>
> That is parsed as a request to set the upstream of the local branch
> 'main' to 'origin'. When 'main' does not exist, the command dies
> with:
>
>     fatal: branch 'main' does not exist
>
> pointing at a branch the user never meant to name.

It is more complete to add the other case here, something along the
lines of ...

    And then when 'main' does exist, the command would die with

        fatal: the requested upstream branch 'origin' does not exist

    leaving the user equally confused.

... no?  In any case, this is much more nicely described than the
previous round.  I see no room for confusion.

> When the operated-on branch is missing and '<remote>/<branch>' names
> a real remote-tracking ref, suggest the intended form:
>
>     $ git branch --set-upstream-to=origin/main
>
> The suggestion is gated on '<remote>/<branch>' existing so it only
> appears when a slipped slash is the likely explanation.

Makes sense.

Do we want to do anything on a case where the operated-on branch
does exist but '<remote>' is not a name suitable for an upstream,
but '<remote>/<branch>' is?

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 1572a4f9ef..cefc4519a7 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -706,6 +706,29 @@ static int edit_branch_description(const char *branch_name)
>  	return 0;
>  }
>  
> +static void die_if_upstream_looks_like_remote(const char *new_upstream, const char *branch_name)
> +{
> +	struct strbuf remote_ref = STRBUF_INIT;
> +	int code;
> +
> +	if (strchr(new_upstream, '/') ||
> +	    !remote_is_configured(remote_get(new_upstream), 0))
> +		return;
> +
> +	strbuf_addf(&remote_ref, "refs/remotes/%s/%s", new_upstream, branch_name);
> +	if (!refs_ref_exists(get_main_ref_store(the_repository), remote_ref.buf)) {
> +		strbuf_release(&remote_ref);
> +		return;
> +	}
> +
> +	code = die_message(_("--set-upstream-to takes a single <remote>/<branch> argument"));
> +	advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
> +			  _("Did you mean to use: git branch --set-upstream-to=%s/%s?"),
> +			  new_upstream, branch_name);

Do we still need the _if_enabled() thing here?  Isn't the caller
gated with the same condition in this version?

> +	strbuf_release(&remote_ref);
> +	exit(code);
> +}
> +
>  int cmd_branch(int argc,
>  	       const char **argv,
>  	       const char *prefix,
> @@ -957,6 +980,9 @@ int cmd_branch(int argc,
>  		if (!refs_ref_exists(get_main_ref_store(the_repository), branch->refname)) {
>  			if (!argc || branch_checked_out(branch->refname))
>  				die(_("no commit on branch '%s' yet"), branch->name);
> +			if (argc == 1 &&
> +			    advice_enabled(ADVICE_SET_UPSTREAM_FAILURE))
> +				die_if_upstream_looks_like_remote(new_upstream, argv[0]);
>  			die(_("branch '%s' does not exist"), branch->name);
>  		}

This is totally a tangent, but has anybody noticed that the web
interface to the lore archive seems to be constipated?  I am reading
over nntp and subscribers are reading from their inbox, so no real
harm done, but from time to time we get reminded how heavily our
development process relies on the services like kernel.org and feel
grateful to have them.