Re: [PATCH 4/7] push: introduce new push.default mode "simple"
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:39
Matthieu Moy [off-list ref] 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.
quoted hunk
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);
+}quoted hunk
@@ -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.