git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johan Herland <johan@herland.net>
Date
Jul 3, 2013, 10:00 UTC
Message-ID
<CALKQrge_REZKfds0T-owJOn2BvfLmHpk7yQeSog=yvofE_zKJQ@mail.gmail.com>
In-Reply-To
<7vli5ogh8r.fsf@alter.siamese.dyndns.org>
On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 35 quoted lines
> 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.

Show 50 quoted lines
> 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

Previous: Junio C HamanoNext: Jonathan del Strother
Message 5 of 62 in “[RFD] Making "git push [--force/--delete]" safer?”
  1. Junio C HamanoJul 2, 2013
  2. Johan HerlandJul 2, 2013
  3. Johan HerlandJul 3, 2013
  4. Junio C HamanoJul 3, 2013
  5. Johan HerlandJul 3, 2013
  6. Jonathan del StrotherJul 3, 2013
  7. Johan HerlandJul 3, 2013
  8. Michael HaggertyJul 3, 2013
  9. Johannes SixtJul 3, 2013
  10. Junio C HamanoJul 3, 2013
  11. Johannes SixtJul 4, 2013
  12. Junio C HamanoJul 4, 2013
  13. Junio C HamanoJul 3, 2013
  14. Junio C HamanoJul 3, 2013
  15. Junio C HamanoJul 3, 2013
  16. 0/7 safer "push --force" with compare-and-swapJunio C Hamano, Jul 9, 2013
  17. 1/7 cache.h: move remote/connect API out of itJunio C Hamano, Jul 9, 2013
  18. 2/7 builtin/push.c: use OPT_BOOL, not OPT_BOOLEANJunio C Hamano, Jul 9, 2013
  19. 3/7 push: beginning of compare-and-swap "force/delete safety"Junio C Hamano, Jul 9, 2013
  20. 4/7 remote.c: add command line option parser for --lockrefJunio C Hamano, Jul 9, 2013
  21. John KeepingJul 16, 2013
  22. Junio C HamanoJul 17, 2013
  23. Junio C HamanoJul 17, 2013
  24. 5/7 push --lockref: implement logic to populate old_sha1_expect[]Junio C Hamano, Jul 9, 2013
  25. 6/7 t5533: test "push --lockref"Junio C Hamano, Jul 9, 2013
  26. 7/7 push: document --lockrefJunio C Hamano, Jul 9, 2013
  27. Aaron SchrabJul 9, 2013
  28. Junio C HamanoJul 9, 2013
  29. Johannes SixtJul 9, 2013
  30. Junio C HamanoJul 9, 2013
  31. Johannes SixtJul 9, 2013
  32. Junio C HamanoJul 9, 2013
  33. Junio C HamanoJul 9, 2013
  34. Johannes SixtJul 11, 2013
  35. Junio C HamanoJul 11, 2013
  36. Junio C HamanoJul 11, 2013
  37. Johannes SixtJul 12, 2013
  38. Junio C HamanoJul 12, 2013
  39. Johannes SixtJul 12, 2013
  40. Junio C HamanoJul 12, 2013
  41. Johannes SixtJul 13, 2013
  42. Junio C HamanoJul 13, 2013
  43. Junio C HamanoJul 13, 2013
  44. Johannes SixtJul 13, 2013
  45. John KeepingJul 14, 2013
  46. Johannes SixtJul 13, 2013
  47. Junio C HamanoJul 14, 2013
  48. Johannes SixtJul 14, 2013
  49. Jonathan NiederJul 14, 2013
  50. Jonathan NiederJul 14, 2013
  51. Johannes SixtJul 14, 2013
  52. Jonathan NiederJul 14, 2013
  53. Junio C HamanoJul 15, 2013
  54. Jonathan NiederJul 15, 2013
  55. Junio C HamanoJul 15, 2013
  56. Johannes SixtJul 15, 2013
  57. Junio C HamanoJul 15, 2013
  58. Default expectation of --lockrefJunio C Hamano, Jul 15, 2013
  59. Johannes SixtJul 15, 2013
  60. Marc BranchaudJul 9, 2013
  61. Michael HaggertyJul 9, 2013
  62. Junio C HamanoJul 9, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.