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

[Johan Herland <johan@herland.net>]

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