Re: [PATCH 7/7] push: document --lockref

Johannes Sixt [off-list ref] writes:

All you have been saying is that you find your

   git push --lockref there topic

is more useful than my

   git push --lockref there +topic

You are trading crystal clear semantics to save users ONE character to
type. IMO, it's a bad deal.

Think how you would explain the option in a tutorial for those who
use the push.default=simple semantics.

"""
	You usually do

		$ git pull [--rebase]

	to integrate with the shared branch and push it back with

		$ git push

	Sometimes the project wants to rewind the tip of such a
	shared branch (perhaps a bad commit included inappropriate
	material that should not be in the history).  You cordinate
	the decision to do such a rewinding with others in the
	project, you "git rebase [-i]" to prepare a replacement
	history, and then try to push tthe result out.  However

		$ git push

	will fail, because this does not fast-forward.  But you and
	your colleagues agreed that the project wants this new
	history!

        With older Git, the only way to make this push go through
        was to "--force" it.  That will risk losing work of other
        people who were not aware of the collective decision to
        rewind this shared branch [discussion of lockref safety
        comes here].  Instead you can use

		$ git push --lockref

"""

How does the last line look with your "--lockref does not override
must-fast-forward" proposal?

"""

	If your current branch is configured to push to update the
	branch 'frotz' of the remote 'origin' (replace these two
	appropriately for your situation), you would say:

		$ git push --lockref origin +HEAD:frotz

"""

How is that crystal clear?  You are just making things more complex
and harder to learn (I was tempted to add "for no good reason" here,
but I'd assume that probably you haven't explained your reasons well
enough to be heard).

The crystal clear semantics would be:

 - to override no-ff safety, use +refspec;

 - to override "mismatch" safety, do not use --lockref/use --no-lockref;

 - do not use --force unless you know the consequences.

Alternatively, this is also crystal clear

 - to use the full safety, do not use anything funky

 - to push a history that does not fast-forward safely, use
   --lockref

 - do not use --force unless you know the consequences.

and that is what the patch does.

I actually think that by implying allow-no-ff in --lockref, you are
hurting users who have configured a push refspec without a + prefix:
They suddenly do not get the push denied when it is not a fast-forward
anymore.

Of course, that is why you should not use --lockref when you do not
have to.  It is a tool to loosen "must fast-forward" in a more
controlled way than the traditional "--force".

 For example, when you have

    [remote "ko"]
        push = master
        push = +pu

and you accidentally rewound master before the point that is already
published, then

   git push --lockref ko

will happily push the rewound master.

Yes, and I am not (and I do expect nobody is) stupid to use --lockref
in such a situation where there is no need to do so.

Am 14.07.2013 21:17, schrieb Junio C Hamano:

Johannes Sixt [off-list ref] writes:

quoted

I actually think that by implying allow-no-ff in --lockref, you are
hurting users who have configured a push refspec without a + prefix:
They suddenly do not get the push denied when it is not a fast-forward
anymore.

Of course, that is why you should not use --lockref when you do not
have to.  It is a tool to loosen "must fast-forward" in a more
controlled way than the traditional "--force".

Sorry, IMO, this goes into a totally wrong direction, in particular, I
think that this is going to close to door to make --lockref the default
some day in a way that helps everyone.

I think I have not understood your motivations for this feature, and I
am not able spend more mindwidth on arguing back and forth to make it
more usable (again: IMO).

So, I bow out, and I appologize to have wasted so much of your time.

-- Hannes

Johannes Sixt wrote:

Sorry, IMO, this goes into a totally wrong direction, in particular, I
think that this is going to close to door to make --lockref the default
some day in a way that helps everyone.

Would a '*' that acts like --lockref on a per ref basis address your
concerns?

I realize that that design would hurt a project of making '+' use
lockref automatically some day.  I think that's ok, and that '+'
meaning "push whatever I have, regardless of what's on the other end,
and I mean it" would be better semantics in the long term (which
doesn't match the current behavior either :/).

