[RFD] Making "git push [--force/--delete]" safer?

[RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Consider these two scenarios.

1. If you are collaborating with others and you have arranged with
   the participants to rewind a shared branch, you would do
   something like this:

        $ git fetch origin branch
        ... fetch everything so that we won't lose anything ...
        $ git checkout -b rebase-work FETCH_HEAD
        $ git rebase -i
        $ git push origin +HEAD:branch

   The last step has to be "--force", as you are deliberately
   pushing a history that does not fast-forward.

2. If you know a branch you pushed over there has now been fully
   merged to the "trunk" and want to remove it, you would do this:

        $ git fetch origin branch:tmp-branch trunk:tmp-trunk
        ... double check to make sure branch is fully merged ...
        $ git merge-base --is-ancestor tmp-branch tmp-trunk; echo $?
        0
        ... good, branch is part of trunk ...
        $ git push origin --delete branch

   The last step would delete the branch, but you made sure it
   has been merged to the trunk, not to lose anybody's work.

But in either of these cases, if something happens at 'origin' to
the branch you are forcing or deleting since you fetched to inspect
it, you may end up losing other people's work.

 - In the first scenario, somebody who is unaware of the decision to
   rewind and rebuild the branch may attempt to push to the branch
   between the time you fetched to rebase it and the time you pushed
   to replace it with the result of the rebasing.

 - In the second scenario, somebody may have pushed a new change to
   the branch since you fetched to inspect.

We can make these pushes safer by optionally allowing the user to
tell "git push" this:

        I am forcing/deleting, based on the assumption that the
        value of 'branch' is still at this object.  If that
        assumption no longer holds, i.e. if something happened to
        the branch since I started preparing for this push, please
        do not proceed and fail this push.

With such a mechanism, the first example would say "'branch' must be
at $(git rev-parse --verify FETCH_HEAD)", and the second example
would say "'branch' must be at $(git rev-parse --verify tmp-branch)".

The network protocol of "git push" conveys enough information to
make this possible.  An early part of the exchange goes like this:

        receiver -> sender
                list of <current object name, refname>
        sender -> receiver
                list of <current object name, new object name, refname>
        sender -> receiver
                packfile payload
                ...

When the "git push" at the last step of the above two examples
contact the other end, we would immediately know what the current
value of 'branch' is.  We can locally fail the command if the value
is different from what we expect.

Now at the syntax level, I see three possibilities to let the user
express this new constraints:

  (1) Extend "refspec" syntax to "src:dst:expect", e.g.

      $ git push there HEAD:branch:deadbabecafe

      would say "update 'branch' with the object at my HEAD, only if
      the current value of 'branch' is deadbabecafe".

      An reservation I have against this syntax is that it does not
      mesh well with the "update the upstream of the currrent
      branch" and other modes, and instead you have to always spell
      three components out.  But perhaps requiring the precondition
      is rare enough that it may be acceptable.

  (2) Add --compare-and-swap=dst:expect parameters, e.g.

      $ git push --cas=master:deadbabecafe --cas=next:cafebabe ":"

      This removes the "reservation" I expressed against (1) above
      (i.e. we are doing a "matching" push in this example, but we
      will fail if 'master' and 'next' are not pointing at the
      expected objects).

  (3) Add a mechanism to call a custom validation script after "git
      push" reads the list of <current object name, refname> tuples,
      but before responding with the proposed update.  The script
      would be fed a list of <current object name, new object
      name, refname> tuples (i.e. what the sender _would_ tell the
      receiving end if there weren't this mechanism), and can tell
      "git push" to fail with its exit status.

      This would be the most flexible in that the validation does
      not have to be limited to "the ref must be still pointing at
      the object we expect" (aka compare-and-swap); the script could
      implement other semantics (e.g. "the ref must be pointing at
      the object or its ancestor").

      But it may be cumbersome to use and the added flexibility may
      not be worth it.

      - The way to specify the validation script could be an
        in-repository hook but then there will need a way to pass
        additional per-invocation parameters (in the earlier sample
        scenarios, values of FETCH_HEAD and tmp-branch).

      - Or it could be a "--validate-script=check.sh" option, and it
        is up to the caller how to tailor that check.sh script
        customized for this particular invocation (i.e. embedding
        the values of FETCH_HEAD and tmp-branch in the script in the
        earlier sample scenarios).

I am inclined to say, if we were to do this, we should do (2) among
the above three.

But of course, others may have better ideas ;-).

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johan Herland]

On Tue, Jul 2, 2013 at 10:57 PM, Junio C Hamano <gitster@pobox.com> wrote:

[...]

>   (2) Add --compare-and-swap=dst:expect parameters, e.g.
>
>       $ git push --cas=master:deadbabecafe --cas=next:cafebabe ":"
>
>       This removes the "reservation" I expressed against (1) above
>       (i.e. we are doing a "matching" push in this example, but we
>       will fail if 'master' and 'next' are not pointing at the
>       expected objects).

I still think this is too long/verbose for the average user to
remember, and type out. Also, I don't like the name, as it is too
'technical', and describes the nature of the implementation (i.e. the
"how") rather than the purpose of using it (i.e. the "why" or "what").

>   (3) Add a mechanism to call a custom validation script after "git
>       push" reads the list of <current object name, refname> tuples,
>       but before responding with the proposed update.  The script
>       would be fed a list of <current object name, new object
>       name, refname> tuples (i.e. what the sender _would_ tell the
>       receiving end if there weren't this mechanism), and can tell
>       "git push" to fail with its exit status.
>
>       This would be the most flexible in that the validation does
>       not have to be limited to "the ref must be still pointing at
>       the object we expect" (aka compare-and-swap); the script could
>       implement other semantics (e.g. "the ref must be pointing at
>       the object or its ancestor").

With this, I guess --dry-run could be reformulated as a trivial
validation script that always returns a non-zero exit code (although
it should still cause 'push' to return zero).

[...]

> I am inclined to say, if we were to do this, we should do (2) among
> the above three.
>
> But of course, others may have better ideas ;-).

I assume that in most cases the expected value of the remote ref would
equal the current value of the corresponding remote-tracking ref in
the user's repo, so why not use that as the default expected value?
E.g.:

  $ git config push.default simple
  $ git checkout -b foo -t origin/foo
  # prepare non-ff update
  $ git push --force-if-expected
  # the above validates foo @ origin != origin/foo before pushing

And if the users expects a different value, (s)he can pass that to the
same option:

  $ git push --force-if-expected=refs/original/foo my_remote HEAD:foo
  # the above fails if foo @ origin != refs/original/foo

The option name probably needs a little work, but as long as it
properly communicates the user's _intent_ I'm fine with whatever we
call it.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johan Herland]

On Wed, Jul 3, 2013 at 12:55 AM, Johan Herland <johan@herland.net> wrote:
> I assume that in most cases the expected value of the remote ref would
> equal the current value of the corresponding remote-tracking ref in
> the user's repo, so why not use that as the default expected value?
> E.g.:
>
>   $ git config push.default simple
>   $ git checkout -b foo -t origin/foo
>   # prepare non-ff update
>   $ git push --force-if-expected
>   # the above validates foo @ origin != origin/foo before pushing

Oops, typo: s/!=/==/

>
> And if the users expects a different value, (s)he can pass that to the
> same option:
>
>   $ git push --force-if-expected=refs/original/foo my_remote HEAD:foo
>   # the above fails if foo @ origin != refs/original/foo
>
> The option name probably needs a little work, but as long as it
> properly communicates the user's _intent_ I'm fine with whatever we
> call it.

Overnight, it occured to me that --force-if-expected could be
simplified by leveraging the existing --force option; for the above
two examples, respectively:

  $ git push --force --expect
  # validate foo @ origin == @{upstream} before pushing

and

  $ git push --force --expect=refs/original/foo my_remote HEAD:foo
  # validate foo @ my_remote == refs/original/foo before pushing

In other words, the --expect option becomes a modifier on the --force
behaviour: If --expect is given, and the remote ref is not as
expected, then the push will still fail, even when --force is given.
Furthermore, this could be fleshed out by allowing the user to
configure push.expect = True, in which case --expect will be assumed
whenever --force is used, and the user can override with --no-expect.

If push.expect == True (or if --expect is given on command-line
without a parameter), we default to using @{upstream} as the expected
value, and we complain to the user if the current branch has no
upstream. This way, you can still enable push.expect even when you do
not configure @{upstream}, but it compels you to always supply
--expect=$something (or --no-expect) when you use --force.


...Johan

PS: I'm still unsure about the option naming. Maybe --validate would
be better than --expect, but I feel it should convey more strongly
that we're doing _pre_-validation, as opposed to (post-)validating the
_result_ of the push, whatever that would look like.

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Overnight, it occured to me that --force-if-expected could be
> simplified by leveraging the existing --force option; for the above
> two examples, respectively:
>
>   $ git push --force --expect
>   # validate foo @ origin == @{upstream} before pushing
>
> and
>
>   $ git push --force --expect=refs/original/foo my_remote HEAD:foo
>   # validate foo @ my_remote == refs/original/foo before pushing

First, on the name.

I do not think either "--validate" or "--expect" is particularly a
good one.  The former lets this feature squat on a good name that
covers a much broader spectrum, forbidding people from adding other
kinds of validation later.  "--expect" is slightly less bad in that
sense; saying "we expect this" does imply "otherwise it is an
unexpected situation and we would fail", but the name still does not
feel ideal.

What is the essense of compare-and-swap?  Perhaps we can find a good
word by thinking that question through.  

To me, it is a way to implement a "lock" on the remote ref without
actually taking a lock (which would leave us open for a stale lock),
and this "lock"-ness is what we want in order to guarantee safety.

So we could perhaps call it "--lockref"?

I'll leave the name open but tentatively use this name in the
following, primarily to see how well it sits on the command line
examples.

Then on the semantics/substance.

I had quite a similar thought as you had while reading your initial
response.  In the most generic form, we would want to be able to
pass necessary information fully via the option, i.e.

	--lockref=theirRefName:expectedValue

but when the option is spelled without details, we could fill in the
default values by making a reasonable guess of what the user could
have meant.  If we only have --lockref without refname nor value,
then we will enable the safety for _all_ refs that we are going to
update during this push.  If we have --lockref=theirRefName without
the expected value for that ref, we will enable the safety only for
the ref (you can give more than one --lockref=theirRefName), and
guess what value we should expect.  If we have a fully specified
option, we do not have to guess the value.

And for the expected value, when we have a tracking branch for the
branch at the remote we are trying to update, its value is a very
good guess of what the user meant.

Note, however, that this is very different from @{upstream}.

You could be pushing a branch "frotz", that is configured to
integrate with "master" taken from "origin", but

 (1) to a branch different from "master" of "origin", e.g.

	$ git push --lockref origin frotz:nitfol
	$ git push --lockref origin :nitfol	;# deleting

 (2) even to a branch of a remote that is different from "origin",
     e.g.

	$ git push --lockref xyzzy frotz:nitfol
	$ git push --lockref xyzzy :nitfol	;# deleting

Even in these case, if you have a remote tracking branch for the
destination (i.e. you have refs/remotes/origin/nitfol in case (1) or
refs/remotes/xyzzy/nitfol in case (2) to be updated by fetching from
origin or xyzzy), we can and should use that value as the default.

There is no room for frotz@{upstream} (or @{upstream} of the current
branch) to get in the picture.

Except when you happen to be pushing with "push.default = upstream",
that is.  But that is a natural consequence of the more generic
check with "our remote tracking branch of the branch we are updating
at the remote" rule.

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johan Herland]

On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> Overnight, it occured to me that --force-if-expected could be
>> simplified by leveraging the existing --force option; for the above
>> two examples, respectively:
>>
>>   $ git push --force --expect
>>   # validate foo @ origin == @{upstream} before pushing
>>
>> and
>>
>>   $ git push --force --expect=refs/original/foo my_remote HEAD:foo
>>   # validate foo @ my_remote == refs/original/foo before pushing
>
> First, on the name.
>
> I do not think either "--validate" or "--expect" is particularly a
> good one.  The former lets this feature squat on a good name that
> covers a much broader spectrum, forbidding people from adding other
> kinds of validation later.  "--expect" is slightly less bad in that
> sense; saying "we expect this" does imply "otherwise it is an
> unexpected situation and we would fail", but the name still does not
> feel ideal.
>
> What is the essense of compare-and-swap?  Perhaps we can find a good
> word by thinking that question through.
>
> To me, it is a way to implement a "lock" on the remote ref without
> actually taking a lock (which would leave us open for a stale lock),
> and this "lock"-ness is what we want in order to guarantee safety.
>
> So we could perhaps call it "--lockref"?
>
> I'll leave the name open but tentatively use this name in the
> following, primarily to see how well it sits on the command line
> examples.

I agree that neither --expect nor --validate are very good. I also
don't like --lockref, mostly because there is no locking involved, and
I think most users will jump to an incorrect conclusion about what
this option does, unless they read the documentation.

Some other suggestions:

a) --update-if. I think this reads quite nicely in the fully specified
variant: --update-if=theirRefName:expectedValue, but it becomes more
cryptic when defaults are assumed (i.e. --update-if without any
arguments).

b) --precond. This makes it clear that we're specifying a precondition
on the push. Again, I think the fully specified version reads nicely,
but it might seem a little cryptic when no arguments are given.

c) --pre-verify, --pre-check are merely variations on (b), other
variations include --pre-verify-ref or --pre-check-ref, making things
more explicit at the cost of option name length.

