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
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
On Mon, Jun 24, 2013 at 9:59 AM, Junio C Hamano wrote: > Johan Herland 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, 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
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,
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 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
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 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
On Mon, Jun 24, 2013 at 6:33 AM, Junio C Hamano wrote:
> From: Ramkumar Ramachandra
>
> When remote.pushdefault or branch..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
> Signed-off-by: Ramkumar Ramachandra
> Signed-off-by: Junio C Hamano
> ---
> 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:\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 'm
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
Junio C Hamano 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
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
Ramkumar Ramachandra 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 writes:
> Junio C Hamano 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
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
"Philip Oakley" writes: > From: "Junio C Hamano" > Sent: Thursday, June 20, 2013 8:23 PM >> Ramkumar Ramachandra 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
From: "Junio C Hamano" Sent: Thursday, June 20, 2013 8:23 PM Ramkumar Ramachandra 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
Ramkumar Ramachandra 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
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
Junio C Hamano 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:\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 P
Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows
Ramkumar Ramachandra writes:
> When remote.pushdefault or branch..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:\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