Jonathan

Jonathan Nieder wrote:

Johannes Sixt wrote:

quoted

Sorry, IMO, this goes into a totally wrong direction, in particular, I
think that this is going to close to door to make --lockref the default
some day in a way that helps everyone.

Would a '*' that acts like --lockref on a per ref basis address your
concerns?

(Aside: '*' is not a great character for that.  * is already taken in
refspec syntax.  There's no clash but the two uses would be confusing.

	*:
	*:*

Some other single-character prefix could work, such as '.' or '~'.)

Am 14.07.2013 22:34, schrieb Jonathan Nieder:

Johannes Sixt wrote:

quoted

Sorry, IMO, this goes into a totally wrong direction, in particular, I
think that this is going to close to door to make --lockref the default
some day in a way that helps everyone.

Would a '*' that acts like --lockref on a per ref basis address your
concerns?

No, because I think that new syntax is not necessary.

But admittedly, I haven't spent any time to think about push.default
modes other than 'matching'. In particular, I wonder how Junio's last
example with push.default=simple can work today:

   $ git pull --rebase  # not a merge
   $ git push

because it is not a fast-forward. I am assuming that a +refspec must be
in the game somehow. Why would we then need that --lockref implies
allow-no-ff when we already have +refspec that already means allow-no-ff?

But as I said, I'm not familiar with push.default other than matching
and my assumption may be wrong.

-- Hannes

Johannes Sixt wrote:

Am 14.07.2013 22:34, schrieb Jonathan Nieder:

quoted

Would a '*' that acts like --lockref on a per ref basis address your
concerns?

No, because I think that new syntax is not necessary.

But admittedly, I haven't spent any time to think about push.default
modes other than 'matching'. In particular, I wonder how Junio's last
example with push.default=simple can work today:

   $ git pull --rebase  # not a merge
   $ git push

because it is not a fast-forward.

Right, let's examine this example more closely.

If I run:

	(1) git pull --rebase
	(2) git push

then normally that push will be a fast-forward.  My changes are
on top of the new upstream changes, just as though I used format-patch
and send-email to submit the changes to a maintainer who would then
apply them.

However, someone else might have pushed to the same branch between
step (1) and (2), causing the fast-forward-only push to fail.

Usually that means other person made a valuable change and I can
simply repeat steps (1), and (2) and they will succeed.

But maybe that intervening push was a mistake.  To distinguish that
possibility I might do something like

	(3) git fetch origin
	(4) gitk @{u}@{1}..@{u}; # Is the change good?

	(5a) git pull --rebase; git push; # Yes, put my change on top of it
	(5b) git push --force; # No, my change is better!

So far so good.  But what if yet another change is made upstream
between step (3) and (5)?

If following approach (5a), that's fine.  We notice the new
intervening change and react accordingly, again.  There is a
possibility of starvation, but no other harm done.