> Then on the semantics/substance.
>
> I had quite a similar thought as you had while reading your initial
> response.  In the most generic form, we would want to be able to
> pass necessary information fully via the option, i.e.
>
>         --lockref=theirRefName:expectedValue
>
> but when the option is spelled without details, we could fill in the
> default values by making a reasonable guess of what the user could
> have meant.  If we only have --lockref without refname nor value,
> then we will enable the safety for _all_ refs that we are going to
> update during this push.  If we have --lockref=theirRefName without
> the expected value for that ref, we will enable the safety only for
> the ref (you can give more than one --lockref=theirRefName), and
> guess what value we should expect.  If we have a fully specified
> option, we do not have to guess the value.
>
> And for the expected value, when we have a tracking branch for the
> branch at the remote we are trying to update, its value is a very
> good guess of what the user meant.
>
> Note, however, that this is very different from @{upstream}.
>
> You could be pushing a branch "frotz", that is configured to
> integrate with "master" taken from "origin", but
>
>  (1) to a branch different from "master" of "origin", e.g.
>
>         $ git push --lockref origin frotz:nitfol
>         $ git push --lockref origin :nitfol     ;# deleting
>
>  (2) even to a branch of a remote that is different from "origin",
>      e.g.
>
>         $ git push --lockref xyzzy frotz:nitfol
>         $ git push --lockref xyzzy :nitfol      ;# deleting
>
> Even in these case, if you have a remote tracking branch for the
> destination (i.e. you have refs/remotes/origin/nitfol in case (1) or
> refs/remotes/xyzzy/nitfol in case (2) to be updated by fetching from
> origin or xyzzy), we can and should use that value as the default.
>
> There is no room for frotz@{upstream} (or @{upstream} of the current
> branch) to get in the picture.
>
> Except when you happen to be pushing with "push.default = upstream",
> that is.  But that is a natural consequence of the more generic
> check with "our remote tracking branch of the branch we are updating
> at the remote" rule.

Fully agree with all of the above. I've been living under the
"push.default = upstream" rock for too long... ;)

So, how do we deal with the various corner cases?

If we don't have a tracking branch for the branch at the remote we are
trying to update (and an expected value is not specified on the
command line) we should obviously fail the push as we were asked to
verify, but don't know what to verify against. AFAICS, there is no
alternative fallbacks for the expected value of the remote ref, and
even if there were, I'm wary of adding too many fallbacks as it makes
the behaviour less transparent.

Similarly, if we're expecting a certain value for a ref, and that ref
does not (yet) exist at the remote, we should also fail (even if the
same push without --force (and --lockref) would succeed), as we
clearly expected the ref to already exist, and its non-existence might
point to a typo somewhere in our command.

For the case where we're pushing multiple refs, and have expected
values for more than one of them, we need to determine if a failing
expectation should cause the entire push to fail, or merely stop that
one ref from being pushed (letting the others go through). I'd feel
safer if the _whole_ push failed, but I'm not a "push.default =
matching" user, so my opinion is of limited value. AFAICS, the current
behaviour of "matching" is that one failing (e.g. non-ff) ref does not
abort the entire push, which I find somewhat unsafe...

...Johan

--
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFD] Making "git push [--force/--delete]" safer?

[Jonathan del Strother]

On 3 July 2013 11:00, Johan Herland <johan@herland.net> wrote:
> On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johan Herland <johan@herland.net> writes:
>>> Overnight, it occured to me that --force-if-expected could be
>>> simplified by leveraging the existing --force option; for the above
>>> two examples, respectively:
>>>
>>>   $ git push --force --expect
>>>   # validate foo @ origin == @{upstream} before pushing
>>>
>>> and
>>>
>>>   $ git push --force --expect=refs/original/foo my_remote HEAD:foo
>>>   # validate foo @ my_remote == refs/original/foo before pushing
>>
>> First, on the name.
>>
>> I do not think either "--validate" or "--expect" is particularly a
>> good one.  The former lets this feature squat on a good name that
>> covers a much broader spectrum, forbidding people from adding other
>> kinds of validation later.  "--expect" is slightly less bad in that
>> sense; saying "we expect this" does imply "otherwise it is an
>> unexpected situation and we would fail", but the name still does not
>> feel ideal.
>>
>> What is the essense of compare-and-swap?  Perhaps we can find a good
>> word by thinking that question through.
>>
>> To me, it is a way to implement a "lock" on the remote ref without
>> actually taking a lock (which would leave us open for a stale lock),
>> and this "lock"-ness is what we want in order to guarantee safety.
>>
>> So we could perhaps call it "--lockref"?
>>
>> I'll leave the name open but tentatively use this name in the
>> following, primarily to see how well it sits on the command line
>> examples.
>
> I agree that neither --expect nor --validate are very good. I also
> don't like --lockref, mostly because there is no locking involved, and
> I think most users will jump to an incorrect conclusion about what
> this option does, unless they read the documentation.
>
> Some other suggestions:
>
> a) --update-if. I think this reads quite nicely in the fully specified
> variant: --update-if=theirRefName:expectedValue, but it becomes more
> cryptic when defaults are assumed (i.e. --update-if without any
> arguments).
>
> b) --precond. This makes it clear that we're specifying a precondition
> on the push. Again, I think the fully specified version reads nicely,
> but it might seem a little cryptic when no arguments are given.
>
> c) --pre-verify, --pre-check are merely variations on (b), other
> variations include --pre-verify-ref or --pre-check-ref, making things
> more explicit at the cost of option name length.

I'm struggling to think of instances where I wouldn't want this
CAS-like behaviour.  Wouldn't it be better to make it the default when
pushing, and allowing the current behaviour with "git push
--blind-force" or something?

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johan Herland]

On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother
<maillist@steelskies.com> wrote:
> I'm struggling to think of instances where I wouldn't want this
> CAS-like behaviour.  Wouldn't it be better to make it the default when
> pushing, and allowing the current behaviour with "git push
> --blind-force" or something?

I believe I agree with you. I guess the reason this hasn't come up
before is that by far most of the pushes we do are either
fast-forwarding, or pushing into a non-shared repo (e.g. my own public
repo),  and this safety only really applies when we're forcing a
non-fast-forward push into a shared repo...

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFD] Making "git push [--force/--delete]" safer?

[Michael Haggerty]

On 07/03/2013 12:11 PM, Johan Herland wrote:
> On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother
> <maillist@steelskies.com> wrote:
>> I'm struggling to think of instances where I wouldn't want this
>> CAS-like behaviour.  Wouldn't it be better to make it the default when
>> pushing, and allowing the current behaviour with "git push
>> --blind-force" or something?
> 
> I believe I agree with you. I guess the reason this hasn't come up
> before is that by far most of the pushes we do are either
> fast-forwarding, or pushing into a non-shared repo (e.g. my own public
> repo),  and this safety only really applies when we're forcing a
> non-fast-forward push into a shared repo...

I didn't see Jonathan's original email but I was having exactly the same
though as him (and was even going to propose the same option name).

Non-ff pushing without knowing what you are going to overwrite is
irresponsible in most scenarios, and (if backwards-compatibility
concerns can be overcome) I think it would be quite prudent to forbid a
non-ff push if there is no local remote-tracking branch that is
up-to-date at the time of the push.  Circumventing that check should
require some extra-super-force option.

So yes, I very much like the general idea of the RFD and personally
would lean towards making it stronger and default, at the 2.0 transition
if necessary.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johannes Sixt]

Am 7/3/2013 12:50, schrieb Michael Haggerty:
> On 07/03/2013 12:11 PM, Johan Herland wrote:
>> On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother
>> <maillist@steelskies.com> wrote:
>>> I'm struggling to think of instances where I wouldn't want this
>>> CAS-like behaviour.  Wouldn't it be better to make it the default when
>>> pushing, and allowing the current behaviour with "git push
>>> --blind-force" or something?
>>
>> I believe I agree with you. I guess the reason this hasn't come up
>> before is that by far most of the pushes we do are either
>> fast-forwarding, or pushing into a non-shared repo (e.g. my own public
>> repo),  and this safety only really applies when we're forcing a
>> non-fast-forward push into a shared repo...
> 
> I didn't see Jonathan's original email but I was having exactly the same
> though as him (and was even going to propose the same option name).
> 
> Non-ff pushing without knowing what you are going to overwrite is
> irresponsible in most scenarios, and (if backwards-compatibility
> concerns can be overcome) I think it would be quite prudent to forbid a
> non-ff push if there is no local remote-tracking branch that is
> up-to-date at the time of the push.  Circumventing that check should
> require some extra-super-force option.

I don't think that is necessary. We already have *two* options to
force-push a ref: the + in front of refspec, and --force.

IMO, the meaning of + should be changed to "force-push with safety", and
--force can then be used to override if the safety triggers (i.e., --force
is your extra-super-force option).

-- Hannes

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> I don't think that is necessary. We already have *two* options to
> force-push a ref: the + in front of refspec, and --force.

They mean exactly the same thing; the only difference being that "+"
prefix is per target ref, while "--force" covers everything, acting
as a mere short-hand to add "+" to everything you push.

If the "--lockref/--update-only-if-ref-is-still-there" option
defeats "--force", it should defeat "+src:dst" exactly the same way.

Re: [RFD] Making "git push [--force/--delete]" safer?

[Johannes Sixt]

Am 7/3/2013 21:53, schrieb Junio C Hamano:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> I don't think that is necessary. We already have *two* options to
>> force-push a ref: the + in front of refspec, and --force.
> 
> They mean exactly the same thing; the only difference being that "+"
> prefix is per target ref, while "--force" covers everything, acting
> as a mere short-hand to add "+" to everything you push.

I know, and I'm saying that we do not have to keep this duplicity.

> If the "--lockref/--update-only-if-ref-is-still-there" option
> defeats "--force", it should defeat "+src:dst" exactly the same way.

This logic is backwards. If anything, then "--force" must defeat the
safety that "--lockref" gives.

-- Hannes

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 7/3/2013 21:53, schrieb Junio C Hamano:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>>> I don't think that is necessary. We already have *two* options to
>>> force-push a ref: the + in front of refspec, and --force.
>> 
>> They mean exactly the same thing; the only difference being that "+"
>> prefix is per target ref, while "--force" covers everything, acting
>> as a mere short-hand to add "+" to everything you push.
>
> I know, and I'm saying that we do not have to keep this duplicity.

Of course we do.  If you change + prefix to "push but always require
tracking ref in the opposite direction", you will break existing
setup by (1) making it impossible to loosen the restriction per ref,
and (2) forcing people to have reverse tracking ref.

It is OK to introduce another prefix that mean a new thing.  It is
absolutely not OK to change the semantics of only one and not the
other.

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Jonathan del Strother <maillist@steelskies.com> writes:

> I'm struggling to think of instances where I wouldn't want this
> CAS-like behaviour.  Wouldn't it be better to make it the default when
> pushing, and allowing the current behaviour with "git push
> --blind-force" or something?

Not until we run this in the wild for a while and the mechanism
proves to be useful without being too cumbersome to some population.

Then at a major version bump, we can start talking about enabling it
by default, allowing people to selectively disable it.

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan del Strother <maillist@steelskies.com> writes:
>
>> I'm struggling to think of instances where I wouldn't want this
>> CAS-like behaviour.  Wouldn't it be better to make it the default when
>> pushing, and allowing the current behaviour with "git push
>> --blind-force" or something?
>
> Not until we run this in the wild for a while and the mechanism
> proves to be useful without being too cumbersome to some population.
>
> Then at a major version bump, we can start talking about enabling it
> by default, allowing people to selectively disable it.

If we enable this by default, we would need to be a lot more careful
designing what should happen when there is no remote-tracking branch
the corresponds to what we are updating/deleting.

The proposed behaviour so far is to fail, and that is justifiable
because "the user asked us to check, but did not say what to check
against, and we tried to check with a remote-tracking branch and
found none.  We cannot satisfy the user's request to check, hence we
fail".

Enabling the check by default will change the picture somewhat; that
justification no longer holds.

If you are pushing to more than one publishing branches of your own,
there is no reason to have remote-tracking branches for the
secondary locations, because you always push to all your publishing
repositories at the same time, and you only need to keep remote
tracking for one of them to remember what you pushed out.  Making a
push to secondaries fail in such a case is bad, and forcing the user
to disable the feature for each secondary remotes is unnice, too.

Re: [RFD] Making "git push [--force/--delete]" safer?

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

>> I'll leave the name open but tentatively use this name in the
>> following, primarily to see how well it sits on the command line
>> examples.
>
> I agree that neither --expect nor --validate are very good. I also
> don't like --lockref, mostly because there is no locking involved, and

Yes and no.

This is not compare-and-swap but is "store-conditional" step in
ll/sc.  It is letting other people's activities to break your lock
to prevent you from making an undesirable update.  So in that sense,
this mechanism is very much a lock.

> Some other suggestions:
>
> a) --update-if. I think this reads quite nicely in the fully specified
> variant: --update-if=theirRefName:expectedValue, but it becomes more
> cryptic when defaults are assumed (i.e. --update-if without any
> arguments).

This name is in line with the "store conditional" aspect of the
operation, but it, together with your --precond and --pre-verify,
share the same problem as your --validate.  This is only to check
one specific precondition "The remote ref being updated must point
at this object", but all the names you suggested are too broad.

If we were to go in the direction (3) I suggested in the original
message to let you specify an arbitrary script that reads the list
of proposed updates and decide to allow them, --update-if=script.sh
would be the ideal name for that option to specify the script to be
run, though. That mechanism is broad enough to deserve such a broad
name, if we were to go in that direction.

> b) --precond. This makes it clear that we're specifying a precondition
> on the push. Again, I think the fully specified version reads nicely,
> but it might seem a little cryptic when no arguments are given.

See above.

> c) --pre-verify, --pre-check are merely variations on (b), other
> variations include --pre-verify-ref or --pre-check-ref, making things
> more explicit at the cost of option name length.

See above.

> So, how do we deal with the various corner cases?

