Re: [PATCH] config doc: rewrite push.default section

Ramkumar Ramachandra [off-list ref] writes:

Design by Junio.

Not necessary.  The conclusion of discussion is a result of
collaboration.

Thanks for writing it down.  It is a good start, but I agree with
reviews by Philip Oakley and Matthieu Moy we already saw.  

 - To understand "if central, works as upstream, otherwise works as
   current", the readers need to know if their workflow is 'central'
   or not, so we need to say how that is decided upfront (probably
   immediately before "Possible values are:" in the introductory
   paragraph for push.default;

 - For each of these choices, what it means is more important to the
   readers than how the implementation achieves that semantics
   (e.g. "current uses refs/heads/foo:refs/heads/foo when you are on
   foo branch"). I think the ideal is a description of the meaning
   that is clear enough not to require any implementation detail.

I do no think the remainder (snipped) belongs to the log message.

As this is an attempt to _define_ the semantics of what should
happen in triangular workflow, I think it would be healthy to wait
for a week or so in order for others (not just two of us) can see if
we have defined a sane semantics we would want to go forward with.
I am reasonably sure 'current' would be the best default for
triangular (and that is why I suggested 'simple' to fall back to
it), but I do not think others had a chance to see what design our
discussion settled, think if that makes sense, and think of a better
alternative.

+* `nothing` - error out unless a refspec is explicitly given.

I do not think 'error out' is the primary effect of this mode.
Wouldn't "do not push anything" be much better?

+* `current` - push the refspec "$HEAD".  HEAD is resolved early to a
+  branch name (referred to as $HEAD).  In other words, push the
+  current branch to update a branch with the same name on the pushing
+  side.

As already pointed out, dropping everything before and including "In
other words, " would be better.  Also "push the current branch" is
talking about the branch on the pushing side (you, the one who is
running "git push"), and the side that is getting updated is at the
receiving end, not "pushing side".

+* `upstream` - push the refspec "$HEAD:branch.$HEAD.merge", and error
+  out if the push destination is not the same as branch.$HEAD.remote.
+  The name "upstream" refers to the revision "@{u[pstream]}" in
+  linkgit:gitrevisions[7].  It is useful in central workflows, to make
+  the `push` symmetrical to `pull`.
+
+* `simple` - in central workflows, behaves like `upstream`, except
+  that it errors out unless branch.$HEAD.merge is equal to $HEAD.  In
+  non-central workflows, behaves like `current`.  It will become the
+  default in Git 2.0.

I think listing 'simple' at the end would be easier to read.  The
above already swaps the order to make sure current and upstream are
described before it, which is good.

But I do not see a reason to move 'matching' which was next to
'nothing' here. The 

+* `matching` - push the refspec ":".  In other words, push all
+  branches having the same name in both ends, even if it means
+  non-fast-forward updates.  This is for those who prepare all the
+  branches into a publishable shape and then push them out with a
+  single command.  Dangerous, and inappropriate unless you are the
+  only person updating your push destination.

It was already pointed out that unnecessary negativity needs to be
fixed, but more importantly the above "Dangerous" is not even
correct.

If you are "I am done with this one branch, I want to push it out,
the other branches I have touched but they are not yet in a good
shape to be pushed out" kind of person, it does not matter if you
are the only one who is pushing to the destination.  You would end
up pushing out half-baked ones as well if you used 'matching', which
is not what you want.

-+
-The `simple`, `current` and `upstream` modes are for those who want to
-push out a single branch after finishing work, even when the other
-branches are not yet ready to be pushed out. If you are working with
-other people to push into the same shared repository, you would want
-to use one of these.

And I do not see a reason to drop this part, especially the first
sentence.

Junio C Hamano wrote:

I do no think the remainder (snipped) belongs to the log message.

Oh, it was never intended to be a proper commit message.  I'll write a
proper one when I send it in with the implementation.

 - To understand "if central, works as upstream, otherwise works as
   current", the readers need to know if their workflow is 'central'
   or not, so we need to say how that is decided upfront (probably
   immediately before "Possible values are:" in the introductory
   paragraph for push.default;

Good idea.

 - For each of these choices, what it means is more important to the
   readers than how the implementation achieves that semantics
   (e.g. "current uses refs/heads/foo:refs/heads/foo when you are on
   foo branch"). I think the ideal is a description of the meaning
   that is clear enough not to require any implementation detail.

I'd definitely like to include the push refspec, to educate users on
how to achieve a one-off matching-push even when push.default is set
to current, for example.

As this is an attempt to _define_ the semantics of what should
happen in triangular workflow, I think it would be healthy to wait
for a week or so in order for others (not just two of us) can see if
we have defined a sane semantics we would want to go forward with.

Agreed.

I am reasonably sure 'current' would be the best default for
triangular (and that is why I suggested 'simple' to fall back to
it), but I do not think others had a chance to see what design our
discussion settled, think if that makes sense, and think of a better
alternative.

You didn't realize one thing, because my wording was terrible:
`simple` is just a "safe" version of `current` :)

* `simple` - behaves exactly like `current`, with one safety feature:
in central workflows, it errors out if branch.$branch.merge is set and
is not equal to $branch (to make sure that the `push` and `pull`
aren't asymmetrical).

In other words, it's `current` with the safety feature of `upstream`.
There's no point in erroring out if no upstream is set, or mentioning
its relationship with `upstream`.

quoted

+* `nothing` - error out unless a refspec is explicitly given.

I do not think 'error out' is the primary effect of this mode.
Wouldn't "do not push anything" be much better?

"Do not push anything" is misleading because it implies an exit status
of zero; what is really does is "error out", no?

quoted

+* `current` - push the refspec "$HEAD".  HEAD is resolved early to a
+  branch name (referred to as $HEAD).  In other words, push the
+  current branch to update a branch with the same name on the pushing
+  side.

As already pointed out, dropping everything before and including "In
other words, " would be better.

I'd like to put it in the end, like Matthieu suggested.

Also "push the current branch" is
talking about the branch on the pushing side (you, the one who is
running "git push"), and the side that is getting updated is at the
receiving end, not "pushing side".

"receiving end" is better, yes.

I think listing 'simple' at the end would be easier to read.  The
above already swaps the order to make sure current and upstream are
described before it, which is good.

As I pointed out, `simple` is just a composition of `current` and
`upstream`.  That's why it comes after them.  Also, I thought
`nothing`, `current`, `upstream` were fundamental: which is why they
come first.

But I do not see a reason to move 'matching' which was next to
'nothing' here.

It's probably a personal bias, but I don't like matching at all :P

quoted

+* `matching` - push the refspec ":".  In other words, push all
+  branches having the same name in both ends, even if it means
+  non-fast-forward updates.  This is for those who prepare all the
+  branches into a publishable shape and then push them out with a
+  single command.  Dangerous, and inappropriate unless you are the
+  only person updating your push destination.

It was already pointed out that unnecessary negativity needs to be
fixed, but more importantly the above "Dangerous" is not even
correct.

Okay, I'll explain my bias:

I have _never_ lost data with git: the reflog is my close companion.
There is one recent instance where I *lost* data, and I was very
unhappy about it.  I work on multiple machines, and not all local
branches are always synced with the upstream branches: when I switch
to a local branch to work on the topic, I notice my prompt and get it
to '=' before starting work.  In this one instance, I was developing
@{push} and toying around with various push.default settings.  I'd
forgotten that I'd set push.default to matching and issued my usual
push to update my branch.  This resulted in a huge number of my
branches getting rewound, and I could do nothing about it!

We've mentioned that pull --rebase is _dangerous_ (with the underline
for effect) in our documentation, when it's trivial to recover from
it: git reset --hard @{1}.  Yet, when I mention matching being
dangerous, I'm accused of being unnecessarily negative :\

In my opinion, `matching` is a bad and unpredictable setting, and
deserves last place on the list.  `nothing` deserves first place,
because it is the most predictable and least surprising.  Do not
mistake my stance for "matching is not useful"; ofcourse it is useful,
and I sometimes use it myself using ":" explicitly.

quoted

-+
-The `simple`, `current` and `upstream` modes are for those who want to
-push out a single branch after finishing work, even when the other
-branches are not yet ready to be pushed out. If you are working with
-other people to push into the same shared repository, you would want
-to use one of these.

And I do not see a reason to drop this part, especially the first
sentence.

I thought we should make it clear in the `matching` section; after
all, it's just `matching` versus everything else.