Re: [PATCH 4/7] push: introduce new push.default mode "simple"

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

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +* `simple` - like `upstream`, but refuses to push if the upstream
> +  branch's name is different from the local one. This is the safest
> +  option and is well-suited for beginners.

Looks good.

> diff --git a/builtin/push.c b/builtin/push.c
> index 6936713..dae8306 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -76,7 +76,40 @@ static int push_url_of_remote(struct remote *remote, const char ***url_p)
>  	return remote->url_nr;
>  }
>  
> -static void setup_push_upstream(struct remote *remote)
> +NORETURN die_push_simple(struct branch *branch, struct remote *remote) {

Not static?

> +	/*
> +	 * There's no point in using shorten_unambiguous_ref here,
> +	 * as the ambiguity would be on the remote side, not what
> +	 * we have locally. Plus, this is supposed to be the simple
> +	 * mode. If the user is doing something crazy like setting
> +	 * upstream to a non-branch, we should probably be showing
> +	 * them the big ugly fully qualified ref.
> +	 */
> +	const char *short_up = skip_prefix(branch->merge[0]->src, "refs/heads/");

Unless you change behaviour depending on NULL-ness of this variable
later in this code (and I do not think you do---this is only for a
message string as far as I can see), I'd prefer to see that ?: you have
at the use site here instead, i.e.

	if (!short_up)
		short_up = branch->merge[0]->src;

perhaps with s/short_up/dest_branch/ or something.

> +	/*
> +	 * Don't show advice for people who explicitely set
> +	 * push.default.
> +	 */
> +	const char *advice_maybe = "";
> +	if (push_default == PUSH_DEFAULT_UNSPECIFIED)
> +		advice_maybe = _("\n"
> +				 "To choose either option permanently, "
> +				 "see push.default in 'git help config'.");

Nice.

> +	die(_("The upstream branch of your current branch does not match\n"
> +	      "the name of your current branch.  To push to the upstream branch\n"
> +	      "on the remote, use\n"
> +	      "\n"
> +	      "    git push %s HEAD:%s\n"
> +	      "\n"
> +	      "To push to the branch of the same name on the remote, use\n"
> +	      "\n"
> +	      "    git push %s %s\n"
> +	      "%s"),
> +	    remote->name, short_up ? short_up : branch->merge[0]->src,
> +	    remote->name, branch->name, advice_maybe);
> +}

> @@ -103,6 +136,9 @@ static void setup_push_upstream(struct remote *remote)
>  		      "your current branch '%s', without telling me what to push\n"
>  		      "to update which remote branch."),
>  		    remote->name, branch->name);
> +	if (simple && strcmp(branch->refname, branch->merge[0]->src)) {
> +		die_push_simple(branch, remote);
> +	}

Lose unnecessary {} pair, perhaps?

> +	git --git-dir=repo1 log -1 --format="%h %s" "other-name" >expect-other-name &&
> +	test_push_success current master &&
> +	git --git-dir=repo1 log -1 --format="%h %s" "other-name" >actual-other-name &&
> +	test_cmp expect-other-name actual-other-name

Hrm.

There is nothing wrong in the above part, but it shows taht it would be
very nice if test_push_success helper also encapsulated the "make sure
others did not change" logic.

Thanks for a pleasant read.