I thought I spelled out everything, but apparently I didn't.  Here
is what I had in mind.

 (1) A bare "--lockref" exists on the command line.  E.g.

     $ git push --lockref [remote [refspec]...] ;# nothing else about lockref

     This will apply to updates of _all_ refs to be updated (e.g.
     with "remote.origin.push = +refs/heads/pu:refs/heads/pu", the
     update of 'pu' at the origin will be rejected if 'pu' fails to
     pass the test) with this push.  We make sure

     - we have remote-tracking branch for the updated ref; if we do
       not have any, we *fail* the update.

     - the value of that remote-tracking branch is the same as what
       the remote advertises to "git push"; if they do not match, we
       *fail* the update.  This includes the case where there is no
       such ref at the remote (may have deleted while we are looking
       the other way).

 (2) Remote ref specified on one of the --lockref option(s).  E.g.

     $ git push --lockref=theirRef[:value] [remote [refspec]...]

     This will apply to updates of _only_ the refs given.  refs not
     covered by --lockref will follow the usual rule (i.e. with
     --force, anything goes, without --force, only fast-forward is
     allowed).  If ":value" is given, we will use it, otherwise we
     will try to find the remote tracking branch for the updated
     ref, just like a non-specific case as above.

     A --lockref=theirRef[:value] that specifies theirRef that is
     not being pushed will be _ignored_ and not checked, so that you
     could say

	[alias]
        	safepush = push --lockref=next
	[remote "origin"]
        	push = refs/heads/maint:refs/heads/maint
        	push = refs/heads/master:refs/heads/master
        	push = refs/heads/next:refs/heads/next
        	push = +refs/heads/pu:refs/heads/pu

     and then do

	$ git safepush origin +next

     after a major version bump to rewind 'next' but still do so
     with safety, while still allowing you to say

	$ git safepush origin maint

     to push out 'maint' without having to worry about --lockref=next
     getting in the way.

 (3) Mixing --lockref and --lockref=theirRef[:value].

     Apply (2) for refs we do have remote ref specified on
     --lockref, and apply (1) for other refs we are going to update.

In any case, this check happens after we learn the current value of
remote refs but before we propose what the updated values would be,
so we can afford to fail the entire push atomically.  We could only
fail the ones that do not pass the check and let others go, but I do
not think it is a good idea.

So in short, I think I agree with you on the semantics.

[PATCH 0/7] safer "push --force" with compare-and-swap

[Junio C Hamano]

When you have to replace an already published branch with its
rebased version, you would have to --force, but then you risk losing
work, if any, that was pushed by somebody else while you are working
on rebasing, as your earlier decision that replacing the old one
with its rebase is OK was based on the assumption that there is
nothing else going on.  Unfortunately, --force is blind, and did not
offer this "... but fail if the tip has moved from what I expect".

And here is a series to remedy it.  It lets you specify what the
expected current values of refs you are attempting to update, and if
you are lazy, the value is taken from your remote tracking branch
for the ref you are attempting to update.

I am not married to the "lockref" name, but I think the semantics
implemented here is what we discussed to do in the earlier
discussion.

cf. http://thread.gmane.org/gmane.comp.version-control.git/229430

This may still be rough at edges, but a basic testset seems to pass.

I haven't bothered to check the smart-http transport; help is
greatly appreciated.

Junio C Hamano (7):
  cache.h: move remote/connect API out of it
  builtin/push.c: use OPT_BOOL, not OPT_BOOLEAN
  push: beginning of compare-and-swap "force/delete safety"
  remote.c: add command line option parser for --lockref
  push --lockref: implement logic to populate old_sha1_expect[]
  t5533: test "push --lockref"
  push: document --lockref

 Documentation/git-push.txt |  26 +++++++
 builtin/fetch-pack.c       |   2 +
 builtin/push.c             |  18 ++++-
 builtin/receive-pack.c     |   1 +
 builtin/send-pack.c        |  26 +++++++
 cache.h                    |  62 ----------------
 connect.c                  |   1 +
 connect.h                  |  13 ++++
 fetch-pack.c               |   1 +
 fetch-pack.h               |   1 +
 refs.c                     |   8 ---
 remote.c                   | 149 +++++++++++++++++++++++++++++++++++++-
 remote.h                   |  83 +++++++++++++++++++++
 send-pack.c                |   2 +
 t/t5533-push-cas.sh        | 176 +++++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c         |   6 ++
 transport.c                |  13 ++++
 transport.h                |   5 ++
 upload-pack.c              |   1 +
 19 files changed, 519 insertions(+), 75 deletions(-)
 create mode 100644 connect.h
 create mode 100755 t/t5533-push-cas.sh

-- 
1.8.3.2-875-g76c723c

[PATCH 1/7] cache.h: move remote/connect API out of it

[Junio C Hamano]

The definition of "struct ref" in "cache.h", a header file so
central to the system, always confused me.  This structure is not
about the local ref used by sha1-name API to name local objects.

It is what refspecs are expanded into, after finding out what refs
the other side has, to define what refs are updated after object
transfer succeeds to what values.  It belongs to "remote.h" together
with "struct refspec".

While we are at it, also move the types and functions related to the
Git transport connection to a new header file connect.h

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch-pack.c   |  2 ++
 builtin/receive-pack.c |  1 +
 builtin/send-pack.c    |  1 +
 cache.h                | 62 --------------------------------------------------
 connect.c              |  1 +
 connect.h              | 13 +++++++++++
 fetch-pack.c           |  1 +
 fetch-pack.h           |  1 +
 refs.c                 |  8 -------
 remote.c               |  8 +++++++
 remote.h               | 54 +++++++++++++++++++++++++++++++++++++++++++
 send-pack.c            |  1 +
 transport.c            |  2 ++
 transport.h            |  1 +
 upload-pack.c          |  1 +
 15 files changed, 87 insertions(+), 70 deletions(-)
 create mode 100644 connect.h

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index aba4465..c6888c6 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1,6 +1,8 @@
 #include "builtin.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
+#include "remote.h"
+#include "connect.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] "
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3eb5fc..7434d9b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -8,6 +8,7 @@
 #include "commit.h"
 #include "object.h"
 #include "remote.h"
+#include "connect.h"
 #include "transport.h"
 #include "string-list.h"
 #include "sha1-array.h"
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 152c4ea..e86d3b5 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -5,6 +5,7 @@
 #include "sideband.h"
 #include "run-command.h"
 #include "remote.h"
+#include "connect.h"
 #include "send-pack.h"
 #include "quote.h"
 #include "transport.h"
diff --git a/cache.h b/cache.h
index dd0fb33..cb2891d 100644
--- a/cache.h
+++ b/cache.h
@@ -1035,68 +1035,6 @@ struct pack_entry {
 	struct packed_git *p;
 };
 
-struct ref {
-	struct ref *next;
-	unsigned char old_sha1[20];
-	unsigned char new_sha1[20];
-	char *symref;
-	unsigned int
-		force:1,
-		forced_update:1,
-		deletion:1,
-		matched:1;
-
-	/*
-	 * Order is important here, as we write to FETCH_HEAD
-	 * in numeric order. And the default NOT_FOR_MERGE
-	 * should be 0, so that xcalloc'd structures get it
-	 * by default.
-	 */
-	enum {
-		FETCH_HEAD_MERGE = -1,
-		FETCH_HEAD_NOT_FOR_MERGE = 0,
-		FETCH_HEAD_IGNORE = 1
-	} fetch_head_status;
-
-	enum {
-		REF_STATUS_NONE = 0,
-		REF_STATUS_OK,
-		REF_STATUS_REJECT_NONFASTFORWARD,
-		REF_STATUS_REJECT_ALREADY_EXISTS,
-		REF_STATUS_REJECT_NODELETE,
-		REF_STATUS_REJECT_FETCH_FIRST,
-		REF_STATUS_REJECT_NEEDS_FORCE,
-		REF_STATUS_UPTODATE,
-		REF_STATUS_REMOTE_REJECT,
-		REF_STATUS_EXPECTING_REPORT
-	} status;
-	char *remote_status;
-	struct ref *peer_ref; /* when renaming */
-	char name[FLEX_ARRAY]; /* more */
-};
-
-#define REF_NORMAL	(1u << 0)
-#define REF_HEADS	(1u << 1)
-#define REF_TAGS	(1u << 2)
-
-extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
-
-#define CONNECT_VERBOSE       (1u << 0)
-extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
-extern int finish_connect(struct child_process *conn);
-extern int git_connection_is_socket(struct child_process *conn);
-struct extra_have_objects {
-	int nr, alloc;
-	unsigned char (*array)[20];
-};
-extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
-				     struct ref **list, unsigned int flags,
-				     struct extra_have_objects *);
-extern int server_supports(const char *feature);
-extern int parse_feature_request(const char *features, const char *feature);
-extern const char *server_feature_value(const char *feature, int *len_ret);
-extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
-
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
 /* A hook for count-objects to report invalid files in pack directory */
diff --git a/connect.c b/connect.c
index a0783d4..a80ebd3 100644
--- a/connect.c
+++ b/connect.c
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "run-command.h"
 #include "remote.h"
+#include "connect.h"
 #include "url.h"
 
 static char *server_capabilities;
diff --git a/connect.h b/connect.h
new file mode 100644
index 0000000..9dff25c
--- /dev/null
+++ b/connect.h
@@ -0,0 +1,13 @@
+#ifndef CONNECT_H
+#define CONNECT_H
+
+#define CONNECT_VERBOSE       (1u << 0)
+extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
+extern int finish_connect(struct child_process *conn);
+extern int git_connection_is_socket(struct child_process *conn);
+extern int server_supports(const char *feature);
+extern int parse_feature_request(const char *features, const char *feature);
+extern const char *server_feature_value(const char *feature, int *len_ret);
+extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
+
+#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index abe5ffb..c2bab42 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -9,6 +9,7 @@
 #include "fetch-pack.h"
 #include "remote.h"
 #include "run-command.h"
+#include "connect.h"
 #include "transport.h"
 #include "version.h"
 
diff --git a/fetch-pack.h b/fetch-pack.h
index 40f08ba..461cbf3 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -2,6 +2,7 @@
 #define FETCH_PACK_H
 
 #include "string-list.h"
+#include "run-command.h"
 
 struct fetch_pack_args {
 	const char *uploadpack;
diff --git a/refs.c b/refs.c
index 4302206..330060c 100644
--- a/refs.c
+++ b/refs.c
@@ -3193,14 +3193,6 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
-struct ref *find_ref_by_name(const struct ref *list, const char *name)
-{
-	for ( ; list; list = list->next)
-		if (!strcmp(list->name, name))
-			return (struct ref *)list;
-	return NULL;
-}
-
 /*
  * generate a format suitable for scanf from a ref_rev_parse_rules
  * rule, that is replace the "%.*s" spec with a "%s" spec
diff --git a/remote.c b/remote.c
index 6f57830..b1ff7a2 100644
--- a/remote.c
+++ b/remote.c
@@ -1302,6 +1302,14 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
 	free(sent_tips.tip);
 }
 
+struct ref *find_ref_by_name(const struct ref *list, const char *name)
+{
+	for ( ; list; list = list->next)
+		if (!strcmp(list->name, name))
+			return (struct ref *)list;
+	return NULL;
+}
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine
diff --git a/remote.h b/remote.h
index cf56724..a850059 100644
--- a/remote.h
+++ b/remote.h
@@ -71,6 +71,52 @@ struct refspec {
 
 extern const struct refspec *tag_refspec;
 
+struct ref {
+	struct ref *next;
+	unsigned char old_sha1[20];
+	unsigned char new_sha1[20];
+	char *symref;
+	unsigned int
+		force:1,
+		forced_update:1,
+		deletion:1,
+		matched:1;
+
+	/*
+	 * Order is important here, as we write to FETCH_HEAD
+	 * in numeric order. And the default NOT_FOR_MERGE
+	 * should be 0, so that xcalloc'd structures get it
+	 * by default.
+	 */
+	enum {
+		FETCH_HEAD_MERGE = -1,
+		FETCH_HEAD_NOT_FOR_MERGE = 0,
+		FETCH_HEAD_IGNORE = 1
+	} fetch_head_status;
+
+	enum {
+		REF_STATUS_NONE = 0,
+		REF_STATUS_OK,
+		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_ALREADY_EXISTS,
+		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_REJECT_FETCH_FIRST,
+		REF_STATUS_REJECT_NEEDS_FORCE,
+		REF_STATUS_UPTODATE,
+		REF_STATUS_REMOTE_REJECT,
+		REF_STATUS_EXPECTING_REPORT
+	} status;
+	char *remote_status;
+	struct ref *peer_ref; /* when renaming */
+	char name[FLEX_ARRAY]; /* more */
+};
+
+#define REF_NORMAL	(1u << 0)
+#define REF_HEADS	(1u << 1)
+#define REF_TAGS	(1u << 2)
+
+extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
+
 struct ref *alloc_ref(const char *name);
 struct ref *copy_ref(const struct ref *ref);
 struct ref *copy_ref_list(const struct ref *ref);
@@ -84,6 +130,14 @@ int check_ref_type(const struct ref *ref, int flags);
  */
 void free_refs(struct ref *ref);
 
+struct extra_have_objects {
+	int nr, alloc;
+	unsigned char (*array)[20];
+};
+extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+				     struct ref **list, unsigned int flags,
+				     struct extra_have_objects *);
+
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
diff --git a/send-pack.c b/send-pack.c
index 7d172ef..9a9908c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -5,6 +5,7 @@
 #include "sideband.h"
 #include "run-command.h"
 #include "remote.h"
+#include "connect.h"
 #include "send-pack.h"
 #include "quote.h"
 #include "transport.h"
diff --git a/transport.c b/transport.c
index e15db98..b84dbf0 100644
--- a/transport.c
+++ b/transport.c
@@ -3,6 +3,8 @@
 #include "run-command.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
+#include "remote.h"
+#include "connect.h"
 #include "send-pack.h"
 #include "walker.h"
 #include "bundle.h"
diff --git a/transport.h b/transport.h
index ea70ea7..b551f99 100644
--- a/transport.h
+++ b/transport.h
@@ -2,6 +2,7 @@
 #define TRANSPORT_H
 
 #include "cache.h"
