Re: [PATCH 4/7] push: introduce new push.default mode "simple"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 23, 2012, 15:52 UTC
- Message-ID
- <xmqqehrela20.fsf@junio.mtv.corp.google.com>
- In-Reply-To
- <1335170284-30768-5-git-send-email-Matthieu.Moy@imag.fr>
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.
Show 10 quoted lines
> 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?
Show 9 quoted lines
> + /* > + * 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.
Show 9 quoted lines
> + /*
> + * 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.
Show 13 quoted lines
> + 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);
> +}Show 7 quoted lines
> @@ -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.