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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2013, 19:48 UTC
Message-ID: <7vvc4re86g.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALKQrge_REZKfds0T-owJOn2BvfLmHpk7yQeSog=yvofE_zKJQ@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

Show 6 quoted lines

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

Show 6 quoted lines

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