From: Junio C Hamano <gitster@pobox•com>
To: Matthieu Moy <Matthieu.Moy@imag•fr>
Cc: git@vger•kernel.org, Jeff King <peff@peff•net>,
Michael Haggerty <mhagger@alum•mit.edu>
Subject: Re: [PATCH 4/7] push: introduce new push.default mode "simple"
Date: Mon, 23 Apr 2012 08:52:55 -0700 [thread overview]
Message-ID: <xmqqehrela20.fsf@junio.mtv.corp.google.com> (raw)
In-Reply-To: <1335170284-30768-5-git-send-email-Matthieu.Moy@imag.fr> (Matthieu Moy's message of "Mon, 23 Apr 2012 10:38:01 +0200")
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.
next prev parent reply other threads:[~2012-04-23 15:53 UTC|newest]
Thread overview: 152+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-28 19:47 [ANNOUNCE] Git 1.7.10-rc3 Junio C Hamano
2012-03-29 9:52 ` Jeff King
2012-03-29 21:22 ` Junio C Hamano
2012-03-29 22:11 ` Jeff King
2012-03-30 1:54 ` Junio C Hamano
2012-03-30 7:13 ` push.default: current vs upstream Jeff King
2012-03-30 20:25 ` Junio C Hamano
2012-03-30 21:01 ` Jeff King
2012-03-30 21:28 ` Junio C Hamano
2012-03-30 21:53 ` Jeff King
2012-03-30 22:15 ` Junio C Hamano
2012-03-30 22:20 ` Jeff King
2012-03-30 23:12 ` Junio C Hamano
2012-03-31 22:49 ` Nathan Gray
2012-03-31 23:48 ` Seth Robertson
2012-04-01 2:22 ` Junio C Hamano
2012-04-01 5:58 ` Nathan Gray
2012-04-02 7:40 ` Matthieu Moy
2012-04-02 16:48 ` Junio C Hamano
2012-04-02 17:20 ` Matthieu Moy
2012-04-02 18:40 ` Junio C Hamano
2012-04-02 18:58 ` Matthieu Moy
2012-04-02 19:47 ` Junio C Hamano
2012-04-02 20:40 ` Matthieu Moy
2012-04-02 20:50 ` Junio C Hamano
2012-04-02 21:02 ` demerphq
2012-04-02 21:16 ` Matthieu Moy
2012-04-04 7:57 ` demerphq
2012-04-05 13:13 ` Jeff King
2012-04-05 16:46 ` Matthieu Moy
2012-04-06 7:15 ` Jeff King
2012-04-06 7:44 ` Matthieu Moy
2012-04-06 8:00 ` Jeff King
2012-04-06 17:41 ` Junio C Hamano
2012-04-06 18:01 ` Jehan Bing
2012-04-07 7:49 ` Michael Haggerty
2012-04-07 7:51 ` Jeff King
2012-04-07 8:40 ` Andrew Sayers
2012-04-12 7:11 ` Jeff King
2012-04-12 15:04 ` Junio C Hamano
2012-04-12 21:16 ` Andrew Sayers
2012-04-12 21:33 ` Junio C Hamano
2012-04-12 22:11 ` Jeff King
2012-04-12 22:59 ` Philip Oakley
2012-04-13 19:31 ` Junio C Hamano
2012-04-17 20:13 ` Andrew Sayers
2012-04-08 4:43 ` Junio C Hamano
2012-04-11 16:17 ` Matthieu Moy
2012-04-11 16:44 ` Junio C Hamano
2012-04-12 7:55 ` Jeff King
2012-04-12 8:09 ` Matthieu Moy
2012-04-12 8:14 ` Jeff King
2012-04-12 8:59 ` Matthieu Moy
2012-04-12 15:56 ` Junio C Hamano
2012-04-19 16:06 ` Junio C Hamano
2012-04-19 20:38 ` Matthieu Moy
2012-04-19 21:02 ` Junio C Hamano
2012-04-19 22:57 ` [RFC/PATCH 0/3] push.default upcomming change Matthieu Moy
2012-04-19 22:57 ` [PATCH 1/3] push: introduce new push.default mode "simple" Matthieu Moy
2012-04-19 23:46 ` Jeff King
2012-04-20 14:52 ` Matthieu Moy
2012-04-20 14:59 ` [PATCH 0/4 v2] push.default upcomming change Matthieu Moy
2012-04-20 14:59 ` [PATCH 1/4] Documentation: explain push.default option a bit more Matthieu Moy
2012-04-20 20:13 ` Jeff King
2012-04-20 21:28 ` Junio C Hamano
2012-04-21 3:51 ` Michael Haggerty
2012-04-21 4:08 ` Junio C Hamano
2012-04-21 5:01 ` Michael Haggerty
2012-04-21 5:42 ` Junio C Hamano
2012-04-20 14:59 ` [PATCH 2/4] push: introduce new push.default mode "simple" Matthieu Moy
2012-04-20 20:33 ` Jeff King
2012-04-22 16:24 ` Zbigniew Jędrzejewski-Szmek
2012-04-20 21:42 ` Junio C Hamano
2012-04-23 8:38 ` Matthieu Moy
2012-04-20 14:59 ` [PATCH 3/4] t5570: use explicit push refspec Matthieu Moy
2012-04-20 14:59 ` [PATCH 4/4] push: start warning upcoming default change for push.default Matthieu Moy
2012-04-20 20:35 ` [PATCH 0/4 v2] push.default upcomming change Jeff King
2012-04-22 11:05 ` Matthieu Moy
2012-04-23 2:50 ` Junio C Hamano
2012-04-23 8:37 ` [PATCH 0/7 v3] " Matthieu Moy
2012-04-23 8:37 ` [PATCH 1/7] Documentation: explain push.default option a bit more Matthieu Moy
2012-04-23 15:20 ` Junio C Hamano
2012-04-23 19:00 ` Philip Oakley
2012-04-23 19:11 ` Junio C Hamano
2012-04-23 21:01 ` Philip Oakley
2012-04-23 8:37 ` [PATCH 2/7] Undocument deprecated alias 'push.default=tracking' Matthieu Moy
2012-04-23 15:21 ` Junio C Hamano
2013-01-31 17:10 ` Ævar Arnfjörð Bjarmason
2013-01-31 17:35 ` Junio C Hamano
2013-01-31 19:07 ` Jonathan Nieder
2013-01-31 19:11 ` Jonathan Nieder
2013-01-31 19:58 ` Junio C Hamano
2013-01-31 19:41 ` Junio C Hamano
2013-01-31 19:57 ` Jonathan Nieder
2013-01-31 20:01 ` Junio C Hamano
2013-01-31 20:11 ` Jonathan Nieder
2013-01-31 20:42 ` Junio C Hamano
2013-01-31 20:44 ` Junio C Hamano
2013-01-31 20:04 ` Jonathan Nieder
2013-01-31 20:08 ` Matthieu Moy
2013-01-31 20:21 ` Junio C Hamano
2013-01-31 20:50 ` Junio C Hamano
2013-01-31 21:00 ` Jonathan Nieder
2013-02-01 1:15 ` Junio C Hamano
2013-01-31 21:08 ` Jonathan Nieder
2013-01-31 20:41 ` Philip Oakley
2012-04-23 8:38 ` [PATCH 3/7] t5528-push-default.sh: add helper functions Matthieu Moy
2012-04-23 15:36 ` Junio C Hamano
2012-04-23 16:02 ` Matthieu Moy
2012-04-23 16:16 ` Junio C Hamano
2012-04-23 16:20 ` Matthieu Moy
2012-04-23 16:57 ` Matthieu Moy
2012-04-23 17:09 ` Junio C Hamano
2012-04-23 16:42 ` Junio C Hamano
2012-04-23 16:45 ` [PATCH 2/3] fixup! " Junio C Hamano
2012-04-23 16:48 ` [PATCH 3/3] push: suggested updates to push configuration documentation Junio C Hamano
2012-04-23 8:38 ` [PATCH 4/7] push: introduce new push.default mode "simple" Matthieu Moy
2012-04-23 10:32 ` Michael Haggerty
2012-04-23 11:20 ` Matthieu Moy
2012-04-23 15:52 ` Junio C Hamano [this message]
2012-04-23 16:09 ` Matthieu Moy
2012-04-23 8:38 ` [PATCH 5/7] t5570: use explicit push refspec Matthieu Moy
2012-04-23 8:38 ` [PATCH 6/7] push: document the future default change for push.default (matching -> simple) Matthieu Moy
2012-04-23 8:38 ` [PATCH 7/7] push: start warning upcoming default change for push.default Matthieu Moy
2012-04-24 7:49 ` [PATCH 0/7 v4] push.default upcomming change Matthieu Moy
2012-04-24 7:50 ` [PATCH 1/7] Documentation: explain push.default option a bit more Matthieu Moy
2012-04-24 7:50 ` [PATCH 2/7] Undocument deprecated alias 'push.default=tracking' Matthieu Moy
2012-04-24 7:50 ` [PATCH 3/7] t5528-push-default.sh: add helper functions Matthieu Moy
2012-04-24 7:50 ` [PATCH 4/7] push: introduce new push.default mode "simple" Matthieu Moy
2012-04-25 1:53 ` Junio C Hamano
2012-04-25 11:01 ` Matthieu Moy
2012-04-24 7:50 ` [PATCH 5/7] t5570: use explicit push refspec Matthieu Moy
2012-04-24 7:50 ` [PATCH 6/7] push: document the future default change for push.default (matching -> simple) Matthieu Moy
2012-04-24 7:50 ` [PATCH 7/7] push: start warning upcoming default change for push.default Matthieu Moy
2012-04-24 19:28 ` [PATCH 0/7 v4] push.default upcomming change Junio C Hamano
2012-04-19 22:57 ` [PATCH 2/3] t5570: use explicit push refspec Matthieu Moy
2012-04-19 22:57 ` [PATCH 3/3] push: start warning upcoming default change for push.default Matthieu Moy
2012-04-26 5:44 ` [PATCH] t5541: warning message is given even with --quiet Junio C Hamano
2012-04-26 6:31 ` Matthieu Moy
2012-04-11 2:08 ` [RFC/PATCH] Give better 'pull' advice when pushing non-ff updates to current branch Christopher Tiwald
2012-04-06 11:38 ` push.default: current vs upstream Dmitry Potapov
2012-04-06 13:36 ` demerphq
2012-04-06 18:03 ` Dmitry Potapov
2012-04-06 18:48 ` demerphq
2012-04-06 19:38 ` Dmitry Potapov
2012-03-30 23:07 ` Junio C Hamano
2012-04-03 20:59 ` Jeff King
2012-04-03 21:04 ` Jeff King
2012-04-03 22:29 ` Junio C Hamano
2012-04-03 23:23 ` Junio C Hamano
2012-04-05 12:45 ` Jeff King
2012-04-08 12:52 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqehrela20.fsf@junio.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Matthieu.Moy@imag$(echo .)fr \
--cc=git@vger$(echo .)kernel.org \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox