Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano gits...@pobox.com wrote:
From: Ramkumar Ramachandra artag...@gmail.com
When remote.pushdefault or branch.name.pushremote is set (a triangular
workflow feature), master@{u} != origin, and push.default is set to
`upstream` or `simple`:
$ git push
fatal: You are pushing to remote 'origin', which is not the upstream of
your current branch 'master', without telling me what to push
to update which remote branch.
The very name of upstream indicates that it is only suitable for
use in central workflows; let us not even attempt to give it a new
meaning in triangular workflows, and error out as usual.
However, the `simple` does not have this problem: it is poised to be
the default for Git 2.0, and we would definitely like it to do
something sensible in triangular workflows.
Redefine simple as safer upstream for centralized workflow as
before, but work as current for triangular workflow.
An earlier round of this change by mistake broke the safety for
simple mode we have had since day 1 of that mode to make sure that
the branch in the repository we update is set to be the one we fetch
and integrate with, but it has been fixed.
Shouldn't there be an acompanying test to demonstrate this mistake being fixed?
Reported-by: Leandro Lucarella leandro.lucare...@sociomantic.com
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/config.txt | 10 +++---
builtin/push.c | 43 +++
2 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5d8ff1a..cae6870 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,9 +1848,13 @@ push.default::
pushing to the same repository you would normally pull from
(i.e. central workflow).
-* `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.
+* `simple` - in centralized workflow, work like `upstream` with an
+ added safety to refuse to push if the upstream branch's name is
+ different from the local one.
++
+When pushing to a remote that is different from the remote you normally
+pull from, work as `current`. This is the safest option and is suited
+for beginners.
+
This mode will become the default in Git 2.0.
diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..f6c8047 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,10 +120,11 @@ static const char message_detached_head_die[] =
\n
git push %s HEAD:name-of-remote-branch\n);
-static void setup_push_upstream(struct remote *remote, int simple)
+static void setup_push_upstream(struct remote *remote, struct branch *branch,
+ int triangular)
{
struct strbuf refspec = STRBUF_INIT;
- struct branch *branch = branch_get(NULL);
+
if (!branch)
die(_(message_detached_head_die), remote-name);
if (!branch-merge_nr || !branch-merge || !branch-remote_name)
@@ -137,18 +138,29 @@ static void setup_push_upstream(struct remote *remote,
int simple)
if (branch-merge_nr != 1)
die(_(The current branch %s has multiple upstream branches,
refusing to push.), branch-name);
- if (strcmp(branch-remote_name, remote-name))
+ if (triangular)
die(_(You are pushing to remote '%s', which is not the
upstream of\n
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);
+
+ if (push_default == PUSH_DEFAULT_SIMPLE) {
+ /* Additional safety */
+ if (strcmp(branch-refname, branch-merge[0]-src))
+ die_push_simple(branch, remote);
+ }
strbuf_addf(refspec, %s:%s, branch-name, branch-merge[0]-src);
add_refspec(refspec.buf);
}
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
Here (and above) we add a refspec to tell Git exactly what to push
from the local end, and into what on the remote end. Is it possible to
end up with multiple simultaneous refspecs matching the same local
ref, but mapping to different remote refs? If so, which will win, and
does that make sense?
+}
+
static char warn_unspecified_push_default_msg[] =
N_(push.default is unset; its implicit value is changing in\n
Git 2.0 from 'matching' to 'simple'. To squelch
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
Johan Herland jo...@herland.net writes:
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
Here (and above) we add a refspec to tell Git exactly what to push
from the local end, and into what on the remote end.
Correct.
Is it possible to end up with multiple simultaneous refspecs
matching the same local ref, but mapping to different remote refs?
Sorry, I don't follow. If you say push.default = current and you
do not give any other stronger clue (e.g. git push origin master
on the command line, or git push [origin] with remote.origin.push
configured), the above function is called and sets up your current
branch to be pushed to the same.
It is a bit more interesting for push.default = upstream, which is
for centralized workflow. If you forked frotz and nitfol branches
both from their master, e.g.
$ git checkout -t -b frotz origin/master
$ git checkout -t -b nitfol origin/master
after having worked on one of the branches, when you want to push it
back, the result of working on the topic branch goes back to master,
but I think that is what you want in the centralized workflow. If
it fast-forwards, you are fine, and if it does not, you will fetch
your upstream, i.e. their master, integrate your work with it, and
then push it back. At that point, you are playing the role of the
integrator of the shared master branch, because what you do on your
topic branch when you integrate others' work from master is exactly
that---you are not perfecting the theme you wanted to achieve on
your topic branch, but are integrating that result into shared
master to advance the overall state of the project. So pushing the
result back to 'master' makes perfect sense. After that, when you
have to restart your work on the other branch, you may first pull
--rebase before continuing, or you may just keep going with your
work based on a tad old origin/master. But when you finish working
on that topic and are about to push it out, you would be doing the
same tentatively don the central integrator's hat, and again it
makes sense to push the result to 'master'.
So in that sense, it is not which one wins. It is more like you
can push only after you become up to date, so there isn't one branch
overwriting the other one.
That is how I view it, anyway.
cf. http://git-blame.blogspot.com/2013/06/fun-with-various-workflows-1.html
+static int is_workflow_triagular(struct remote *remote)
s/triagular/triangular/
Thanks.
+{
+ struct remote *fetch_remote = remote_get(NULL);
+ return (fetch_remote fetch_remote != remote);
This changed from a strcmp() to a pointer compare. That might be safe,
depending on the sources of the two struct remote *, but I'm not sure.
Given the way remote_get() works, it should be correct, I think.
--
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 3/6] push: change `simple` to accommodate triangular workflows
Johan Herland wrote:
An earlier round of this change by mistake broke the safety for
simple mode we have had since day 1 of that mode to make sure that
the branch in the repository we update is set to be the one we fetch
and integrate with, but it has been fixed.
Shouldn't there be an acompanying test to demonstrate this mistake being
fixed?
Read earlier iteration: it didn't get merged.
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
Here (and above) we add a refspec to tell Git exactly what to push
from the local end, and into what on the remote end.
Nope, we add the refspec foo, without the :destination part. The
remote end is unspecified (and defaults to foo, but that is in the
transport layer).
Is it possible to
end up with multiple simultaneous refspecs matching the same local
ref, but mapping to different remote refs? If so, which will win, and
does that make sense?
It is impossible. We either:
- Get an explicit refspec from the user and never run
setup_default_push_refspecs() to begin with.
- Run setup_push_refspecs() and add *one* refspec depending on the
push.default value.
--
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 3/6] push: change `simple` to accommodate triangular workflows
Johan Herland jo...@herland.net writes: An earlier round of this change by mistake broke the safety for simple mode we have had since day 1 of that mode to make sure that the branch in the repository we update is set to be the one we fetch and integrate with, but it has been fixed. Shouldn't there be an acompanying test to demonstrate this mistake being fixed? An operation that has to expect failure due to safety was disabled by the broken version. The squashed end result reverts that change to the test, to make sure we did not break the safety. -- 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 3/6] push: change `simple` to accommodate triangular workflows
On Mon, Jun 24, 2013 at 9:59 AM, Junio C Hamano gits...@pobox.com wrote: Johan Herland jo...@herland.net writes: An earlier round of this change by mistake broke the safety for simple mode we have had since day 1 of that mode to make sure that the branch in the repository we update is set to be the one we fetch and integrate with, but it has been fixed. Shouldn't there be an acompanying test to demonstrate this mistake being fixed? An operation that has to expect failure due to safety was disabled by the broken version. The squashed end result reverts that change to the test, to make sure we did not break the safety. Ok, thanks. ...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 3/6] push: change `simple` to accommodate triangular workflows
On Mon, Jun 24, 2013 at 9:46 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Johan Herland wrote:
+static void setup_push_current(struct remote *remote, struct branch
*branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
Here (and above) we add a refspec to tell Git exactly what to push
from the local end, and into what on the remote end.
Nope, we add the refspec foo, without the :destination part. The
remote end is unspecified (and defaults to foo, but that is in the
transport layer).
Ok, so foo is not always semantically equivalent to foo:foo, and
when adding foo:bar it is always considered more specific than (and
superior to) foo. I think that makes sense.
Is it possible to
end up with multiple simultaneous refspecs matching the same local
ref, but mapping to different remote refs? If so, which will win, and
does that make sense?
It is impossible. We either:
- Get an explicit refspec from the user and never run
setup_default_push_refspecs() to begin with.
- Run setup_push_refspecs() and add *one* refspec depending on the
push.default value.
Thanks, that's what I wanted to hear. But then, does it make sense to
say that we will only ever have exactly _one_ push refspec in the
current context, and we should therefore replace the static const
char **refspec; string array with a single static const char
*refspec; string? That would make it obvious that there is no room
for ambiguity with overlapping refspecs.
...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 3/6] push: change `simple` to accommodate triangular workflows
Johan Herland wrote: But then, does it make sense to say that we will only ever have exactly _one_ push refspec in the current context, and we should therefore replace the static const char **refspec; string array with a single static const char *refspec; string? That would make it obvious that there is no room for ambiguity with overlapping refspecs. Multiple refspecs can be specified on the command-line; set_refspecs() is responsible for calling add_refspec() multiple times for each refspec, and _that_ is the primary use of the refspec variable. The single add_refspec() invocation in the push.default switch is a special case that reuses the variable. -- 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
[PATCH 3/6] push: change `simple` to accommodate triangular workflows
From: Ramkumar Ramachandra artag...@gmail.com
When remote.pushdefault or branch.name.pushremote is set (a triangular
workflow feature), master@{u} != origin, and push.default is set to
`upstream` or `simple`:
$ git push
fatal: You are pushing to remote 'origin', which is not the upstream of
your current branch 'master', without telling me what to push
to update which remote branch.
The very name of upstream indicates that it is only suitable for
use in central workflows; let us not even attempt to give it a new
meaning in triangular workflows, and error out as usual.
However, the `simple` does not have this problem: it is poised to be
the default for Git 2.0, and we would definitely like it to do
something sensible in triangular workflows.
Redefine simple as safer upstream for centralized workflow as
before, but work as current for triangular workflow.
An earlier round of this change by mistake broke the safety for
simple mode we have had since day 1 of that mode to make sure that
the branch in the repository we update is set to be the one we fetch
and integrate with, but it has been fixed.
Reported-by: Leandro Lucarella leandro.lucare...@sociomantic.com
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/config.txt | 10 +++---
builtin/push.c | 43 +++
2 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5d8ff1a..cae6870 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,9 +1848,13 @@ push.default::
pushing to the same repository you would normally pull from
(i.e. central workflow).
-* `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.
+* `simple` - in centralized workflow, work like `upstream` with an
+ added safety to refuse to push if the upstream branch's name is
+ different from the local one.
++
+When pushing to a remote that is different from the remote you normally
+pull from, work as `current`. This is the safest option and is suited
+for beginners.
+
This mode will become the default in Git 2.0.
diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..f6c8047 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,10 +120,11 @@ static const char message_detached_head_die[] =
\n
git push %s HEAD:name-of-remote-branch\n);
-static void setup_push_upstream(struct remote *remote, int simple)
+static void setup_push_upstream(struct remote *remote, struct branch *branch,
+ int triangular)
{
struct strbuf refspec = STRBUF_INIT;
- struct branch *branch = branch_get(NULL);
+
if (!branch)
die(_(message_detached_head_die), remote-name);
if (!branch-merge_nr || !branch-merge || !branch-remote_name)
@@ -137,18 +138,29 @@ static void setup_push_upstream(struct remote *remote,
int simple)
if (branch-merge_nr != 1)
die(_(The current branch %s has multiple upstream branches,
refusing to push.), branch-name);
- if (strcmp(branch-remote_name, remote-name))
+ if (triangular)
die(_(You are pushing to remote '%s', which is not the
upstream of\n
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);
+
+ if (push_default == PUSH_DEFAULT_SIMPLE) {
+ /* Additional safety */
+ if (strcmp(branch-refname, branch-merge[0]-src))
+ die_push_simple(branch, remote);
+ }
strbuf_addf(refspec, %s:%s, branch-name, branch-merge[0]-src);
add_refspec(refspec.buf);
}
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
+}
+
static char warn_unspecified_push_default_msg[] =
N_(push.default is unset; its implicit value is changing in\n
Git 2.0 from 'matching' to 'simple'. To squelch this message\n
@@ -173,9 +185,16 @@ static void
warn_unspecified_push_default_configuration(void)
warning(%s\n, _(warn_unspecified_push_default_msg));
}
+static int is_workflow_triagular(struct remote *remote)
+{
+ struct remote *fetch_remote = remote_get(NULL);
+ return (fetch_remote fetch_remote != remote);
+}
+
static void setup_default_push_refspecs(struct remote *remote)
{
- struct branch *branch;
+ struct branch *branch = branch_get(NULL);
+ int triangular = is_workflow_triagular(remote);
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano wrote:
Decouple `simple` from `upstream` completely, and change it to mean
`current` with a safety feature: a `push` and `pull` should not be
asymmetrical in the special case of central workflows.
Double negation confused my parser. 'push' and 'pull' should be
kept symmetrical in central workflows?
They're not the same thing. It is very much intentional and intended:
the safety net is not to ensure that the push and pull are
symmetrical (i.e. among other things, error out if
branch.$branch.merge is unset), but rather ensure that the push and
pull are never asymmetrical.
Without any configuration the current branch is pushed out, which
loosens the safety we implemented in the current 'safer upstream'.
I am not convinced this is a good change. I am not convinced this is
a bad change, either, yet, but this loosening smells bad.
Provided that we would want to keep the Push the current one to the
same name but you have to have it set up as your integration source
safety for central workflow (which I am starting to think we
should), we would want something like this on top of your entire
series, I think. The behaviour change can be seen in the revert of
one test you made to the test that expects simple to fail due to
the safety.
Now I'd like to question what you are labelling as safety. What is
the consequence of erroring out when branch.$branch.merge is unset
when pushing using `upstream`? For me, it only means additional
inconvenience: any new branches I create can't be pushed out without
explicitly setting branch.$branch.merge to an invalid value. What
is invalid about it? The fact that it doesn't exist, @{u} still
doesn't resolve, and git branch -u doesn't work. Hell, even git push
-u doesn't work! So, what is this huge safety that can justify
inconveniencing me like this? By making sure that
branch.$branch.merge is set, my prompt responds immediately to
divergence, and this is awesome. Predictably, I use git push -u when
I push out a new branch with `current`. So, unless you have a damn
good reason to inconvenience me in the name of safety,
branch.$branch.merge should default to refs/heads/$branch, unless set
explicitly.
I didn't want to contaminate this series with an unrelated improvement
to `upstream`, which is why you don't see the change here: it is
orthogonal to designing a good `simple`, and I only brought it up to
question the safety you're carrying over to `simple`; what
obligation does `simple` have to carry over this feature? I've made
it clear that I want a clean break from `upstream`, and I find your
proposal is very inelegant: `simple` has two modes of operation; when
branch.$branch.remote is equal to $pushremote, branch.$branch.merge
must be set and equal to $branch (the `upstream` mode); when
branch.$branch.remote is unequal to $pushremote, don't care about
whether branch.$branch.merge is set (the `current` mode). My proposal
is much smoother than this modal operation, 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 3/6] push: change `simple` to accommodate triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Decouple `simple` from `upstream` completely, and change it to mean `current` with a safety feature: a `push` and `pull` should not be asymmetrical in the special case of central workflows. Double negation confused my parser. 'push' and 'pull' should be kept symmetrical in central workflows? They're not the same thing. It is very much intentional and intended: the safety net is not to ensure that the push and pull are symmetrical (i.e. among other things, error out if branch.$branch.merge is unset), but rather ensure that the push and pull are never asymmetrical. H not to ensure that the push and pull are symmetrical rather ensure that the push and pull are never asymmetrical. They still talk the same thing to me. What am I missing? Am I being clueless, or is there something else going on? Your among other things, after reading it three times, unfortunately did not help clarify anything to me, so perhaps somebody other than me (or you for that matter) who is more clueful can help find a different way to explain the difference you are trying to express to me. Help, anybody? Provided that we would want to keep the Push the current one to the same name but you have to have it set up as your integration source safety for central workflow (which I am starting to think we should), we would want something like this on top of your entire series, I think. The behaviour change can be seen in the revert of one test you made to the test that expects simple to fail due to the safety. Now I'd like to question what you are labelling as safety. What is the consequence of erroring out when branch.$branch.merge is unset when pushing using `upstream`? Nothing noteworthy. I wasn't suggesting to change what `upstream` does at all. If you do not have a `upstream` configured, we do not know what branch you are integrating with, and the operation has failed in the code even before we started adding triangular. I do not see a reason to change that in the triangular world; `upstream` is about the central workflow as you originally wrote in the config.txt patch in this series. The name of the branch the repository you fetch from and integrate with does not have anything to do with the name you want to push your derived work as to a different repository I thought we already discussed and agreed on this point. http://thread.gmane.org/gmane.comp.version-control.git/227028/focus=227313 The conclusion is that using push.default=`upstream` is meaningless when you are using a triangular workflow. If you are using a centralized workflow, and if a branch does not have branch.*.merge configured, we do not know to which branch you are pushing it back, so we error out. What I am changing from the patch you posted with the how about this on top patch back to the current behaviour is what 'simple' does for centralized workflow. I didn't want to contaminate this series with an unrelated improvement to `upstream` I think we share that, and it is not just `upstream`, but also `simple` when it is applied to folks who employ a centralized workflow. The safety we need to keep is the one we have had since day one of introducing 'simple' for them. When you are doing a centralized workflow, 'simple' was defined to be 'upstream' with added restriction that the branch at the remote you integrate with must have the same name as the current branch you are pushing, i.e. in [branch frotz] merge = refs/heads/$branch the value of $branch must be 'frotz'; otherwise 'simple' errors out. http://thread.gmane.org/gmane.comp.version-control.git/194175/focus=196199 Now, no existing series has casted in stone the best behaviour for `simple` in a triangular workflow. With this series (and also with my fixup patch I sent last night), it is defined to act as `current`, but it may need a bit more safety to help new users avoid pushing branches they did not intend to (perhaps pushing out `current` only when the branch with the same name already exists at the destination? I dunno). -- 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 3/6] push: change `simple` to accommodate triangular workflows
From: Junio C Hamano gits...@pobox.com Sent: Thursday, June 20, 2013 8:23 PM Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Decouple `simple` from `upstream` completely, and change it to mean `current` with a safety feature: a `push` and `pull` should not be asymmetrical in the special case of central workflows. Double negation confused my parser. 'push' and 'pull' should be kept symmetrical in central workflows? They're not the same thing. It is very much intentional and intended: the safety net is not to ensure that the push and pull are symmetrical (i.e. among other things, error out if branch.$branch.merge is unset), but rather ensure that the push and pull are never asymmetrical. H not to ensure that the push and pull are symmetrical rather ensure that the push and pull are never asymmetrical. They still talk the same thing to me. What am I missing? Am I being clueless, or is there something else going on? I think it is a case of the user having explicitly set push=Africa and pull=Europe which can't be a setting for simple symmetry. But then again I haven't been following the fine details. (that is there are some defaults that allow stuff to be half set) Your among other things, after reading it three times, unfortunately did not help clarify anything to me, so perhaps somebody other than me (or you for that matter) who is more clueful can help find a different way to explain the difference you are trying to express to me. Help, anybody? Philip [...] -- 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 3/6] push: change `simple` to accommodate triangular workflows
Philip Oakley philipoak...@iee.org writes: From: Junio C Hamano gits...@pobox.com Sent: Thursday, June 20, 2013 8:23 PM Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Double negation confused my parser. 'push' and 'pull' should be kept symmetrical in central workflows? They're not the same thing. It is very much intentional and intended: the safety net is not to ensure that the push and pull are symmetrical (i.e. among other things, error out if branch.$branch.merge is unset), but rather ensure that the push and pull are never asymmetrical. H not to ensure that the push and pull are symmetrical rather ensure that the push and pull are never asymmetrical. They still talk the same thing to me. What am I missing? Am I being clueless, or is there something else going on? I think it is a case of the user having explicitly set push=Africa and pull=Europe which can't be a setting for simple symmetry. Yeah but then that is not a discussion about central workflow. I can understand In a central workflow push and pull should be symmetrical. I can also, with a bit of double-negation brain twisting, understand In a central workflow, push and pull should not be asymmetrical. But when I suggest to avoid double-negation, I was told that these two statements mean different things, and the original should not be rewritten to avoid double-negation, which is where my brain stopped and asked for help. -- 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 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano wrote:
They're not the same thing. It is very much intentional and intended:
the safety net is not to ensure that the push and pull are
symmetrical (i.e. among other things, error out if
branch.$branch.merge is unset), but rather ensure that the push and
pull are never asymmetrical.
not to ensure that the push and pull are symmetrical
rather ensure that the push and pull are never asymmetrical.
They still talk the same thing to me. What am I missing?
Never mind the wording then. I am proposing preventing asymmetry by
explicitly disallowing a push when $branch is different from
branch.$branch.merge, instead of shutting down immediately when
branch.$branch.merge is unset.
Now I'd like to question what you are labelling as safety. What is
the consequence of erroring out when branch.$branch.merge is unset
when pushing using `upstream`?
Nothing noteworthy.
I wasn't suggesting to change what `upstream` does at all.
No, but I did. I just argued for a sane default for
branch.$branch.merge (the part you snipped out).
The conclusion is that using push.default=`upstream` is meaningless
when you are using a triangular workflow.
Yes, and I agreed with that.
If you are using a
centralized workflow, and if a branch does not have branch.*.merge
configured, we do not know to which branch you are pushing it back,
so we error out.
And I said: have a sane default.
What I am changing from the patch you posted with the how about
this on top patch back to the current behaviour is what 'simple'
does for centralized workflow.
Yes, I know. I read the patch.
When you are doing a centralized workflow, 'simple' was defined
Again, I'm well aware what it _was_ defined as. Was it not clear that
I argued for a change from the very first email where I posted the
patch and changed a test? Do you have a counter-argument, or is it
simply that we must respect its historical meaning?
Now, no existing series has casted in stone the best behaviour for
`simple` in a triangular workflow. With this series (and also with
my fixup patch I sent last night), it is defined to act as `current`,
but it may need a bit more safety to help new users avoid pushing
branches they did not intend to (perhaps pushing out `current` only
when the branch with the same name already exists at the destination?
I dunno).
I see no reason to plan safety features in advance, especially since
we have neither branch.$branch.push nor @{push} yet.
--
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 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano gits...@pobox.com writes:
Junio C Hamano gits...@pobox.com writes:
Without any configuration the current branch is pushed out, which
loosens the safety we implemented in the current 'safer upstream'.
I am not convinced this is a good change. I am not convinced this is
a bad change, either, yet, but this loosening smells bad.
Provided that we would want to keep the Push the current one to the
same name but you have to have it set up as your integration source
safety for central workflow (which I am starting to think we
should), we would want something like this on top of your entire
series, I think. The behaviour change can be seen in the revert of
one test you made to the test that expects simple to fail due to
the safety.
And with the small refactoring of setup_default_push_refspecs (the
important part being that we grab branch in this function, not in
its helper functions per push.default mode), branch.*.push can fall
out rather naturally, like this patch on top.
A footnote unrelated to this patch.
The function is_workflow_triangular() in the how about this patch
needs to be tweaked from the version I am responding to, in order to
take the case where fetch-remote is not defined into account, i.e.
static int is_workflow_triagular(struct remote *remote)
{
struct remote *fetch_remote = remote_get(NULL);
return (fetch_remote fetch_remote != remote);
}
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);
+}
+
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;
+ }
+
+ 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);
--
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 3/6] push: change `simple` to accommodate triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: They're not the same thing. It is very much intentional and intended: the safety net is not to ensure that the push and pull are symmetrical (i.e. among other things, error out if branch.$branch.merge is unset), but rather ensure that the push and pull are never asymmetrical. not to ensure that the push and pull are symmetrical rather ensure that the push and pull are never asymmetrical. They still talk the same thing to me. What am I missing? Never mind the wording then. I am proposing preventing asymmetry by explicitly disallowing a push when $branch is different from branch.$branch.merge, instead of shutting down immediately when branch.$branch.merge is unset. We always said upstream is to update the branch you fetch and integrate with, and tried to make sure the push destination is the branch you configured the current branch (i.e. the one you are trying to push out) to fetch and integrate with. That is how we prevent asymmetry. We fail if branch.$branch.merge is set to something else. We also fail if branch.$branch.merge is *not* set, because by definition the branch you are trying to push to in that scenario cannot be the branch you fetch and integrat with by git pull [--rebase]. I know your patch was attempting to change the behaviour for the latter. You are trying to let anything go if branch.*.merge is not set. How would it help prevent assymetry? Now I'd like to question what you are labelling as safety. What is the consequence of erroring out when branch.$branch.merge is unset when pushing using `upstream`? Nothing noteworthy. I wasn't suggesting to change what `upstream` does at all. No, but I did Really? Then where did this come from? I didn't want to contaminate this series with an unrelated improvement to `upstream` If you are using a centralized workflow, and if a branch does not have branch.*.merge configured, we do not know to which branch you are pushing it back, so we error out. And I said: have a sane default. Like you said, I do not want to contaminate this series with such an unrelated change. Worse, you are trying to break a sane default by replacing it with anything goes. We already have a sane default, which is to error out. We do not need your broken 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 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano gits...@pobox.com writes: Like you said, I do not want to contaminate this series with such an unrelated change. Worse, you are trying to break a sane default by replacing it with anything goes. We already have a sane default, which is to error out. We do not need your broken default. This came out as a bit stronger than I wanted to. Add to the last: Even if we later find out that changing the default to loosen it to anything goes is a good idea, I do not think it belongs to this series. -- 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
[PATCH 3/6] push: change `simple` to accommodate triangular workflows
When remote.pushdefault or branch.name.pushremote is set (a triangular
workflow feature), master@{u} != origin, and push.default is set to
`upstream` or `simple`:
$ git push
fatal: You are pushing to remote 'origin', which is not the upstream of
your current branch 'master', without telling me what to push
to update which remote branch.
Unfortunately, in the case of `upstream`, the very name indicates that
it is only suitable for use in central workflows; let us not even
attempt to give it a new meaning in triangular workflows, and error out
as usual. However, the `simple` does not have this problem: it is
poised to be the default for Git 2.0, and we would definitely like it to
do something sensible in triangular workflows.
Decouple `simple` from `upstream` completely, and change it to mean
`current` with a safety feature: a `push` and `pull` should not be
asymmetrical in the special case of central workflows.
Reported-by: Leandro Lucarella leandro.lucare...@sociomantic.com
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Documentation/config.txt | 10 ++
builtin/push.c | 21 -
t/t5528-push-default.sh | 2 +-
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9f04f74..81628e8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1850,10 +1850,12 @@ push.default::
symmetrical to `pull` in central workflows, and cannot be used in
non-central workflows.
-* `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. It will become the default
- in Git 2.0.
+* `simple` - a safer version of `current`; push the current branch to
+ update a branch with the same name on the receiving end, with a
+ safety feature: in central workflows, error out if
+ branch.$branch.merge is set and not equal to $branch, to make sure
+ that a `push` and `push` are never asymmetrical. It will become the
+ default in Git 2.0.
* `matching` - push all branches having the same name on both ends
(essentially ignoring all newly created local branches).
diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..d8d27d9 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,6 +120,25 @@ static const char message_detached_head_die[] =
\n
git push %s HEAD:name-of-remote-branch\n);
+static void setup_push_simple(struct remote *remote)
+{
+ struct branch *branch = branch_get(NULL);
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ if (!branch-merge_nr || !branch-merge || !branch-remote_name)
+ /* No upstream configured */
+ goto end;
+ if (branch-merge_nr != 1)
+ die(_(The current branch %s has multiple upstream branches,
+ refusing to push.), branch-name);
+ if (!strcmp(branch-remote_name, remote-name)
+ strcmp(branch-refname, branch-merge[0]-src))
+ /* Central workflow safety feature */
+ die_push_simple(branch, remote);
+end:
+ add_refspec(branch-name);
+}
+
static void setup_push_upstream(struct remote *remote, int simple)
{
struct strbuf refspec = STRBUF_INIT;
@@ -188,7 +207,7 @@ static void setup_default_push_refspecs(struct remote
*remote)
break;
case PUSH_DEFAULT_SIMPLE:
- setup_push_upstream(remote, 1);
+ setup_push_simple(remote);
break;
case PUSH_DEFAULT_UPSTREAM:
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 69ce6bf..e54dd02 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -85,7 +85,7 @@ test_expect_success 'push from/to new branch with current
creates remote branch'
test_expect_success 'push to existing branch, with no upstream configured' '
test_config branch.master.remote repo1
git checkout master
- test_push_failure simple
+ test_push_success simple master
test_push_failure upstream
'
--
1.8.3.1.454.g30263f3.dirty
--
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 3/6] push: change `simple` to accommodate triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes:
When remote.pushdefault or branch.name.pushremote is set (a triangular
workflow feature), master@{u} != origin, and push.default is set to
`upstream` or `simple`:
$ git push
fatal: You are pushing to remote 'origin', which is not the upstream of
your current branch 'master', without telling me what to push
to update which remote branch.
Unfortunately, in the case of `upstream`, the very name indicates that
it is only suitable for use in central workflows; let us not even
attempt to give it a new meaning in triangular workflows, and error out
as usual.
Sensible.
However, the `simple` does not have this problem: it is poised to
be the default for Git 2.0, and we would definitely like it to do
something sensible in triangular workflows.
Decouple `simple` from `upstream` completely, and change it to mean
`current` with a safety feature: a `push` and `pull` should not be
asymmetrical in the special case of central workflows.
Double negation confused my parser. 'push' and 'pull' should be
kept symmetrical in central workflows?
+* `simple` - a safer version of `current`; push the current branch to
+ update a branch with the same name on the receiving end, with a
+ safety feature: in central workflows, error out if
+ branch.$branch.merge is set and not equal to $branch,
If branch.$branch.merge is _not_ set, what happens in the current
code, and what should happen?
+ to make sure
+ that a `push` and `push` are never asymmetrical. It will become the
+ default in Git 2.0.
Ditto.
* `matching` - push all branches having the same name on both ends
(essentially ignoring all newly created local branches).
diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..d8d27d9 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,6 +120,25 @@ static const char message_detached_head_die[] =
\n
git push %s HEAD:name-of-remote-branch\n);
+static void setup_push_simple(struct remote *remote)
+{
+ struct branch *branch = branch_get(NULL);
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
OK.
+ if (!branch-merge_nr || !branch-merge || !branch-remote_name)
+ /* No upstream configured */
+ goto end;
Without any configuration the current branch is pushed out, which
loosens the safety we implemented in the current 'safer upstream'.
I am not convinced this is a good change. I am not convinced this is
a bad change, either, yet, but this loosening smells bad.
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 69ce6bf..e54dd02 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -85,7 +85,7 @@ test_expect_success 'push from/to new branch with current
creates remote branch'
test_expect_success 'push to existing branch, with no upstream configured' '
test_config branch.master.remote repo1
git checkout master
- test_push_failure simple
+ test_push_success simple master
test_push_failure upstream
'
Likewise.
--
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 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano gits...@pobox.com writes:
Without any configuration the current branch is pushed out, which
loosens the safety we implemented in the current 'safer upstream'.
I am not convinced this is a good change. I am not convinced this is
a bad change, either, yet, but this loosening smells bad.
Provided that we would want to keep the Push the current one to the
same name but you have to have it set up as your integration source
safety for central workflow (which I am starting to think we
should), we would want something like this on top of your entire
series, I think. The behaviour change can be seen in the revert of
one test you made to the test that expects simple to fail due to
the safety.
This patch is somewhat minimal in that it does not address other
issues I raised in the review of the series; it only addresses the
simple must be safe issue.
builtin/push.c | 60 +
t/t5528-push-default.sh | 2 +-
2 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 783bacf..84c4a90 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,29 +120,11 @@ static const char message_detached_head_die[] =
\n
git push %s HEAD:name-of-remote-branch\n);
-static void setup_push_simple(struct remote *remote)
-{
- struct branch *branch = branch_get(NULL);
- if (!branch)
- die(_(message_detached_head_die), remote-name);
- if (!branch-merge_nr || !branch-merge || !branch-remote_name)
- /* No upstream configured */
- goto end;
- if (branch-merge_nr != 1)
- die(_(The current branch %s has multiple upstream branches,
- refusing to push.), branch-name);
- if (!strcmp(branch-remote_name, remote-name)
- strcmp(branch-refname, branch-merge[0]-src))
- /* Central workflow safety feature */
- die_push_simple(branch, remote);
-end:
- add_refspec(branch-name);
-}
-
-static void setup_push_upstream(struct remote *remote)
+static void setup_push_upstream(struct remote *remote, struct branch *branch,
+ int triangular)
{
struct strbuf refspec = STRBUF_INIT;
- struct branch *branch = branch_get(NULL);
+
if (!branch)
die(_(message_detached_head_die), remote-name);
if (!branch-merge_nr || !branch-merge || !branch-remote_name)
@@ -156,16 +138,29 @@ static void setup_push_upstream(struct remote *remote)
if (branch-merge_nr != 1)
die(_(The current branch %s has multiple upstream branches,
refusing to push.), branch-name);
- if (strcmp(branch-remote_name, remote-name))
+ if (triangular)
die(_(You are pushing to remote '%s', which is not the
upstream of\n
your current branch '%s', without telling me what to
push\n
to update which remote branch.),
remote-name, branch-name);
+ if (push_default == PUSH_DEFAULT_SIMPLE) {
+ /* Additional safety */
+ if (strcmp(branch-refname, branch-merge[0]-src))
+ die_push_simple(branch, remote);
+ }
+
strbuf_addf(refspec, %s:%s, branch-name, branch-merge[0]-src);
add_refspec(refspec.buf);
}
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+ if (!branch)
+ die(_(message_detached_head_die), remote-name);
+ add_refspec(branch-name);
+}
+
static char warn_unspecified_push_default_msg[] =
N_(push.default is unset; its implicit value is changing in\n
Git 2.0 from 'matching' to 'simple'. To squelch this message\n
@@ -190,9 +185,16 @@ static void
warn_unspecified_push_default_configuration(void)
warning(%s\n, _(warn_unspecified_push_default_msg));
}
+static int is_workflow_triagular(struct remote *remote)
+{
+ struct remote *fetch_remote = remote_get(NULL);
+ return (fetch_remote != remote);
+}
+
static void setup_default_push_refspecs(struct remote *remote)
{
- struct branch *branch;
+ struct branch *branch = branch_get(NULL);
+ int triangular = is_workflow_triagular(remote);
switch (push_default) {
default:
@@ -205,18 +207,18 @@ static void setup_default_push_refspecs(struct remote
*remote)
break;
case PUSH_DEFAULT_SIMPLE:
- setup_push_simple(remote);
+ if (triangular)
+ setup_push_current(remote, branch);
+ else
+ setup_push_upstream(remote, branch, triangular);
break;
case PUSH_DEFAULT_UPSTREAM:
- setup_push_upstream(remote);
+ setup_push_upstream(remote, branch, triangular);
break;
case PUSH_DEFAULT_CURRENT:
-