+#include "run-command.h"
 #include "remote.h"
 
 struct git_transport_options {
diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..b03492e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -10,6 +10,7 @@
 #include "revision.h"
 #include "list-objects.h"
 #include "run-command.h"
+#include "connect.h"
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
-- 
1.8.3.2-875-g76c723c

[PATCH 2/7] builtin/push.c: use OPT_BOOL, not OPT_BOOLEAN

[Junio C Hamano]

The command line parser of "git push" for "--tags", "--delete", and
"--thin" options still used outdated OPT_BOOLEAN.  Because these
options do not give escalating levels when given multiple times,
they should use OPT_BOOL.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 2d84d10..342d792 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -427,15 +427,15 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0 , "all", &flags, N_("push all refs"), TRANSPORT_PUSH_ALL),
 		OPT_BIT( 0 , "mirror", &flags, N_("mirror all refs"),
 			    (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
-		OPT_BOOLEAN( 0, "delete", &deleterefs, N_("delete refs")),
-		OPT_BOOLEAN( 0 , "tags", &tags, N_("push tags (can't be used with --all or --mirror)")),
+		OPT_BOOL( 0, "delete", &deleterefs, N_("delete refs")),
+		OPT_BOOL( 0 , "tags", &tags, N_("push tags (can't be used with --all or --mirror)")),
 		OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
 		{ OPTION_CALLBACK, 0, "recurse-submodules", &flags, N_("check"),
 			N_("control recursive pushing of submodules"),
 			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
-		OPT_BOOLEAN( 0 , "thin", &thin, N_("use thin pack")),
+		OPT_BOOL( 0 , "thin", &thin, N_("use thin pack")),
 		OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", N_("receive pack program")),
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", N_("receive pack program")),
 		OPT_BIT('u', "set-upstream", &flags, N_("set upstream for git pull/status"),
-- 
1.8.3.2-875-g76c723c

[PATCH 3/7] push: beginning of compare-and-swap "force/delete safety"

[Junio C Hamano]

This teaches the deepest part of the callchain for "git push" (and
"git send-pack") to optionally allow "the old value of the ref must
be this, otherwise fail this push" we discussed earlier.

Nobody sets the new "expect_old_sha1" and "expect_old_no_trackback"
bitfields yet, so this is still a no-op.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/send-pack.c |  5 +++++
 remote.c            | 21 +++++++++++++++++++--
 remote.h            |  4 ++++
 send-pack.c         |  1 +
 transport-helper.c  |  6 ++++++
 transport.c         |  5 +++++
 6 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e86d3b5..c86c556 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -55,6 +55,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "needs force";
 			break;
 
+		case REF_STATUS_REJECT_STALE:
+			res = "error";
+			msg = "stale info";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/remote.c b/remote.c
index b1ff7a2..81bc876 100644
--- a/remote.c
+++ b/remote.c
@@ -1416,13 +1416,30 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		}
 
 		/*
+		 * If we know what the old value of the remote ref
+		 * should be, reject any push, even forced ones,
+		 * if they do not match.
+		 *
+		 * It also is an error if the user told us to check
+		 * with the remote-tracking branch to find the value
+		 * to expect, but we did not have such a tracking
+		 * branch.
+		 */
+		if (ref->expect_old_sha1 &&
+		    (ref->expect_old_no_trackback ||
+		     hashcmp(ref->old_sha1, ref->old_sha1_expect))) {
+			ref->status = REF_STATUS_REJECT_STALE;
+			continue;
+		}
+
+		/*
 		 * Decide whether an individual refspec A:B can be
 		 * pushed.  The push will succeed if any of the
 		 * following are true:
 		 *
-		 * (1) the remote reference B does not exist
+		 * (1) the remote reference B does not exist (i.e. create)
 		 *
-		 * (2) the remote reference B is being removed (i.e.,
+		 * (2) the remote reference B is being removed (i.e. delete;
 		 *     pushing :B where no source is specified)
 		 *
 		 * (3) the destination is not under refs/tags/, and
diff --git a/remote.h b/remote.h
index a850059..7ad37e6 100644
--- a/remote.h
+++ b/remote.h
@@ -75,10 +75,13 @@ struct ref {
 	struct ref *next;
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
+	unsigned char old_sha1_expect[20]; /* used by expect-old */
 	char *symref;
 	unsigned int
 		force:1,
 		forced_update:1,
+		expect_old_sha1:1,
+		expect_old_no_trackback:1,
 		deletion:1,
 		matched:1;
 
@@ -102,6 +105,7 @@ struct ref {
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_REJECT_FETCH_FIRST,
 		REF_STATUS_REJECT_NEEDS_FORCE,
+		REF_STATUS_REJECT_STALE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
diff --git a/send-pack.c b/send-pack.c
index 9a9908c..b228d65 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -227,6 +227,7 @@ int send_pack(struct send_pack_args *args,
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_REJECT_FETCH_FIRST:
 		case REF_STATUS_REJECT_NEEDS_FORCE:
+		case REF_STATUS_REJECT_STALE:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport-helper.c b/transport-helper.c
index db9bd18..95d22f8 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -683,6 +683,11 @@ static int push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "stale info")) {
+			status = REF_STATUS_REJECT_STALE;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
@@ -756,6 +761,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_STALE:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
diff --git a/transport.c b/transport.c
index b84dbf0..98f5270 100644
--- a/transport.c
+++ b/transport.c
@@ -709,6 +709,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "needs force", porcelain);
 		break;
+	case REF_STATUS_REJECT_STALE:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "stale info", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -1078,6 +1082,7 @@ static int run_pre_push_hook(struct transport *transport,
 	for (r = remote_refs; r; r = r->next) {
 		if (!r->peer_ref) continue;
 		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
+		if (r->status == REF_STATUS_REJECT_STALE) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;
 
 		strbuf_reset(&buf);
-- 
1.8.3.2-875-g76c723c

[PATCH 4/7] remote.c: add command line option parser for --lockref

[Junio C Hamano]

Update "git push" and "git send-pack" to parse this commnd line
option.

The intended sematics is:

 * "--lockref" alone, without specifying the details, will protect
   _all_ remote refs that are going to be updated by requiring their
   current value to be the same as the remote-tracking branch we
   have for them, unless otherwise specified;

 * "--lockref=refname", without specifying the expected value, will
   protect that refname, if it is going to be updated, by requiring
   its current value to be the same as the remote-tracking branch we
   have for it;

 * "--lockref=refname:value" will protect that refname, if it is
   going to be updated, by requiring its current value to be the
   same as the specified value (which is allowed to be different
   from the remote-tracking branch we have for the refname, or we do
   not even have to have such a remote-tracking branch when this
   form is used);

 * "--lockref=refname:" (empty value) is to expect that refname does
   not exist (yet); and

 * "--no-lockref" will cancel all the previous --lockref on the
   command line.

In any of the forms, when we try to use a remote-tracking branch for
the remote ref being updated, it is an error if there is no such
remote-tracking branch on our end.

Because the command line options are parsed _before_ we know which
remote we are pushing to, there needs further processing to the
parsed data after we instantiate the transport object to:

 * expand "refname" given by the user to a full refname to be
   matched with the list of "struct ref" used in match_push_refs()
   and set_ref_status_for_push(); and

 * learning the actual local ref that is the remote-tracking branch
   for the specified remote ref.

Further, some processing need to be deferred until we find the set
of remote refs and match_push_refs() returns in order to find the
ones that need to be checked after explicit ones have been processed
for "--lockref" (no specific details).

These post-processing will be the topic of the next patch.

Oh, of course, the option name is still not cast in stone.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c      |  6 ++++++
 builtin/send-pack.c | 17 +++++++++++++++
 remote.c            | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            | 22 ++++++++++++++++++++
 4 files changed, 104 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index 342d792..31a5ba0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -21,6 +21,8 @@ static const char *receivepack;
 static int verbosity;
 static int progress = -1;
 
+static struct push_cas_option cas;
+
 static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
@@ -432,6 +434,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
+		{ OPTION_CALLBACK,
+		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
+		  N_("require old value of ref to be at this value"),
+		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
 		{ OPTION_CALLBACK, 0, "recurse-submodules", &flags, N_("check"),
 			N_("control recursive pushing of submodules"),
 			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index c86c556..061e2b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -108,6 +108,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int flags;
 	unsigned int reject_reasons;
 	int progress = -1;
+	struct push_cas_option cas = {0};
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
@@ -170,6 +171,22 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				helper_status = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--" CAS_OPT_NAME)) {
+				if (parse_push_cas_option(&cas, NULL, 0) < 0)
+					exit(1);
+				continue;
+			}
+			if (!strcmp(arg, "--no-" CAS_OPT_NAME)) {
+				if (parse_push_cas_option(&cas, NULL, 1) < 0)
+					exit(1);
+				continue;
+			}
+			if (!prefixcmp(arg, "--" CAS_OPT_NAME "=")) {
+				if (parse_push_cas_option(&cas,
+							  strchr(arg, '=') + 1, 1) < 0)
+					exit(1);
+				continue;
+			}
 			usage(send_pack_usage);
 		}
 		if (!dest) {
diff --git a/remote.c b/remote.c
index 81bc876..e9b423a 100644
--- a/remote.c
+++ b/remote.c
@@ -1938,3 +1938,62 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
 	string_list_clear(&ref_names, 0);
 	return stale_refs;
 }
+
+/*
+ * Lockref aka CAS
+ */
+void clear_cas_option(struct push_cas_option *cas)
+{
+	int i;
+
+	for (i = 0; i < cas->nr; i++)
+		free(cas->entry->refname);
+	free(cas->entry);
+	memset(cas, 0, sizeof(*cas));
+}
+
+static struct push_cas *add_cas_entry(struct push_cas_option *cas,
+				      const char *refname,
+				      size_t refnamelen)
+{
+	struct push_cas *entry;
+	ALLOC_GROW(cas->entry, cas->nr + 1, cas->alloc);
+	entry = &cas->entry[cas->nr++];
+	memset(entry, 0, sizeof(*entry));
+	entry->refname = xmemdupz(refname, refnamelen);
+	return entry;
+}
+
+int parseopt_push_cas_option(const struct option *opt, const char *arg, int unset)
+{
+	return parse_push_cas_option(opt->value, arg, unset);
+}
+
+int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unset)
+{
+	const char *colon;
+	struct push_cas *entry;
+
+	if (unset) {
+		/* "--no-lockref" */
+		clear_cas_option(cas);
+		return 0;
+	}
+
+	if (!arg) {
+		/* just "--lockref" */
+		cas->use_tracking_for_rest = 1;
+		return 0;
+	}
+
+	/* "--lockref=refname" or "--lockref=refname:value" */
+	colon = strchrnul(arg, ':');
+	entry = add_cas_entry(cas, arg, colon - arg);
+	if (!*colon)
+		entry->use_tracking = 1;
+	else if (!colon[1])
+		hashclr(entry->expect);
+	else if (get_sha1(colon + 1, entry->expect))
+		return error("cannot parse expected object name '%s'", colon + 1);
+	return 0;
+}
diff --git a/remote.h b/remote.h
index 7ad37e6..28eb6a3 100644
--- a/remote.h
+++ b/remote.h
@@ -1,6 +1,8 @@
 #ifndef REMOTE_H
 #define REMOTE_H
 
+#include "parse-options.h"
+
 enum {
 	REMOTE_CONFIG,
 	REMOTE_REMOTES,
@@ -230,4 +232,24 @@ struct ref *guess_remote_head(const struct ref *head,
 /* Return refs which no longer exist on remote */
 struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map);
 
+/*
+ * Lockref aka CAS
+ */
+#define CAS_OPT_NAME "lockref"
+
+struct push_cas_option {
+	unsigned use_tracking_for_rest:1;
+	struct push_cas {
+		unsigned char expect[20];
+		unsigned use_tracking:1;
+		char *refname;
+	} *entry;
+	int nr;
+	int alloc;
+};
+
+extern int parseopt_push_cas_option(const struct option *, const char *arg, int unset);
+extern int parse_push_cas_option(struct push_cas_option *, const char *arg, int unset);
+extern void clear_cas_option(struct push_cas_option *);
+
 #endif
-- 
1.8.3.2-875-g76c723c

Re: [PATCH 4/7] remote.c: add command line option parser for --lockref

[John Keeping]

On Tue, Jul 09, 2013 at 12:53:27PM -0700, Junio C Hamano wrote:
> diff --git a/remote.c b/remote.c
> index 81bc876..e9b423a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1938,3 +1938,62 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
>  	string_list_clear(&ref_names, 0);
>  	return stale_refs;
>  }
> +
> +/*
> + * Lockref aka CAS
> + */
> +void clear_cas_option(struct push_cas_option *cas)
> +{
> +	int i;
> +
> +	for (i = 0; i < cas->nr; i++)
> +		free(cas->entry->refname);

Should this be

	free(cas->entry[i]->refname);

?

> +	free(cas->entry);
> +	memset(cas, 0, sizeof(*cas));
> +}

Re: [PATCH 4/7] remote.c: add command line option parser for --lockref

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Tue, Jul 09, 2013 at 12:53:27PM -0700, Junio C Hamano wrote:
>> diff --git a/remote.c b/remote.c
>> index 81bc876..e9b423a 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1938,3 +1938,62 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
>>  	string_list_clear(&ref_names, 0);
>>  	return stale_refs;
>>  }
>> +
>> +/*
>> + * Lockref aka CAS
>> + */
>> +void clear_cas_option(struct push_cas_option *cas)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < cas->nr; i++)
>> +		free(cas->entry->refname);
>
> Should this be
>
> 	free(cas->entry[i]->refname);
>
> ?

Yes, I think so.

Re: [PATCH 4/7] remote.c: add command line option parser for --lockref

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Tue, Jul 09, 2013 at 12:53:27PM -0700, Junio C Hamano wrote:
>> diff --git a/remote.c b/remote.c
>> index 81bc876..e9b423a 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1938,3 +1938,62 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet
>>  	string_list_clear(&ref_names, 0);
>>  	return stale_refs;
>>  }
>> +
>> +/*
>> + * Lockref aka CAS
>> + */
>> +void clear_cas_option(struct push_cas_option *cas)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < cas->nr; i++)
>> +		free(cas->entry->refname);
>
> Should this be
>
> 	free(cas->entry[i]->refname);
>
> ?

Yes, more like "free(cas->entry[i].refname)".

Thanks for spotting.

>
>> +	free(cas->entry);
>> +	memset(cas, 0, sizeof(*cas));
>> +}

[PATCH 5/7] push --lockref: implement logic to populate old_sha1_expect[]

[Junio C Hamano]

This plugs the push_cas_option data collected by the command line
option parser to the transport system with a new function
apply_push_cas(), which is called after match_push_refs() has
already been called.  At this point, we know which remote we are
talking to, and what remote refs we are going to update, so we can
fill in the details that may have been missing from the command
line, such as

 (1) what abbreviated refname the user gave us matches the actual
     refname at the remote; and

 (2) which remote tracking branch in our local repository to read the
     value of the object to expect at the remote.

