Re: [PATCH 6/6] push: honor branch.*.push
On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano gits...@pobox.com wrote:
When branch.*.push configuration variable is defined for the current
branch, a lazy-typing git push (and git push there) will push
the commit at the tip of the current branch to the destination and
update the branch named by that variable.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/push.c | 18 +-
remote.c | 5 +
remote.h | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/builtin/push.c b/builtin/push.c
index f6c8047..a140b8e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -185,6 +185,15 @@ static void
warn_unspecified_push_default_configuration(void)
warning(%s\n, _(warn_unspecified_push_default_msg));
}
+static void setup_per_branch_push(struct branch *branch)
+{
+ struct strbuf refspec = STRBUF_INIT;
+
+ strbuf_addf(refspec, %s:%s,
+ branch-name, branch-push_name);
+ add_refspec(refspec.buf);
This goes back to the question I raised in 3/6: If this code path adds
refspec foo:bar, and - say - setup_push_current() has already added
refspec foo:foo (or simply foo), then do we end up pushing into
foo or bar? To me, branch.*.push feels more specific than
push.default = current, so it would make sense that foo:bar
overrides foo:foo, but that is not obvious from the refspec alone.
IMHO, this definitely needs some tests.
+}
+
static int is_workflow_triagular(struct remote *remote)
{
struct remote *fetch_remote = remote_get(NULL);
@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
static void setup_default_push_refspecs(struct remote *remote)
{
struct branch *branch = branch_get(NULL);
- int triangular = is_workflow_triagular(remote);
+ int triangular;
+
+ if (branch-push_name) {
+ setup_per_branch_push(branch);
+ return;
I guess this return ensures that branch.*.push overrides push.default,
but might there be other sources of add_refspec() that would
complicate things?
+ }
+
+ triangular = is_workflow_triagular(remote);
switch (push_default) {
default:
diff --git a/remote.c b/remote.c
index e71f66d..e033fef 100644
--- a/remote.c
+++ b/remote.c
@@ -372,6 +372,11 @@ static int handle_config(const char *key, const char
*value, void *cb)
if (!value)
return config_error_nonbool(key);
add_merge(branch, xstrdup(value));
+ } else if (!strcmp(subkey, .push)) {
+ if (!value)
+ return config_error_nonbool(key);
+ free(branch-push_name);
+ branch-push_name = xstrdup(value);
}
return 0;
}
diff --git a/remote.h b/remote.h
index cf56724..84e0f72 100644
--- a/remote.h
+++ b/remote.h
@@ -138,6 +138,8 @@ struct branch {
struct refspec **merge;
int merge_nr;
int merge_alloc;
+
+ char *push_name;
};
struct branch *branch_get(const char *name);
Otherwise, this patch, and the entire series looks good to me.
...Johan
--
Johan Herland, jo...@herland.net
www.herland.net
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Johan Herland jo...@herland.net writes:
+static void setup_per_branch_push(struct branch *branch)
+{
+ struct strbuf refspec = STRBUF_INIT;
+
+ strbuf_addf(refspec, %s:%s,
+ branch-name, branch-push_name);
+ add_refspec(refspec.buf);
This goes back to the question I raised in 3/6: If this code path adds
refspec foo:bar, and - say - setup_push_current() has already added
refspec foo:foo (or simply foo), then do we end up pushing into
foo or bar?
I think you answered your own question below with the return.
@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
static void setup_default_push_refspecs(struct remote *remote)
{
struct branch *branch = branch_get(NULL);
- int triangular = is_workflow_triagular(remote);
+ int triangular;
+
+ if (branch-push_name) {
+ setup_per_branch_push(branch);
+ return;
I guess this return ensures that branch.*.push overrides push.default,
but might there be other sources of add_refspec() that would
complicate things?
The default-push-refspecs is meant to be used only when there is no
other stronger clue given by the user (e.g. refspec on the command
line, e.g. git push there master, pushing with configured refspecs
on remote.$name.push), so I think it is a bug if somebody calls this
function when there is other source.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Junio C Hamano wrote:
@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
static void setup_default_push_refspecs(struct remote *remote)
{
struct branch *branch = branch_get(NULL);
- int triangular = is_workflow_triagular(remote);
+ int triangular;
+
+ if (branch-push_name) {
+ setup_per_branch_push(branch);
+ return;
+ }
The most obvious question comes first: what result can I expect when
this interacts with remote.name.push?
Why did you design this feature like this? Will the user _not_ want
refspec mapping except when pushing out the current branch with a
plain git push?
Also, you managed to throw out all safety out the window. What
happens when the user does:
# on branch master, derived from origin
$ git push ram
And branch.master.push is set to next? Will you let her shoot herself
in the foot like this?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
@@ -194,7 +203,14 @@ static int is_workflow_triagular(struct remote *remote)
static void setup_default_push_refspecs(struct remote *remote)
{
struct branch *branch = branch_get(NULL);
- int triangular = is_workflow_triagular(remote);
+ int triangular;
+
+ if (branch-push_name) {
+ setup_per_branch_push(branch);
+ return;
+ }
The most obvious question comes first: what result can I expect when
this interacts with remote.name.push?
Now you bring it up, the branch.*.push may want to be more specific
(when I am on _this_ branch, do this) than remote.*.push (when I am
pushing to that remote, I want this to happen in general), but this
default codepath would not be exercised when you have remote.*.push,
so the logic may need to be moved higher up in the foodchain.
Also, you managed to throw out all safety out the window. What
happens when the user does:
# on branch master, derived from origin
$ git push ram
And branch.master.push is set to next? Will you let her shoot herself
in the foot like this?
It is not shooting in the foot, if branch.master.push is explicitly
set to update next. I do not see any issue in that part.
But the relative strength betweenh branch.*.push and remote.*.push
may need to be thought out. I haven't.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Junio C Hamano wrote: # on branch master, derived from origin $ git push ram And branch.master.push is set to next? Will you let her shoot herself in the foot like this? It is not shooting in the foot, if branch.master.push is explicitly set to update next. I do not see any issue in that part. The question does not pertain to master being mapped to next; it pertains to central-workflow versus triangular-workflow: origin versus ram. If the user has set push.default to upstream, she _expects_ triangular pushes to always be denied, and this is the first violation of that rule. I'm tilting towards building a dependency between branch.name.push and push.default. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
On Mon, Jun 24, 2013 at 4:19 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Junio C Hamano wrote: # on branch master, derived from origin $ git push ram And branch.master.push is set to next? Will you let her shoot herself in the foot like this? It is not shooting in the foot, if branch.master.push is explicitly set to update next. I do not see any issue in that part. The question does not pertain to master being mapped to next; it pertains to central-workflow versus triangular-workflow: origin versus ram. If the user has set push.default to upstream, she _expects_ triangular pushes to always be denied, and this is the first violation of that rule. I'm tilting towards building a dependency between branch.name.push and push.default. I haven't followed this topic closely, and the concern described below is probably a consequence of that. Please ignore if my worries are unfounded. It seems to me like we're adding (or have already added) quite a bit of config variables and command-line options for specifying (at varying levels of specificity and overridability) either the remote to push to ($remote), or what ref to push into on the remote ($remote_ref). It worries me that we allow $remote and $remote_ref to be set _separately_ and at separate levels of specificity. I wonder if this too easily allows users to shoot themselves in the foot by specifying $remote in one place (e.g. on the command line), then (in their mind - unrelated) specifying $remote_ref in another place (e.g. branch.foo.push), and then being negatively surprised when the two conspire to push into the wrong $remote and/or $remote_ref. I haven't yet dug deep enough to figure out an obvious failure mode (and I probably should not have sent this email until I'd found one), but I wonder if we'd be better off forcing the $remote and $remote_ref configuration for a given branch to appear as more of a single unit. What if, when setting up tracking for a given branch, we immediately specified its complete pull and push targets? For example, when in a centralized workflow (e.g. push.default = upstream) and we're checking out local branch foo from origin's foo, we could set up the following configuration [1]: [branch foo] pull = origin/foo push = origin/foo In a triangular workflow (assuming we had configuration to specify such, and also a default push remote), we could then instead set up the following config: [branch foo] pull = origin/foo push = my_public/foo This leaves no ambiguity for even the most novice user as to the pull and push targets for a given branch, and it's also easy to change it, either by editing the config file directly, or by using hypothetical commands: git branch foo --pulls-from=origin/bar git branch foo --pushes-to=other_repo/foo Any git pull without arguments will fail if branch.current.pull is unset or invalid. Likewise any git push without arguments will fail if branch.current.push is unset or invalid. Obviously, specifying the remote and/or refspec on the command-line would still override, as it does today, but for the argument-less forms of git pull and git push, the hierarchy of options and defaults being consulted to figure out $remote and $remote_ref would be small and easily understandable. ...Johan [1]: I do realize that I'm abusing the foo/bar notation to mean $remote/$ref, and that this does not work in the general case where both remote names and ref names may contain slashes, or when remote names don't correspond to eponymous ref namespaces... -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: # on branch master, derived from origin $ git push ram And branch.master.push is set to next? Will you let her shoot herself in the foot like this? It is not shooting in the foot, if branch.master.push is explicitly set to update next. I do not see any issue in that part. The question does not pertain to master being mapped to next; it pertains to central-workflow versus triangular-workflow: origin versus ram. If the user has set push.default to upstream, she _expects_ triangular pushes to always be denied,... If the user said git push without an explicit request to push to ram, and if branch.master.pushremote was not set to ram, and still the command git push pushed the branch to ram, then I would understand what you are worried about, but otherwise I do not see how what you are saying makes sense. A safety valve is different from a straight-jacket. If you are working largely with a central repository and have push.default set to upstream, are you now disallowed to push out things to other places to ask help from your colleague to check your wip? Why? Or are you saying that with push.default set to upstream, only these two forms should be allowed? $ git push ;# no destination, no refspec $ git push there ref:spec ;# both explicitly specified -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Johan Herland jo...@herland.net writes: I haven't yet dug deep enough to figure out an obvious failure mode (and I probably should not have sent this email until I'd found one), but I wonder if we'd be better off forcing the $remote and $remote_ref configuration for a given branch to appear as more of a single unit. That sounds sensible. I could see perhaps we would want to require, for branch.*.push to be effective, branch.*.pushremote must be set (honestly speaking, branch.*.push is not my itch and I'd probably be happier if we didn't add it in the first place, though ;-). What if, when setting up tracking for a given branch, we immediately specified its complete pull and push targets? For example, when in a centralized workflow (e.g. push.default = upstream) and we're checking out local branch foo from origin's foo, we could set up the following configuration [1]: [branch foo] pull = origin/foo push = origin/foo They should both be refs/heads/foo, as these are meant to name the name in _their_ repository. I see what you are saying, but the behaviour you want happens without branch.foo.push, and the addition may be redundant. I do not immediately see what it is buying us. Other than that when the user stops being centralized and starts to push to his own publishing repository, branch.*.push needs to be removed in addition to flipping push.default from upstream to current, that is. In a triangular workflow (assuming we had configuration to specify such, and also a default push remote), we could then instead set up the following config: [branch foo] pull = origin/foo push = my_public/foo Again, these are both refs/heads/foo. This leaves no ambiguity for even the most novice user as to the pull and push targets for a given branch, and it's also easy to change it, either by editing the config file directly, or by using hypothetical commands: git branch foo --pulls-from=origin/bar git branch foo --pushes-to=other_repo/foo But you need to do that for _all_ branches when you acquire your own publishing point; isn't that a rather cumbersome usability glitch? Obviously, specifying the remote and/or refspec on the command-line would still override, as it does today, but for the argument-less forms of git pull and git push, the hierarchy of options and defaults being consulted to figure out $remote and $remote_ref would be small and easily understandable. Not really. In addition to you need to run around and change configuration for all branches issue, you can never do push.default=matching, if you always set branch.foo.push and made it stronger than push.default, no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Junio C Hamano wrote: If the user said git push without an explicit request to push to ram, and if branch.master.pushremote was not set to ram, and still the command git push pushed the branch to ram, then I would understand what you are worried about, but otherwise I do not see how what you are saying makes sense. We currently have no system to differentiate between those two cases. A safety valve is different from a straight-jacket. If you are working largely with a central repository and have push.default set to upstream, are you now disallowed to push out things to other places to ask help from your colleague to check your wip? Why? I've been harping about how this safety valve is poorly done, and I'm the first person interested in loosening it. But I do not think this is the way to do it: drop in branch.name.push magic, and make upstream suddenly work with triangular workflows? Or are you saying that with push.default set to upstream, only these two forms should be allowed? $ git push ;# no destination, no refspec $ git push there ref:spec ;# both explicitly specified No, no. What I meant is: From the documentation of push.default, I _expect_ upstream to kick in only in the first case. In the second case, I _know_ that my push.default is inconsequential. With branch.name.push magic, you've sneaked in a push refspec under the carpet, and the first form suddenly doesn't care about push.default anymore. remote.name.push is also an under-the-carpet implementation that I don't like: it's done in push_with_options() which can completely bypass setup_default_push_refspecs(), shooting the user in the face. I want properly defined precedence and well-defined overall behavior. Not sneaky stuff. I don't have an answer, but will start a discussion with some code soon. In the meantime, since we've already figured out this series, merge it without 6/6. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: If the user said git push without an explicit request to push to ram, and if branch.master.pushremote was not set to ram, and still the command git push pushed the branch to ram, then I would understand what you are worried about, but otherwise I do not see how what you are saying makes sense. We currently have no system to differentiate between those two cases. I am not sure what two cases you are talking about? If you do not set anywhere (like branch.master.pushremote or remote.pushdefault) to push to ram, and if you did not say git push ram but just said git push, we will not push to ram (otherwise it is broken). So if the push is going to ram, the user must have asked us to push there, either via the command line, or these explicit configuration variables. And why do you need to differenciate between the command line ram and configured ram? After all, isn't the configuration merely a typesaver? If you know often push to ram, you configure to push there. If you don't, you don't. Or are you saying that with push.default set to upstream, only these two forms should be allowed? $ git push ;# no destination, no refspec $ git push there ref:spec ;# both explicitly specified No, no. What I meant is: From the documentation of push.default, I _expect_ upstream to kick in only in the first case. In the second case, I _know_ that my push.default is inconsequential. The push.default is meant to be in effect only when there is no other stronger clue from the user for what to update (e.g. command line refspec, remote.*.push). Because branch.*.push weatherbaloon patch did not have any documentation update, it did not say it is a yet another way to explicitly configure the push destination branch. Perhaps that led to your expectation for upstream to kick in. Of course, that requires, as you earlier pointed out, that the logic to read from branch.*.push need to be moved out of the push.default logic (in the weatherbaloon patch) and hosted to do_push() where it checks if there is remote-pushrefspec[]. If branch.*.push wants to defeat remote.*.push, then it should be checked before deciding to use that configured remote.*.push. I want properly defined precedence and well-defined overall behavior. Of course. There is no sneakiness. As I already said, I personally do not know what branch.*.push buys us, and will happy to drop 6/6. It is merely a weatherbaloon patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Junio C Hamano wrote:
If you do not set anywhere (like branch.master.pushremote or
remote.pushdefault) to push to ram, and if you did not say git
push ram but just said git push, we will not push to ram
(otherwise it is broken). So if the push is going to ram, the
user must have asked us to push there, either via the command line,
or these explicit configuration variables. And why do you need to
differenciate between the command line ram and configured ram?
After all, isn't the configuration merely a typesaver? If you know
often push to ram, you configure to push there. If you don't, you
don't.
Yes, you're talking about configuration variables in general. If the user says
$ git status
expecting long-form output, but gets short-form output, the world
hasn't ended. But if the user does
$ git push -f
expecting the push to go to one place, when it actually goes to
another place, she would have killed a few co-workers.
I'm not saying that we need to differentiate between configuration
variables and CLI options; what I _am_ saying is that we need to think
twice about moving a CLI option to a configuration variable, precisely
because we do not differentiate between the two cases.
As I have said earlier, my philosophy to prevent disasters does not
involve erroring-out or denying for the most part; rather it focuses
on giving the user immediate feedback like:
$ git push -f
# pushing master:next to ram (press ^C to abort)
Another form of feedback is the refspec @{p[ush]}, which I'm
struggling to complete.
The push.default is meant to be in effect only when there is no
other stronger clue from the user for what to update (e.g. command
line refspec, remote.*.push).
Yes, I know what it does currently. With your patch, the list becomes
push.default will be in effect only when there is no CLI refspec,
remote.name.push or branch.name.push in effect. I'm wondering if
there's a more elegant way to do it.
Of course, that requires, as you earlier pointed out, that the logic
to read from branch.*.push need to be moved out of the push.default
logic (in the weatherbaloon patch) and hosted to do_push() where it
checks if there is remote-pushrefspec[]. If branch.*.push wants to
defeat remote.*.push, then it should be checked before deciding to
use that configured remote.*.push.
I suspect there's a better way to do this.
As I already said, I personally do not know what branch.*.push buys
us, and will happy to drop 6/6. It is merely a weatherbaloon patch.
I hope we come out with a better version soon.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Ramkumar Ramachandra artag...@gmail.com writes: I'm not saying that we need to differentiate between configuration variables and CLI options; what I _am_ saying is that we need to think twice about moving a CLI option to a configuration variable, precisely because we do not differentiate between the two cases. With remote.pushdefault, I think the ship has long sailed. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] push: honor branch.*.push
Junio C Hamano wrote: With remote.pushdefault, I think the ship has long sailed. What's wrong with the early feedback approach I suggested? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
