Matthieu Moy <Matthieu.Moy@xxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html