to populate the old_sha1_expect[] field of each of the remote ref.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c      |  6 ++++++
 builtin/send-pack.c |  3 +++
 remote.c            | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            |  3 +++
 transport.c         |  6 ++++++
 transport.h         |  4 ++++
 6 files changed, 83 insertions(+)

diff --git a/builtin/push.c b/builtin/push.c
index 31a5ba0..b0e3691 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -299,6 +299,12 @@ static int push_with_options(struct transport *transport, int flags)
 	if (thin)
 		transport_set_option(transport, TRANS_OPT_THIN, "yes");
 
+	if (!is_empty_cas(&cas)) {
+		if (!transport->smart_options)
+			die("underlying transport does not support --lockref option");
+		transport->smart_options->cas = &cas;
+	}
+
 	if (verbosity > 0)
 		fprintf(stderr, _("Pushing to %s\n"), transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 061e2b2..41dc512 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -247,6 +247,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	if (match_push_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;
 
+	if (!is_empty_cas(&cas))
+		apply_push_cas(&cas, remote, remote_refs);
+
 	set_ref_status_for_push(remote_refs, args.send_mirror,
 		args.force_update);
 
diff --git a/remote.c b/remote.c
index e9b423a..db418ff 100644
--- a/remote.c
+++ b/remote.c
@@ -1997,3 +1997,64 @@ int parse_push_cas_option(struct push_cas_option *cas, const char *arg, int unse
 		return error("cannot parse expected object name '%s'", colon + 1);
 	return 0;
 }
+
+int is_empty_cas(const struct push_cas_option *cas)
+{
+	return !cas->use_tracking_for_rest && !cas->nr;
+}
+
+/*
+ * Look at remote.fetch refspec and see if we have a remote
+ * tracking branch for the refname there.  Fill its current
+ * value in sha1[].
+ * If we cannot do so, return negative to signal an error.
+ */
+static int remote_tracking(struct remote *remote, const char *refname,
+			   unsigned char sha1[20])
+{
+	char *dst;
+
+	dst = apply_refspecs(remote->fetch, remote->fetch_refspec_nr, refname);
+	if (!dst)
+		return -1; /* no tracking ref for refname at remote */
+	if (read_ref(dst, sha1))
+		return -1; /* we know what the tracking ref is but we cannot read it */
+	return 0;
+}
+
+static void apply_cas(struct push_cas_option *cas,
+		      struct remote *remote,
+		      struct ref *ref)
+{
+	int i;
+
+	/* Find an explicit --lockref=<name>[:<value>] entry */
+	for (i = 0; i < cas->nr; i++) {
+		struct push_cas *entry = &cas->entry[i];
+		if (!refname_match(entry->refname, ref->name, ref_rev_parse_rules))
+			continue;
+		ref->expect_old_sha1 = 1;
+		if (!entry->use_tracking)
+			hashcpy(ref->old_sha1_expect, cas->entry[i].expect);
+		else if (remote_tracking(remote, ref->name, ref->old_sha1_expect))
+			ref->expect_old_no_trackback = 1;
+		return;
+	}
+
+	/* Are we using "--lockref" to cover all? */
+	if (!cas->use_tracking_for_rest)
+		return;
+
+	ref->expect_old_sha1 = 1;
+	if (remote_tracking(remote, ref->name, ref->old_sha1_expect))
+		ref->expect_old_no_trackback = 1;
+}
+
+void apply_push_cas(struct push_cas_option *cas,
+		    struct remote *remote,
+		    struct ref *remote_refs)
+{
+	struct ref *ref;
+	for (ref = remote_refs; ref; ref = ref->next)
+		apply_cas(cas, remote, ref);
+}
diff --git a/remote.h b/remote.h
index 28eb6a3..baa1c68 100644
--- a/remote.h
+++ b/remote.h
@@ -252,4 +252,7 @@ extern int parseopt_push_cas_option(const struct option *, const char *arg, int
 extern int parse_push_cas_option(struct push_cas_option *, const char *arg, int unset);
 extern void clear_cas_option(struct push_cas_option *);
 
+extern int is_empty_cas(const struct push_cas_option *);
+void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
+
 #endif
diff --git a/transport.c b/transport.c
index 98f5270..b321d6a 100644
--- a/transport.c
+++ b/transport.c
@@ -1147,6 +1147,12 @@ int transport_push(struct transport *transport,
 			return -1;
 		}
 
+		if (transport->smart_options &&
+		    transport->smart_options->cas &&
+		    !is_empty_cas(transport->smart_options->cas))
+			apply_push_cas(transport->smart_options->cas,
+				       transport->remote, remote_refs);
+
 		set_ref_status_for_push(remote_refs,
 			flags & TRANSPORT_PUSH_MIRROR,
 			flags & TRANSPORT_PUSH_FORCE);
diff --git a/transport.h b/transport.h
index b551f99..10f7556 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
 	int depth;
 	const char *uploadpack;
 	const char *receivepack;
+	struct push_cas_option *cas;
 };
 
 struct transport {
@@ -127,6 +128,9 @@ struct transport *transport_get(struct remote *, const char *);
 /* Transfer the data as a thin pack if not null */
 #define TRANS_OPT_THIN "thin"
 
+/* Check the current value of the remote ref */
+#define TRANS_OPT_CAS "cas"
+
 /* Keep the pack that was transferred if not null */
 #define TRANS_OPT_KEEP "keep"
 
-- 
1.8.3.2-875-g76c723c

[PATCH 6/7] t5533: test "push --lockref"

[Junio C Hamano]

Prepare two repositories, src and dst, the latter of which is a
clone of the former (with tracking branches), and push from the
latter into the former, using --lockref=name (using tracking ref for
"name" when updating "name"), --lockref=name:value, --lockref=name:
(i.e. check creation), and --lockref (using tracking ref for
anything that we update).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5533-push-cas.sh | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)
 create mode 100755 t/t5533-push-cas.sh

diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
new file mode 100755
index 0000000..c080467
--- /dev/null
+++ b/t/t5533-push-cas.sh
@@ -0,0 +1,176 @@
+#!/bin/sh
+
+test_description='compare & swap push force/delete safety'
+
+. ./test-lib.sh
+
+setup_srcdst_basic () {
+	rm -fr src dst &&
+	git clone --no-local . src &&
+	git clone --no-local src dst &&
+	(
+		cd src && git checkout HEAD^0
+	)
+}
+
+test_expect_success setup '
+	: create template repository
+	test_commit A &&
+	test_commit B &&
+	test_commit C
+'
+
+test_expect_success 'push to create (protected)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		test_commit D &&
+		test_must_fail git push --lockref=master: origin master &&
+		test_must_fail git push --force --lockref=master: origin master
+	) &&
+	>expect &&
+	git ls-remote src refs/heads/naster >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to create (allowed)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		test_commit D &&
+		git push --lockref=naster: origin HEAD:naster
+	) &&
+	git ls-remote dst refs/heads/master |
+	sed -e "s/master/naster/" >expect &&
+	git ls-remote src refs/heads/naster >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to update (protected)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		test_commit D &&
+		test_must_fail git push --lockref=master:master origin master &&
+		test_must_fail git push --force --lockref=master:master origin master
+	) &&
+	git ls-remote . refs/heads/master >expect &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to update (protected, tracking)' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git checkout master &&
+		test_commit D &&
+		git checkout HEAD^0
+	) &&
+	git ls-remote src refs/heads/master >expect &&
+	(
+		cd dst &&
+		test_commit E &&
+		git ls-remote . refs/remotes/origin/master >expect &&
+		test_must_fail git push --lockref=master origin master &&
+		test_must_fail git push --force --lockref=master origin master &&
+		git ls-remote . refs/remotes/origin/master >actual &&
+		test_cmp expect actual
+	) &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to update (allowed)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		test_commit D &&
+		git push --lockref=master:master^ origin master
+	) &&
+	git ls-remote dst refs/heads/master >expect &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to update (allowed, tracking)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		test_commit D &&
+		git push --lockref=master origin master
+	) &&
+	git ls-remote dst refs/heads/master >expect &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to update (still rejected with non-ff check)' '
+	setup_srcdst_basic &&
+	git ls-remote src refs/heads/master >expect &&
+	(
+		cd dst &&
+		git reset --hard HEAD^ &&
+		test_commit D &&
+		test_must_fail git push --lockref=master origin master
+	) &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to delete (protected)' '
+	setup_srcdst_basic &&
+	git ls-remote src refs/heads/master >expect &&
+	(
+		cd dst &&
+		test_must_fail git push --lockref=master:master^ origin :master &&
+		test_must_fail git push --force --lockref=master:master^ origin :master
+	) &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push to delete (allowed)' '
+	setup_srcdst_basic &&
+	(
+		cd dst &&
+		git push --lockref=master origin :master
+	) &&
+	>expect &&
+	git ls-remote src refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cover everything with default lockref (protected)' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git branch naster master^
+	)
+	git ls-remote src refs/heads/\* >expect &&
+	(
+		cd dst &&
+		test_must_fail git push --lockref origin master master:naster
+	) &&
+	git ls-remote src refs/heads/\* >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cover everything with default lockref (allowed)' '
+	setup_srcdst_basic &&
+	(
+		cd src &&
+		git branch naster master^
+	)
+	(
+		cd dst &&
+		git fetch &&
+		git push --lockref origin master master:naster
+	) &&
+	git ls-remote dst refs/heads/master |
+	sed -e "s/master/naster/" >expect &&
+	git ls-remote src refs/heads/naster >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.3.2-875-g76c723c

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

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-push.txt | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index f7dfe48..e7c8bd6 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
+	   [--lockref[=<refname>[:[<expect>]]]]
 	   [--no-verify] [<repository> [<refspec>...]]
 
 DESCRIPTION
@@ -146,6 +147,31 @@ already exists on the remote side.
 	to the `master`	branch). See the `<refspec>...` section above
 	for details.
 
+--lockref::
+--lockref=<refname>::
+--lockref=<refname>:<expect>::
+	When updating <refname> at the remote, make sure that the
+	ref currently points at <expect> (an object name), and else
+	fail the push, even if `--force` is specified.  If only
+	<refname> is given, the expected value is taken from the
+	remote-tracking branch that holds the last-observed value of
+	the <refname>.  <expect> given as an empty string means the
+	<refname> should not exist and this push must be creating
+	it.  If `--lockref` (without any value) is given, make sure
+	each ref this push is going to update points at the object
+	our remote-tracking branch for it points at.
++
+This is meant to make `--force` safer to use.  Imagine that you have
+to rebase what you have already published.  You will have to
+`--force` the push to replace the history you originally published
+with the rebased history.  If somebody else built on top of your
+original history while you are rebasing, the tip of the branch at
+the remote may advance with her commit, and blindly pushing with
+`--force` will lose her work.  By using this option to specify that
+you expect the history you are updating is what you rebased and want
+to replace, you can make sure other people's work will not be losed
+by a forced push. in such a case.
+
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
 	passed in the invocation. In this case, 'git push' derives the
-- 
1.8.3.2-875-g76c723c

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

[Aaron Schrab]

At 12:53 -0700 09 Jul 2013, Junio C Hamano <gitster@pobox.com> wrote:
>+This is meant to make `--force` safer to use.  Imagine that you have
>+to rebase what you have already published.  You will have to
>+`--force` the push to replace the history you originally published
>+with the rebased history.  If somebody else built on top of your
>+original history while you are rebasing, the tip of the branch at
>+the remote may advance with her commit, and blindly pushing with
>+`--force` will lose her work.  By using this option to specify that
>+you expect the history you are updating is what you rebased and want
>+to replace, you can make sure other people's work will not be losed
>+by a forced push. in such a case.

s/losed/lost/

How does this behave if --force is not used?  I think it would be best 
if it was a no-op in that case to make it easy to add a config option to 
turn this on by default.

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

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> How does this behave if --force is not used?

Both the usual "must fast-forward" safety and the "ref should not
have moved" safety apply.

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

[Johannes Sixt]

Am 09.07.2013 21:53, schrieb Junio C Hamano:
> +--lockref::
> +--lockref=<refname>::
> +--lockref=<refname>:<expect>::
> ...
> +This is meant to make `--force` safer to use.

This is a contradiction. "--force" means "I mean it, dude", and not "I
mean it sometimes". It would make sense if this sentence were "This is
meant to make `+refspec` safer to use."

Do you intend to require users to opt in to safety by saying --lockref
until the end of time? Which makes it actually usable only for scripts
and aliases. How do you override when the safety triggers, e.g., in an
alias that uses --force --lockref? Add --i-really-mean-it?

Or do we want to make --lockref the default at least for cases where
necessary ingredients can be derived automatically, perhaps in Git 3.0?
Then, how do you override when the safety triggers? Add --i-really-mean-it?

IMO, the way forward is:

1. Teach users to use +refspec to force-push. Do not encourage 'push
--force'.

2. Add --lockref as an opt-in for +refspec. Do not apply the safety to
'push --force'. (Current users and scripts do not see a behavior change
because they do not use --lockref, either.)

3. Make --lockref behavior the default at least for +refspec. Then 'push
--force' is still able to override the safety.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 09.07.2013 21:53, schrieb Junio C Hamano:
>> +--lockref::
>> +--lockref=<refname>::
>> +--lockref=<refname>:<expect>::
>> ...
>> +This is meant to make `--force` safer to use.
>
> This is a contradiction. "--force" means "I mean it, dude", and not "I
> mean it sometimes". It would make sense if this sentence were "This is
> meant to make `+refspec` safer to use."

No, this *IS* making --force safer by letting you to say in addition
to --force alone which is blind, add --lockref to defeat it.

I do not see any good reason to change the samentics of "+refspec"
for something like this.  "+refspec" and "--force refspec" have
meant the same thing forever.  If --lockref adds safety to +refspec,
the same safety should apply to "--force refspec".

> Do you intend to require users to opt in to safety by saying --lockref
> until the end of time?

For normal users this is *NOT* necessary.  I do not know where
people are getting the idea of making it default.

Rewinding a branch, needing to --force, is an exceptional case.

> Which makes it actually usable only for scripts
> and aliases. How do you override when the safety triggers, e.g., in an
> alias that uses --force --lockref?

