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

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

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

[...]

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

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

On Wed, Jul 3, 2013 at 12:55 AM, Johan Herland <johan@herland.net> wrote:

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

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> writes:

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.

On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

On 3 July 2013 11:00, Johan Herland <johan@herland.net> 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?

On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother <maillist@steelskies.com> wrote:

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

On 07/03/2013 12:11 PM, Johan Herland wrote:

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

Am 7/3/2013 12:50, schrieb Michael Haggerty:

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

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

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.

Am 7/3/2013 21:53, schrieb Junio C Hamano:

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

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

-- Hannes

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

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.

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

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.

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

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.

Johan Herland <johan@herland.net> writes:

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.

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.

See above.

See above.

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.

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

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

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

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

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

On Tue, Jul 09, 2013 at 12:53:27PM -0700, Junio C Hamano wrote:

Should this be

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

?

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

Yes, I think so.

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

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

Thanks for spotting.

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

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

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

At 12:53 -0700 09 Jul 2013, Junio C Hamano <gitster@pobox.com> wrote:

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.

Aaron Schrab <aaron@schrab.com> writes:

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

Am 09.07.2013 21:53, schrieb Junio C Hamano:

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

Johannes Sixt <j6t@kdbg.org> writes:

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

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.

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

Am 09.07.2013 22:37, schrieb Junio C Hamano:

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

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

Johannes Sixt <j6t@kdbg.org> writes:

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.

Junio C Hamano <gitster@pobox.com> 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.

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

Am 10.07.2013 01:08, schrieb Junio C Hamano:

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

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.

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

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

I have a reroll that goes in that direction.

Johannes Sixt <j6t@kdbg.org> writes:

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.

Am 12.07.2013 00:14, schrieb Junio C Hamano:

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

Johannes Sixt <j6t@kdbg.org> writes:

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.

Am 12.07.2013 19:40, schrieb Junio C Hamano:

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

Johannes Sixt <j6t@kdbg.org> writes:

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?

This is traditional --force.

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

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.

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.

Am 12.07.2013 23:19, schrieb Junio C Hamano:

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

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

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

Yes.

Of course.

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.

Not to me, obviously ;)

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

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

... or the + in the refsepc.

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

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:

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

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.

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

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

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.

Am 13.07.2013 22:08, schrieb Junio C Hamano:

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.

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

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.

I think

   git push --lockref=jch +jch +pu

would do.

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

...

Good point.

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.

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

-- Hannes

On Sat, Jul 13, 2013 at 01:08:09PM -0700, Junio C Hamano wrote:

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

Am 13.07.2013 20:14, schrieb Junio C Hamano:

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

Johannes Sixt <j6t@kdbg.org> writes:

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

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.

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

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

Am 14.07.2013 21:17, schrieb Junio C Hamano:

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

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

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

-- Hannes

Johannes Sixt wrote:

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

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

Jonathan

Jonathan Nieder wrote:

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

	*:
	*:*

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

Am 14.07.2013 22:34, schrieb Jonathan Nieder:

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

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

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

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

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

-- Hannes

Johannes Sixt wrote:

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.

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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?

Junio C Hamano wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

Sure.

Am 14.07.2013 22:59, schrieb Johannes Sixt:

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

-- Hannes

Johannes Sixt <j6t@kdbg.org> writes:

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

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.

Am 15.07.2013 05:50, schrieb Junio C Hamano:

Point taken.

OK.

-- Hannes

On 13-07-09 04:37 PM, Junio C Hamano wrote:

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.

On 07/09/2013 09:53 PM, Junio C Hamano wrote:

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.

s/are/were/

s/advance/have advanced/

s/losed/lost/

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

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> writes:

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