In case (5b), it may be a serious problem.  I don't know about the
intervening change until I read the "git push" output, and in the
usual case I just won't notice.  The new lockref UI is meant to
address this problem.  So in the new world order, in case (5b) it
sounds like I should have instead used

	(5b') git push --allow-non-ff

Suppose I am writing a script that is meant to set the remote
repository to a known state.  Other contributors are only using
fast-forward updates so once my change goes in they will act
appropriately.  I just need to get my ref update in, without being
blocked by other ref updates.

Then I will use

	(5c) git push --force

which means not to use this new lockref trick that looks at my
remote-tracking branch and instead to just force the ref update.  This
would for example be the right semantics when pushing to a mirror from
a relay that also fetches from a canonical repository.  It avoids
needing to fetch from the target repo before every push.

Of course if ref updates are highly contended, even the current "git
push --force" will sometimes fail, since it internally *does* use a
compare-and-swap against the result of an ls-remote.  That's a (minor)
bug, imho.  Fixing it will require tweaking the protocol to make the
compare-and-swap optional.

Johannes Sixt [off-list ref] writes:

Am 14.07.2013 21:17, schrieb Junio C Hamano:

quoted

Johannes Sixt [off-list ref] writes:

quoted

I actually think that by implying allow-no-ff in --lockref, you are
hurting users who have configured a push refspec without a + prefix:
They suddenly do not get the push denied when it is not a fast-forward
anymore.

Of course, that is why you should not use --lockref when you do not
have to.  It is a tool to loosen "must fast-forward" in a more
controlled way than the traditional "--force".

Sorry, IMO, this goes into a totally wrong direction, in particular, I
think that this is going to close to door to make --lockref the default
some day in a way that helps everyone.

I would presume that you would force that "reverse tracking"
short-hand as the expected value, as "default" will not have other
sources of information.

I think the use of "reverse tracking" is way overrated.  It is
probably the only default value that we could use, if the user is
too lazy not to specify it, but I do not think it is particularly a
sensible or safe default.

The following does not discuss "should --lockref automatically
disable the 'must fast-forward' check?".  The problem highlighted is
the same, regardless of the answer to that question.

After rebasing beyond what is already published, you try the
"lockref" push, e.g. (we assume you work on master and push back to
update master at your origin):

	$ git fetch
        $ git rebase -i @{u}~4 ;# rebase beyond what is there
        $ git push ;# of course this will not fast-forward
        $ git push --lockref
	... or with your "must-fast-forward is independent"
	$ git push --lockref origin +master
        ... or also with your "--lockref is default"
	$ git push origin +master

If somebody else pushed while you are working on the rebase, the
last step (one of the above push) will fail due to stale
expectation.  What now?

The user would want to keep the updated tip, so the first thing that
happens will always be

	$ git fetch
	$ git log ..@{u} ;# what will we be losing?

The right thing to do at this point is to rebase your 'master' again
on top of @{u}

	$ git rebase -i @{u}

before attempting to push back again.  If you do that, then you can
do another "lockref" push.

But the thing is, a novice who does not know what he is doing will
likely to do this:

        $ git push --lockref
	... or with your "must-fast-forward is independent"
	$ git push --lockref origin +master
        ... or also with your "--lockref is default"
	$ git push origin +master

	... rejected due to stale expectation
        $ git fetch

You just have updated the lockref base, so if you did, without doing
anything else, 

	$ git push origin +master

then you will lose the updated contents.

The conclusion?  It does not make sense to make "lockref" the
default.

The --lockref mechanism is necessary _only_ when you want to break
the usual "must fast-forward" safety, and the user needs to be made
very aware of what he is doing.  Making it default and making it
appear easy to invoke with a single "+", is totally going in a wrong
direction.  Besides, by making it the default and turning "+" into
"only defeat 'must fast-forward", you will break existing setting of
people who have "remote.*.push = +ref" configured, without having a
remote-tracking for that ref.

So it will not happen; "lockref" will not be on by default, even if
it is made independent of "must fast-forward".

Am 15.07.2013 05:50, schrieb Junio C Hamano:

        ... or also with your "--lockref is default"
	$ git push origin +master

	... rejected due to stale expectation
        $ git fetch

You just have updated the lockref base, so if you did, without doing
anything else, 

	$ git push origin +master

then you will lose the updated contents.

The conclusion?  It does not make sense to make "lockref" the
default.

Point taken.

So it will not happen; "lockref" will not be on by default, even if
it is made independent of "must fast-forward".

OK.

-- Hannes

Am 14.07.2013 22:59, schrieb Johannes Sixt:

... I wonder how Junio's last
example with push.default=simple can work today:

   $ git pull --rebase  # not a merge
   $ git push

because it is not a fast-forward.

*blush* I was mostly asleep and and totally off the rails when I wrote
this nonsense.

-- Hannes