The original request for this feature did come from script writers,
who want to spin

	until
                git fetch &&
                ... magic integrate of the ongoing work ... &&
                git push --lockref
	do
        	: spin
	done

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

[Johannes Sixt]

Am 09.07.2013 22:37, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 09.07.2013 21:53, schrieb Junio C Hamano:
>>> +--lockref::
>>> +--lockref=<refname>::
>>> +--lockref=<refname>:<expect>::
>>> ...
>>> +This is meant to make `--force` safer to use.
>>
>> This is a contradiction. "--force" means "I mean it, dude", and not "I
>> mean it sometimes". It would make sense if this sentence were "This is
>> meant to make `+refspec` safer to use."
> 
> No, this *IS* making --force safer by letting you to say in addition
> to --force alone which is blind, add --lockref to defeat it.
> 
> I do not see any good reason to change the samentics of "+refspec"
> for something like this.  "+refspec" and "--force refspec" have
> meant the same thing forever.

So what? They still mean the same thing as long as --lockref is not used.

>  If --lockref adds safety to +refspec,
> the same safety should apply to "--force refspec".

No. --force means "I know what I am doing, no safety needed, thank you".

By applying the safety to --force as well, you lose it as the obvious
tool that overrides the safety.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> No. --force means "I know what I am doing, no safety needed, thank you".

I sympathize the desire to keep a big red button to override
everything, but it is still not clear how these two independent
safety should work together and should possibly seletively be
overriden.

A proposed ref update can be in one of the four:

 1. The update fast-forwards, and the ref to be updated is at the
    expected place (or you simply do not care what the current value
    is);

 2. The update does not fast-forward, and the ref to be updated is
    at the expected place (or you simply do not care what the
    current value is);

 3. The update fast-forwards, but the ref to be updated is not at the
    expected place; or

 4. The update does not fast-forward, and the ref to be updated is
    not at the expected place.

So far we had only 1. and 2. because we did not have this "old value
has to be at X".  And --force has been the way to allow 2. to go
through.

Now we are adding 3. and 4. to the mix.

If --force were the big red button that allows all four, is that
sufficient to cover the necessary cases, especially given that some
people seem to want to make the --lockref on by default (implying
that 3. and 4. will both fail by default unless forced in some way)?
For example, would there be a case where we want to allow 3. but not
4. (or vice versa)?

You _could_ structure the safety into hierarchies:

 * safest: no-ff will be rejected, and current value at an
   unexpected place is also rejected.  That would be:

   $ git push --lockref

 * --lockref only: no-ff is not even checked, but current value
     must be at an expected place.  How would that be spelled???

   $ git push --lockref ???

 * --force: anything goes.

   $ git push --force --no-lockref

Where does "ff-check only" fit in the hierarchy?

This is one of the reasons why the original design of "--lockref"
was to even countermand "allow non-fast-forward" (which is the
original meaning of "--force").

I _think_ I am OK if we introduced "--allow-no-ff" that means the
current "--force" (i.e. "rewinding is OK"), that does not defeat the
"--lockref" safety.  That is the intended application (you know that
push does not fast-forward because you rebased, but you also want to
make sure there is nothing you are losing by enforcing --lockref
safety).

If that is what happens, then I think "--force" that means "anything
goes" makes sense.

With the posted series, adding "--force --no-lockref" to the command
line is how to spell that big red button.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I _think_ I am OK if we introduced "--allow-no-ff" that means the
> current "--force" (i.e. "rewinding is OK"), that does not defeat the
> "--lockref" safety.  That is the intended application (you know that
> push does not fast-forward because you rebased, but you also want to
> make sure there is nothing you are losing by enforcing --lockref
> safety).
>
> If that is what happens, then I think "--force" that means "anything
> goes" makes sense.

Or perhaps you were implicitly assuming that "--lockref" would
automatically mean "I know I am rewinding, so as soon as I say
--lockref, I mean --allow-no-ff", and I did not realize that.

If that is the semantics you are proposing, then I think it makes
sense to make "--force" the big red button that lets anything go.

