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

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

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

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 1572a4f9ef..dede60d27b 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);
> +	strbuf_release(&remote_ref);
> +	exit(code);
> +}
> +
>  int cmd_branch(int argc,
>  	       const char **argv,
>  	       const char *prefix,
> @@ -957,6 +980,15 @@ 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);
> +			/*
> +			 * Check the advice up front to avoid the ref
> +			 * lookups when the hint is off. The helper still
> +			 * calls advise_if_enabled() so the hint carries the
> +			 * standard "disable this message" instructions.
> +			 */
> +			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);
>  		}

Hmph, something like adding a single liner in the caller, like this. ...

	code = die_message(_("--set-upstream-to takes a single <remote>/<branch> argument"));
+	/* use _if_enabled here to show the hint on how to disable */
	advise_if_enabled(ADVICE_SET_UPSTREAM_FAILURE,
			  _("Did you mean to use: git branch --set-upstream-to=%s/%s?"),
			  new_upstream, branch_name);
	strbuf_release(&remote_ref);
	exit(code);

... was what I meant, because the most puzzling piece is that the
function calls _if_enabled form there, when the caller is presumably
already checked _enabled() and leaves the reader wondering if there
are other callers of this function that does not check before
calling it.

But this is so tiny a thing that once the code is written, it is
probably not worth the churn to redo it.  Let's declare victory and
mark the topic ready for 'next'?