I was considering "--lockref" to be orthogonal to the traditional
"ff only check", and rejecting a push when the updated ref's current
value is expected (i.e. --lockref satisfied) but the update does not
fast-forward, and that was where my resistance to allow "--force" to
override "--lockref" comes from (because otherwise there is no way
to say "I know I want to bypass 'ff-only' check, but instead make
sure the current value is this").

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

[Johannes Sixt]

Am 10.07.2013 01:08, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I _think_ I am OK if we introduced "--allow-no-ff" that means the
>> current "--force" (i.e. "rewinding is OK"), that does not defeat the
>> "--lockref" safety.  That is the intended application (you know that
>> push does not fast-forward because you rebased, but you also want to
>> make sure there is nothing you are losing by enforcing --lockref
>> safety).
>>
>> If that is what happens, then I think "--force" that means "anything
>> goes" makes sense.
> 
> Or perhaps you were implicitly assuming that "--lockref" would
> automatically mean "I know I am rewinding, so as soon as I say
> --lockref, I mean --allow-no-ff", and I did not realize that.

That's what I mean, sort of. Because of your 4 cases of a ref update, I
do not think that

> 3. The update fast-forwards, but the ref to be updated is not at the
>    expected place; or

is important to consider. The point of --lockref is to avoid data loss,
but if the push is fast-forward, there is no data loss.

> If that is the semantics you are proposing, then I think it makes
> sense to make "--force" the big red button that lets anything go.
> 
> I was considering "--lockref" to be orthogonal to the traditional
> "ff only check", and rejecting a push when the updated ref's current
> value is expected (i.e. --lockref satisfied) but the update does not
> fast-forward, and that was where my resistance to allow "--force" to
> override "--lockref" comes from (because otherwise there is no way
> to say "I know I want to bypass 'ff-only' check, but instead make
> sure the current value is this").

Again: Why not just define +refspec as the way to achieve this check?

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Or perhaps you were implicitly assuming that "--lockref" would
>> automatically mean "I know I am rewinding, so as soon as I say
>> --lockref, I mean --allow-no-ff", and I did not realize that.
>
> That's what I mean, sort of. Because of your 4 cases of a ref update, I
> do not think that
>
>> 3. The update fast-forwards, but the ref to be updated is not at the
>>    expected place; or
>
> is important to consider. The point of --lockref is to avoid data loss,
> but if the push is fast-forward, there is no data loss.
>
>> If that is the semantics you are proposing, then I think it makes
>> sense to make "--force" the big red button that lets anything go.

I have a reroll that goes in that direction.

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Again: Why not just define +refspec as the way to achieve this check?

What justification do we have to break existing people's
configuration that says something like:

	[remote "ko"]
		url = kernel.org:/pub/scm/git/git.git
                push = master
                push = next
                push = +pu
                push = maint

by adding a _new_ requirement they may not be able to satisify?
Notice that the above is a typical "push only" publishing point,
where you do not need any remote tracking branches.

I am not opposed if your proposal were to introduce a new syntax
element that calls for this new feature, e.g.

	[remote "ko"]
		url = kernel.org:/pub/scm/git/git.git
                push = *pu
                fetch = +refs/heads/*:refs/remotes/ko/*

but changing what "+" means to something new will simply not fly.

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

[Johannes Sixt]

Am 12.07.2013 00:14, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Again: Why not just define +refspec as the way to achieve this check?
> 
> What justification do we have to break existing people's
> configuration that says something like:
> 
> 	[remote "ko"]
> 		url = kernel.org:/pub/scm/git/git.git
>                 push = master
>                 push = next
>                 push = +pu
>                 push = maint
> 
> by adding a _new_ requirement they may not be able to satisify?
> Notice that the above is a typical "push only" publishing point,
> where you do not need any remote tracking branches.

Why would it break? When you do not specify --lockref, there is no
change whatsoever.

To achieve any safety at all for these push-only refs, you have to be
very explicit by saying --lockref=pu:$myoldpu
--lockref=master:$myoldmaster etc, and what is wrong if in this case
--lockref semantics are applied, but only pu is allowed to be no-ff?

> I am not opposed if your proposal were to introduce a new syntax
> element that calls for this new feature, e.g.
> 
> 	[remote "ko"]
> 		url = kernel.org:/pub/scm/git/git.git
>                 push = *pu
>                 fetch = +refs/heads/*:refs/remotes/ko/*
> 
> but changing what "+" means to something new will simply not fly.

I still do not see why we need two different kinds of ways to spell the
same strong kind of override (--force and +refspec) under the presence
of --lockref, and why we need a third one (--allow-no-ff) to give a
weaker kind of override (that makes sense only when --lockref was given
in the first place).

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 12.07.2013 00:14, schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> 
>>> Again: Why not just define +refspec as the way to achieve this check?
>> 
>> What justification do we have to break existing people's
>> configuration that says something like:
>> 
>> 	[remote "ko"]
>> 		url = kernel.org:/pub/scm/git/git.git
>>                 push = master
>>                 push = next
>>                 push = +pu
>>                 push = maint
>> 
>> by adding a _new_ requirement they may not be able to satisify?
>> Notice that the above is a typical "push only" publishing point,
>> where you do not need any remote tracking branches.
>
> Why would it break? When you do not specify --lockref, there is no
> change whatsoever.

I thought your suggestion "Why not just define +pu as the way to
achieve _THIS_ check?" was to make +pu to mean

	git push ko --lockref pu

which would mean "check refs/remotes/ko/pu and make sure the remote
side still is at that commit".

If that is not what you meant, please clarify what _THIS_ is.

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

[Johannes Sixt]

Am 12.07.2013 19:40, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 12.07.2013 00:14, schrieb Junio C Hamano:
>>> Johannes Sixt <j6t@kdbg.org> writes:
>>>
>>>> Again: Why not just define +refspec as the way to achieve this check?
>>>
>>> What justification do we have to break existing people's
>>> configuration that says something like:
>>>
>>> 	[remote "ko"]
>>> 		url = kernel.org:/pub/scm/git/git.git
>>>                 push = master
>>>                 push = next
>>>                 push = +pu
>>>                 push = maint
>>>
>>> by adding a _new_ requirement they may not be able to satisify?
>>> Notice that the above is a typical "push only" publishing point,
>>> where you do not need any remote tracking branches.
>>
>> Why would it break? When you do not specify --lockref, there is no
>> change whatsoever.
> 
> I thought your suggestion "Why not just define +pu as the way to
> achieve _THIS_ check?" was to make +pu to mean
> 
> 	git push ko --lockref pu
> 
> which would mean "check refs/remotes/ko/pu and make sure the remote
> side still is at that commit".
> 
> If that is not what you meant, please clarify what _THIS_ is.

We have three independent options that the user can choose in any
combination:

 o --force given or not;

 o --lockref semantics enabled or not;

 o refspec with or without +;

and these two orthogonal preconditions of the push

 o push is fast-forward or it is not ("ff", "noff");

 o the branch at the remote is at the expected rev or it is not
   ("match", "mismatch").

Here is a table with the expected outcome. "ok" means that the push is
allowed(*), "fail" means that the push is denied. (Four more lines with
--force are omitted because they have "ok" in all spots.)

                       ff   noff     ff      noff
                      match match mismatch mismatch

--lockref +refspec     ok    ok    denied   denied
--lockref  refspec     ok  denied  denied   denied
          +refspec     ok    ok      ok       ok
           refspec     ok  denied    ok     denied

Notice that without --lockref semantics enabled, +refspec and refspec
keep the current behavior.

(*) As we are talking only about the client-side of the push here, I'm
saying "allowed" instead of "succeeds" because the server can have
additional restrictions that can make the push fail.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> We have three independent options that the user can choose in any
> combination:
>
>  o --force given or not;
>
>  o --lockref semantics enabled or not;
>
>  o refspec with or without +;
>
> and these two orthogonal preconditions of the push
>
>  o push is fast-forward or it is not ("ff", "noff");
>
>  o the branch at the remote is at the expected rev or it is not
>    ("match", "mismatch").
>
> Here is a table with the expected outcome. "ok" means that the push is
> allowed(*), "fail" means that the push is denied. (Four more lines with
> --force are omitted because they have "ok" in all spots.)
>
>                        ff   noff     ff      noff
>                       match match mismatch mismatch
>
> --lockref +refspec     ok    ok    denied   denied
> --lockref  refspec     ok  denied  denied   denied

I am confused with these.  The latter is the most typical:

	git fetch
        git checkout topic
        git rebase topic
	git push --lockref topic

where we know it is "noff" already, and we just want to make sure
that nobody mucked with our remote while we are rebasing.

If nobody updated the remote, why should this push be denied?  And in
order to make it succeed, you need to force with +refspec or --force,
but that would bypass match/mismatch safety, which makes the whole
"make sure the other end is unchanged" safety meaningless, no?

>           +refspec     ok    ok      ok       ok

This is traditional --force.

>            refspec     ok  denied    ok     denied

We are not asking for --lockref, so match/mismatch does not affect
the outcome.

> Notice that without --lockref semantics enabled, +refspec and refspec
> keep the current behavior.

But I do not think the above table with --lockref makes much sense.

Let's look at noff/match case.  That is the only interesting one.

This should fail:

	git push topic

due to no-ff.

Your table above makes this fail:

        git push --lockref topic

and the user has to force it, like this?

	git push --lockref --force topic ;# or alternatively
        git push --lockref +topic

Why is it even necessary?

If you make

	git push --lockref topic

succeed in noff/match case, everything makes more sense to me.

The --lockref option is merely a weaker form of --force but still a
way to override the noff check.  If the user wants to keep noff
check, the user can simply choose not to use the option.

Of course, that form should fail if "mismatch".  And then you can
force it,

	git push --force [--lockref] topic

As "--force" is "anything goes", it does not matter if you give the
other option on the command line.

> (*) As we are talking only about the client-side of the push here, I'm
> saying "allowed" instead of "succeeds" because the server can have
> additional restrictions that can make the push fail.

Yes, you and I have known from the beginning that we are in
agreement on that, but it is a good idea to explicitly say so for
the sake of bystanders.

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

[Johannes Sixt]

Am 12.07.2013 23:19, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> We have three independent options that the user can choose in any
>> combination:
>>
>>  o --force given or not;
>>
>>  o --lockref semantics enabled or not;
>>
>>  o refspec with or without +;
>>
>> and these two orthogonal preconditions of the push
>>
>>  o push is fast-forward or it is not ("ff", "noff");
>>
>>  o the branch at the remote is at the expected rev or it is not
>>    ("match", "mismatch").
>>
>> Here is a table with the expected outcome. "ok" means that the push is
>> allowed(*), "fail" means that the push is denied. (Four more lines with
>> --force are omitted because they have "ok" in all spots.)
>>
>>                        ff   noff     ff      noff
>>                       match match mismatch mismatch
>>
>> --lockref +refspec     ok    ok    denied   denied
>> --lockref  refspec     ok  denied  denied   denied
> 
> I am confused with these.  The latter is the most typical:
> 
> 	git fetch
>         git checkout topic
>         git rebase topic
> 	git push --lockref topic
> 
> where we know it is "noff" already, and we just want to make sure
> that nobody mucked with our remote while we are rebasing.

Today (without --lockref), the above sequence would fail to push.
(Because there is no + and no --force.)

> If nobody updated the remote, why should this push be denied?  And in
> order to make it succeed, you need to force with +refspec or --force,
> but that would bypass match/mismatch safety, which makes the whole
> "make sure the other end is unchanged" safety meaningless, no?

I am suggesting that +refspec would *not* override the match/mismatch
safety, but --force would.

> 
>>           +refspec     ok    ok      ok       ok
> 
> This is traditional --force.
> 
>>            refspec     ok  denied    ok     denied
> 
> We are not asking for --lockref, so match/mismatch does not affect
> the outcome.

I think you are worried that a deviation from the principle that
+refspec == --force hurts current users. But I am arguing that this is
not the case because "current" users do not use --lockref. As you have
seen from the table, without --lockref there is *no change* in behavior.

I still have not seen an example where +refspec != --force would have
unexpected consequences. (The inequality is merely that +refspec fails
on mismatch when --lockref was also given while --force does not.)

>> Notice that without --lockref semantics enabled, +refspec and refspec
>> keep the current behavior.
> 
> But I do not think the above table with --lockref makes much sense.
> 
> Let's look at noff/match case.  That is the only interesting one.
> 
> This should fail:
> 
> 	git push topic
> 
> due to no-ff.

Yes.

> Your table above makes this fail:
> 
>         git push --lockref topic
> 
> and the user has to force it,

Of course.

> like this?
> 
> 	git push --lockref --force topic ;# or alternatively
>         git push --lockref +topic
> 
> Why is it even necessary?

Because it is no-ff. How do you achieve the push today (without
--lockref)? You use one of these two options. It does not change with
--lockref.

> If you make
> 
> 	git push --lockref topic
> 
> succeed in noff/match case, everything makes more sense to me.

Not to me, obviously ;)

> The --lockref option is merely a weaker form of --force but still a
> way to override the noff check.

No; --lockref only adds the check that the destination is at the
expected revision, but does *NOT* override the no-ff check. Why should
it? (This is not a rethoric question.)

(I think I said differently in an earlier messages, but back then things
were still blurry. The table in my previous message is what I mean.)

>  If the user wants to keep noff
> check, the user can simply choose not to use the option.

No. If the user wants to keep the no-ff check, she does not use the + in
the refspec and does not use --force. (Just like today.)

> Of course, that form should fail if "mismatch".  And then you can
> force it,
> 
> 	git push --force [--lockref] topic
> 
> As "--force" is "anything goes", it does not matter if you give the
> other option on the command line.

... or the + in the refsepc.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> I am suggesting that +refspec would *not* override the match/mismatch
> safety, but --force would.

OK.

I earlier did not read from your message that you wanted to change
"+refspec" to mean "allow non-ff push", so the two entries in your
table:

>                        ff   noff     ff      noff
>                       match match mismatch mismatch
>
> --lockref +refspec     ok    ok    denied   denied
> --lockref  refspec     ok  denied  denied   denied

did not make sense to me.  If you are making "+refspec" to mean
"--allow-no-ff refspec", then above is at least internally
consistent.

>> Let's look at noff/match case.  That is the only interesting one.
>> 
>> This should fail:
>> 
>> 	git push topic
>> 
>> due to no-ff.
>
> Yes.
>
>> Your table above makes this fail:
>> 
>>         git push --lockref topic
>> 
>> and the user has to force it,
>
> Of course.
>
>> like this?
>> 
>> 	git push --lockref --force topic ;# or alternatively
>>         git push --lockref +topic
>> 
>> Why is it even necessary?

> Because it is no-ff. How do you achieve the push today (without
> --lockref)? You use one of these two options. It does not change with
> --lockref.

But by going that route, you are making --lockref _less_ useful, no?

"git push topic" in no-ff/match case fails as it should.  The whole
purpose of "--lockref" is to make this case easier and safer than
the today's system, where the anything-goes "--force" is the only
way to make this push.  We want to give a user who

 - rebased the topic, and

 - knows where the topic at the remote should be

a way to say "I know I am pushing a no-ff, and I want to make sure
the current value is this" in order to avoid losing somebody else's
work queued on top of the topic at the remote while he was rebasing.

You _CAN_ introduce a new --allow-no-ff at the same time and fail a
no-ff/match push:

	git push --lockref topic

and then allow it back with:

	git push --lockref --allow-no-ff topic
	git push --lockref +topic ;# +topic is now --allow-no-ff topic

but why _SHOULD_ we?  As soon as the user _says_ --lockref, the user
is telling us he is pushing a no-ff.  If that is not the case, the
user can push without --lockref in the first place.

The only potential thing you are gaining with such a change is that
you are allowing people to say "this will fast-forward _and_ the I
know the current value; if either of these two assumptions is
violated, please fail this push".

If "--lockref" automatically implies "--allow-no-ff" (the design in
the reposted patch), you cannot express that combination.  But once
you use "--lockref" in such a situation , for the push to succeed,
you know that the push replaces not just _any_ ancestor of what you
are pushing, but replaces the exact current value.  So I do not think
your implicit introduction of --allow-no-ff via redefining the
semantics of the plus prefix is not adding much value (if any),
while making the common case less easy to use.

> No; --lockref only adds the check that the destination is at the
> expected revision, but does *NOT* override the no-ff check.

You _could_ do it in that way, but that is less useful.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If "--lockref" automatically implies "--allow-no-ff" (the design in
> the reposted patch), you cannot express that combination.  But once
> you use "--lockref" in such a situation , for the push to succeed,
> you know that the push replaces not just _any_ ancestor of what you
> are pushing, but replaces the exact current value.  So I do not think
> your implicit introduction of --allow-no-ff via redefining the
> semantics of the plus prefix is not adding much value (if any),
> while making the common case less easy to use.
>
>> No; --lockref only adds the check that the destination is at the
>> expected revision, but does *NOT* override the no-ff check.
>
> You _could_ do it in that way, but that is less useful.

Another issue I have with the proposal is that we close the door to
"force only this one" convenience we have with "+ref" vs "--force
ref".  Assuming that it is useful to require lockref while still
making sure that the usual "must fast-forward" rule is followed (if
that is not the case, I do not see a reason why your proposal is any
useful---am I missing something?), I would prefer to allow users a
way to decorate this basic syntax to say:

    git push --lockref master jch pu

things like

 (1) pu may not fast-forward and please override that "must
     fast-forward" check from it, while still keeping the lockref
     safety (e.g. "+pu" that does not --force, which is your
     proposal);

 (2) any of them may not fast-forward and please override that "must
     fast-forward" check from it, while still keeping the lockref
     safety (without adding "--allow-no-ff", I do not see how it is
     possible with your proposal, short of forcing user to add "+"
     everywhere);

 (3) I know jch does not fast-forward so please override the "must
     fast-forward", but still apply the lockref safety, pu may not
     even satisfy lockref safety so please force it (as the "only
     force this one" semantics is removed from "+", I do not see how
     it is possible with your proposal).

So I would understand if your proposal _were_ to

 * add "--allow-no-ff" option;

 * change the meaning of "+ref" to "--allow-no-ff for only this
   ref"; and

 * add a new "*ref" (or whatever new syntax) to still allow people
   to say "--force only this ref".

but we still need to assume that it makes sense to ask lockref but
still want to ensure the update fast-forwards.  I personally do not
think it does [*1*].

The semantics the posted patch (rerolled to allow "--force" push
anything) implements lets "--lockref" to imply "--allow-no-ff" and
that makes it much simpler; we do not have to deal with any of the
above complexity.


[Footnote]

 *1* The assurance --lockref gives is a lot stronger than "must
     fast-forward".  You may have fetched the topic whose tip was at
     commit X, and rebased it on top of X~4 to create a new history
     leading to Y.

           o----o----Y
          /
     o---o----o----o----o----X
	X~4

     When you "git push --lockref=topic:X Y:X", you are requiring
     their tip to be still at X.  Other people's change cannot be to
     add something on top of X (which will be lost if we replace the
     tip of the topic with Y).

     If your change were not a rebase but to build one of you own:

     o---o----o----o----o----X---Y

     your "git push --lockref=topic:X Y:X" still requires the tip is
     at X.  If somebody rewound the tip to X~2 in the meantime
     (because they decided the tip 2 commits were not good), your
     "git push Y:X" without the "--lockref" will lose their rewind,
     because Y will still be a fast-forward update of X~2.
     "--lockref=topic:X" will protect you in this case as well.

     So I think "--lockref" that automatically disables "must
     fast-forward" check is the right thing to do, as we are
     replacing the weaker "must fast-forward" with something
     stronger.  I do not think we are getting anything from forcing
     the user to say "--allow-no-ff" with "+ref" syntax when the
     user says "--lockref".  It is not making it safer, and it is
     making it less convenient.

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

[Johannes Sixt]

Am 13.07.2013 22:08, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If "--lockref" automatically implies "--allow-no-ff" (the design in
>> the reposted patch), you cannot express that combination.  But once
>> you use "--lockref" in such a situation , for the push to succeed,
>> you know that the push replaces not just _any_ ancestor of what you
>> are pushing, but replaces the exact current value.  So I do not think
>> your implicit introduction of --allow-no-ff via redefining the
>> semantics of the plus prefix is not adding much value (if any),
>> while making the common case less easy to use.
>>
>>> No; --lockref only adds the check that the destination is at the
>>> expected revision, but does *NOT* override the no-ff check.
>>
>> You _could_ do it in that way, but that is less useful.
> 
> Another issue I have with the proposal is that we close the door to
> "force only this one" convenience we have with "+ref" vs "--force
> ref".  Assuming that it is useful to require lockref while still
> making sure that the usual "must fast-forward" rule is followed (if
> that is not the case, I do not see a reason why your proposal is any
> useful---am I missing something?),

The ability to express "require both fast-forward and --lockref" is just
an artefact of the independence of fast-forward-ness and --lockref in my
proposal. It is not something that I think is absolutely necessary.

> I would prefer to allow users a
> way to decorate this basic syntax to say:
> 
>     git push --lockref master jch pu
> 
> things like
> 
>  (1) pu may not fast-forward and please override that "must
>      fast-forward" check from it, while still keeping the lockref
>      safety (e.g. "+pu" that does not --force, which is your
>      proposal);

That must be a misunderstanding. In my proposal

    git push --lockref +pu

would do what you need here. I don't know where you get the idea that
these two

    git push --lockref +pu
    git push +pu

would be different with regard to non-fast-forward-ness. The table
entries were correct.

[Please do not use the option name "--force" in the discussion unless
you mean "all kinds of safety off".]

>  (2) any of them may not fast-forward and please override that "must
>      fast-forward" check from it, while still keeping the lockref
>      safety (without adding "--allow-no-ff", I do not see how it is
>      possible with your proposal, short of forcing user to add "+"
>      everywhere);

The point of my proposal is to force users to add + when they want to
allow non-fast-forward. Usually, this is shorter to type anyway than to
insert --force or --allow-no-ff in the command.

> 
>  (3) I know jch does not fast-forward so please override the "must
>      fast-forward", but still apply the lockref safety, pu may not
>      even satisfy lockref safety so please force it (as the "only
>      force this one" semantics is removed from "+", I do not see how
>      it is possible with your proposal).

I think

   git push --lockref=jch +jch +pu

would do.

> The semantics the posted patch (rerolled to allow "--force" push
> anything) implements lets "--lockref" to imply "--allow-no-ff" and
> that makes it much simpler; we do not have to deal with any of the
> above complexity.

But see my other post, where this hurts users who have a fast-forward
push refspec configured.

> [Footnote]
> 
>  *1* The assurance --lockref gives is a lot stronger than "must
>      fast-forward".
...
>      If your change were not a rebase but to build one of you own:
> 
>      o---o----o----o----o----X---Y
> 
>      your "git push --lockref=topic:X Y:X" still requires the tip is
>      at X.  If somebody rewound the tip to X~2 in the meantime
>      (because they decided the tip 2 commits were not good), your
>      "git push Y:X" without the "--lockref" will lose their rewind,
>      because Y will still be a fast-forward update of X~2.
>      "--lockref=topic:X" will protect you in this case as well.

Good point.

>      So I think "--lockref" that automatically disables "must
>      fast-forward" check is the right thing to do, as we are
>      replacing the weaker "must fast-forward" with something
>      stronger.

But I do not share this conclusion. My conclusion is that your proposal
replaces one kind of check with a very different kind of check.

>      I do not think we are getting anything from forcing
>      the user to say "--allow-no-ff" with "+ref" syntax when the
>      user says "--lockref".

Is this the same misunderstanding? My proposal does not require
--allow-no-ff with +ref syntax when --lockref is used.

-- Hannes

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

[John Keeping]

On Sat, Jul 13, 2013 at 01:08:09PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If "--lockref" automatically implies "--allow-no-ff" (the design in
> > the reposted patch), you cannot express that combination.  But once
> > you use "--lockref" in such a situation , for the push to succeed,
> > you know that the push replaces not just _any_ ancestor of what you
> > are pushing, but replaces the exact current value.  So I do not think
> > your implicit introduction of --allow-no-ff via redefining the
> > semantics of the plus prefix is not adding much value (if any),
> > while making the common case less easy to use.
> >
> >> No; --lockref only adds the check that the destination is at the
> >> expected revision, but does *NOT* override the no-ff check.
> >
> > You _could_ do it in that way, but that is less useful.
> 
> Another issue I have with the proposal is that we close the door to
> "force only this one" convenience we have with "+ref" vs "--force
> ref".  Assuming that it is useful to require lockref while still
> making sure that the usual "must fast-forward" rule is followed (if
> that is not the case, I do not see a reason why your proposal is any
> useful---am I missing something?), I would prefer to allow users a
> way to decorate this basic syntax to say:
> 
>     git push --lockref master jch pu
> 
> things like
> 
>  (1) pu may not fast-forward and please override that "must
>      fast-forward" check from it, while still keeping the lockref
>      safety (e.g. "+pu" that does not --force, which is your
>      proposal);
> 
>  (2) any of them may not fast-forward and please override that "must
>      fast-forward" check from it, while still keeping the lockref
>      safety (without adding "--allow-no-ff", I do not see how it is
>      possible with your proposal, short of forcing user to add "+"
>      everywhere);
> 
>  (3) I know jch does not fast-forward so please override the "must
>      fast-forward", but still apply the lockref safety, pu may not
>      even satisfy lockref safety so please force it (as the "only
>      force this one" semantics is removed from "+", I do not see how
>      it is possible with your proposal).

I haven't been following this thread too closely, but I was assuming
that the interface would be something like this:

    git push origin +master
    git push --force origin master

mean the same thing and do what they do now.

    git push origin *master
    git push --lockref origin master

both mean the same thing: using the new compare-and-swap mode only
update master if the remote side corresponds to remotes/origin/master
[1].

    git push origin *master:refs/heads/master:@{1}

means to push the local ref master to the remote ref refs/heads/master
if it currently points at "@{1}".

In this scenario, giving both --lockref and --force should be an error
because the user is probably confused (the obvious interpretation is
that --force wins, but I don't think that's sensible).

I'm not sure what should happen with:

    git push --force origin *master

where it appears that the user is asking for a compare-and-swap update
of master but the --force is overriding this.  I think we have to let
--force win because when the refspec comes from remote.<name>.push we
have to let the command-line --force override the specified behaviour.

I don't particularly like the name --lockref, the original --cas feels
more descriptive to me.


[1] In fact, I suspect this would have to be "the ref that
    refs/heads/master maps to using remote.origin.fetch".

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

[Johannes Sixt]

Am 13.07.2013 20:14, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>>> Your table above makes this fail:
>>>
>>>         git push --lockref topic
>>>
>>> and the user has to force it,
>>
>> Of course.
>>
>>> like this?
>>>
>>> 	git push --lockref --force topic ;# or alternatively
>>>         git push --lockref +topic
>>>
>>> Why is it even necessary?
> 
>> Because it is no-ff. How do you achieve the push today (without
>> --lockref)? You use one of these two options. It does not change with
>> --lockref.
> 
> But by going that route, you are making --lockref _less_ useful, no?
> 
> "git push topic" in no-ff/match case fails as it should.  The whole
> purpose of "--lockref" is to make this case easier and safer than
> the today's system, where the anything-goes "--force" is the only
> way to make this push.  We want to give a user who
> 
>  - rebased the topic, and
> 
>  - knows where the topic at the remote should be
> 
> a way to say "I know I am pushing a no-ff, and I want to make sure
> the current value is this" in order to avoid losing somebody else's
> work queued on top of the topic at the remote while he was rebasing.
> 
> You _CAN_ introduce a new --allow-no-ff at the same time and fail a
> no-ff/match push:
> 
> 	git push --lockref topic
> 
> and then allow it back with:
> 
> 	git push --lockref --allow-no-ff topic
> 	git push --lockref +topic ;# +topic is now --allow-no-ff topic
> 
> but why _SHOULD_ we?  As soon as the user _says_ --lockref, the user
> is telling us he is pushing a no-ff.  If that is not the case, the
> user can push without --lockref in the first place.
> 
> The only potential thing you are gaining with such a change is that
> you are allowing people to say "this will fast-forward _and_ the I
> know the current value; if either of these two assumptions is
> violated, please fail this push".
> 
> If "--lockref" automatically implies "--allow-no-ff" (the design in
> the reposted patch), you cannot express that combination.  But once
> you use "--lockref" in such a situation , for the push to succeed,
> you know that the push replaces not just _any_ ancestor of what you
> are pushing, but replaces the exact current value.  So I do not think
> your implicit introduction of --allow-no-ff via redefining the
> semantics of the plus prefix is not adding much value (if any),
> while making the common case less easy to use.
> 
>> No; --lockref only adds the check that the destination is at the
>> expected revision, but does *NOT* override the no-ff check.
> 
> You _could_ do it in that way, but that is less useful.

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.

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.

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. 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.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> 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.

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

[Johannes Sixt]

Am 14.07.2013 21:17, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>> 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

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

[Jonathan Nieder]

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

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

[Jonathan Nieder]

Jonathan Nieder wrote:
> 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?

(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 '~'.)

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

[Johannes Sixt]

Am 14.07.2013 22:34, schrieb Jonathan Nieder:
> 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?

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

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

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 14.07.2013 22:34, schrieb Jonathan Nieder:

>> 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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 	(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

t is clear you want to allow-no-ff in this case (otherwise the push
will not go through), and that is what the "--force" option meant in
the old world.  The compare-and-swap safety is to help this case by
letting you say

	git push --lockref

which is a weaker form of "--force".  We ignore "fast-forward"-ness,
like the current "--force" does, but replace it with another form of
safety "we know replacing this old value with what we are pushing is
OK---if somebody updated the ref in the meantime, then the push is
not OK, so please fail".

> 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.

I am not sure I follow.  Do other contributors update this remote
repository?  They are "only using fast-forward updates", so their
updates may not lose anything we pushed, but with "--force", aren't
you losing their work on top of yours?

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 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.
>
> I am not sure I follow.  Do other contributors update this remote
> repository?  They are "only using fast-forward updates", so their
> updates may not lose anything we pushed, but with "--force", aren't
> you losing their work on top of yours?

Yep, I meant that when you really *do* want to force a push
regardless of what's on the remote end, the current --force behavior
is more useful than --lockref.

The example I used to introduce (5c) is too vague to be useful.  A
more compelling example (to me, at least) is the one from later in
that message involving a relay, which does not involve other
contributors at all.

That is, suppose I maintain a mirror of the branches from
git://repo.or.cz/git.git by pushing regularly to a hosting service
where I do not have shell access.  Since I can't fetch from the target
repository or push from the source, I instead fetch and then push from
a relay, like this:I might push like this:

	git fetch upstream
	git push --force origin refs/remotes/upstream/*:refs/heads/*

Or, in the same spirit, with a detached HEAD:

	git fetch upstream refs/heads/*:refs/heads/*
	git push --force origin :

The --force is to account for "pu" and "next" rewinding.

In this scenario, assuming I have exclusive access to the repository
and the push updates the remote-tracking branches, --lockref and
--force work equally well.  The commands might run once every 6 hours
using a cronjob.

Now suppose my relay has some downtime.  That's fine --- I can still
maintain the mirror by running the same commands on another machine.
But when the old relay comes back up, "push --lockref" will fail and
"pu" and "next" in my mirror are not updated any more.

That is why I said that --force is more appropriate than --lockref
for this application.

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Now suppose my relay has some downtime.  That's fine --- I can still
> maintain the mirror by running the same commands on another machine.
> But when the old relay comes back up, "push --lockref" will fail and
> "pu" and "next" in my mirror are not updated any more.
>
> That is why I said that --force is more appropriate than --lockref
> for this application.

Sure.

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

[Johannes Sixt]

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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 14.07.2013 21:17, schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>> 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".

Default expectation of --lockref

[Junio C Hamano]

> 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
>
> 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
>
>       ... rejected due to stale expectation
>       $ git fetch
>
> You just have updated the lockref base, so if you did, without doing
> anything else, 
>
>       $ git push --lockref
>
> then you will lose the updated contents.

We _might_ be able to use the reflog on refs/remotes/origin/master
to come up with a better default expectation.

We are pushing an updated master, and the commit at the tip of the
branch has a committer timestamp.  refs/remotes/origin/master should
at least have two reflog entries for it at this point.  The latest
one is our latest "git fetch" after the previous lockref push failed
(and we see somebody else updated the master at the origin).  One
before is the one we based our judgement that the rebased result can
replace it.  They both have timestamps for reflog updates.

So we _could_ use refs/remotes/origin/master@{$timestamp} where
the $timestamp is the committer timestamp of the tip of 'master'
we are pushing to replace the 'master' branch at 'origin'.

I do not particularly like this approach, though.  I do not
particulary like the "look at the tracking branch of what we are
updating" in the first place myself, because it requires you to have
such a tracking branch, but now with this, we will also require you
to have a reflog on such a tracking branch, too, which is even
worse.  And it is making it too complex and obscure, even though I
think the semantics would make sense.

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

[Johannes Sixt]

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

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

[Marc Branchaud]

On 13-07-09 04:37 PM, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Am 09.07.2013 21:53, schrieb Junio C Hamano:
>>> +--lockref::
>>> +--lockref=<refname>::
>>> +--lockref=<refname>:<expect>::
>>> ...
>>> +This is meant to make `--force` safer to use.
>>
>> This is a contradiction. "--force" means "I mean it, dude", and not "I
>> mean it sometimes". It would make sense if this sentence were "This is
>> meant to make `+refspec` safer to use."
> 
> No, this *IS* making --force safer by letting you to say in addition
> to --force alone which is blind, add --lockref to defeat it.
> 
> I do not see any good reason to change the samentics of "+refspec"
> for something like this.  "+refspec" and "--force refspec" have
> meant the same thing forever.  If --lockref adds safety to +refspec,
> the same safety should apply to "--force refspec".
> 
>> Do you intend to require users to opt in to safety by saying --lockref
>> until the end of time?
> 
> For normal users this is *NOT* necessary.  I do not know where
> people are getting the idea of making it default.
> 
> Rewinding a branch, needing to --force, is an exceptional case.

Yes, rewinding is exceptional.

However, when a rewind has to happen, I think most users would want to have
this feature most of the time.  I think anyone who rewinds a shared branch
would hate to inadvertently throw away someone else's work.  Rare is the
person who really won't care about that.

So I agree with those who say that this would be nice default behaviour.  I
also don't think we need to make --force different from +refspec, mainly
because if the rewound ref turns out to have moved a simple "git fetch" will
update it and likely allow the next rewind attempt to succeed.  A helpful
error message would make this plain.

I also appreciate the desire to let this stew a while before making it the
default.  However, I don't think that leaving it as an option of push will
give it enough exposure.  I myself want this feature, and I do rewind or
delete a branch every few months or so, but I'm almost certainly going to
forget to use this option the next time the need arises.

But if it was instead/also a configurable option I could just turn on, that
would be awesome.

<bikeshed>
For the option name, how about --match-baseref ?
</bikeshed>

		M.

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

[Michael Haggerty]

On 07/09/2013 09:53 PM, Junio C Hamano wrote:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-push.txt | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index f7dfe48..e7c8bd6 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
>  	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
> +	   [--lockref[=<refname>[:[<expect>]]]]
>  	   [--no-verify] [<repository> [<refspec>...]]
>  
>  DESCRIPTION
> @@ -146,6 +147,31 @@ already exists on the remote side.
>  	to the `master`	branch). See the `<refspec>...` section above
>  	for details.
>  
> +--lockref::
> +--lockref=<refname>::
> +--lockref=<refname>:<expect>::
> +	When updating <refname> at the remote, make sure that the
> +	ref currently points at <expect> (an object name), and else
> +	fail the push, even if `--force` is specified.  If only
> +	<refname> is given, the expected value is taken from the
> +	remote-tracking branch that holds the last-observed value of
> +	the <refname>.  <expect> given as an empty string means the
> +	<refname> should not exist and this push must be creating
> +	it.  If `--lockref` (without any value) is given, make sure
> +	each ref this push is going to update points at the object
> +	our remote-tracking branch for it points at.

I thought that the explanation in your patch 4/7 log message was
clearer.  In particular, I think that documenting the forms separately,
as you did in the log message, makes it unambiguous, whereas for example
the distinction in prose between "If only <refname> is given" and
"<expect> given as an empty string" is easy to miss.

Does "--lockref" only apply to references that need non-ff updates, or
to all references that are being pushed?  This is mostly interesting for
the zero-argument form (especially if a config option is invented to
make this the default), but the question should also be answered for the
other forms.

> +This is meant to make `--force` safer to use.  Imagine that you have
> +to rebase what you have already published.  You will have to
> +`--force` the push to replace the history you originally published
> +with the rebased history.  If somebody else built on top of your
> +original history while you are rebasing, the tip of the branch at

s/are/were/

> +the remote may advance with her commit, and blindly pushing with

s/advance/have advanced/

> +`--force` will lose her work.  By using this option to specify that
> +you expect the history you are updating is what you rebased and want
> +to replace, you can make sure other people's work will not be losed

s/losed/lost/

> +by a forced push. in such a case.

s/push./push/ or s/in such a case.//

> +
>  --repo=<repository>::
>  	This option is only relevant if no <repository> argument is
>  	passed in the invocation. In this case, 'git push' derives the
> 

Another minor point: "git update-ref" allows either 40 "0" or the empty
string to check that the ref doesn't already exist.  For consistency it
might be nice to accept 40 "0" here as well.

I still really like the idea of the feature.

<bikeshed>
The name "--lockref" is OK, but for me it's less a question of
"locking", because as far as the user is concerned the push is an atomic
operation so there is no sense of a "lock" that is being held for a
finite period of time.  For me it is more a question of "checking" or
"verifying".  I see that the word "verify" already has a meaning for
this command, so maybe "--checkref" or "--checkold" or "--checkoldref"?
</bikeshed>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> <bikeshed>
> The name "--lockref" is OK, but for me it's less a question of
> "locking", because as far as the user is concerned the push is an atomic
> operation so there is no sense of a "lock" that is being held for a
> finite period of time.

Yeah, I think this is more like "taking a lease".