Tools that do an automatic fetch defeat "git push --force-with-lease"

When I'm rewriting history, "git push --force-with-lease" is a nice safeguard compared to "git push --force", but it still assumes the remote-tracking ref gives the old state the user wants to overwrite. Tools that do an implicit fetch, assuming it to be a safe operation, may break this assumption. In the worst case, Visual Studio Code does an automatic fetch every 3 minutes by default [1], making --force-with-lease pretty much reduce to --force.

For a safer workflow, "git push" would check against a separate "old" ref that isn't updated by "git fetch", but is updated by "git push" the same way the remote-tracking ref is and maybe also by commands that update the local branch to take into account remote changes (I'm not sure what reasonable scenarios there are, if any). Has there been any work on this? I can write a wrapper script for the simple case of "git push" of a single branch to the same branch on a remote, which is pretty much all I use right now, but a native implementation would support all of the command-line usage forms, which would be nice.

Thanks for reading!

[1] https://github.com/Microsoft/vscode/blob/535a3de60023c81d75d0eac22044284f07dbcddf/extensions/git/src/autofetch.ts

Matt

Matt McCutchen <matt@mattmccutchen.net> wrote:

That's a big problem, but even without such tools, I find --force-with-lease without an argument to be pretty limited in usefulness.

I like to type "git fetch" myself regularly, just to see what's new upstream before integrating it; this already breaks it. But even avoiding "git fetch" doesn't help if you are working on more than one branch at a time, because doing "git pull" on one branch will do an implicit "git fetch" on the other.

I like the idea of git maintaining a separate "last integrated" commit for each branch, I think this could solve it in a nice way. I'm probably not qualified enough to work on this myself though, but I'm happy to give input if someone else wants to.

On Sat, Apr 8, 2017 at 4:15 AM, Matt McCutchen <matt@mattmccutchen.net> wrote:

Is it correct that you'd essentially want something that works like:

    git push --force-with-lease=master:master origin master:master

As opposed to the current:

    git push --force-with-lease=master:origin/master origin master:master

Which is how the default:

    git push --force-with-lease

Works now, assuming you're on the master branch with correct tracking info.

I haven't used this feature but I'm surprised it works the way it does, as you point out just having your remote refs updated isn't a strong signal for wanting to clobber whatever that ref points to.

Junio implemented this in v1.8.3.2-736-g28f5d17611 & noted in that commit that the current semantics may not be a sensible default. I think looking at the local ref instead of the remote tracking ref would be a better default.

On Sat, Apr 08, 2017 at 09:35:04AM +0200, Ævar Arnfjörð Bjarmason wrote:

I don't think that would do anything useful. It would reject any push where the remote "master" is not the same as your own master. And of course if they _are_ the same, then the push is a noop.

The point of the --force-with-lease feature is that you would mark a point in time where you started some rewind-y operation (like a rebase), and at the end you would want to make sure nobody had moved the remote ref when you push over it (which needs to be a force because of the rewind).

So the best way to use it is something like:

  git fetch              ;# update 'master' from remote
  git tag base master    ;# mark our base point
  git rebase -i master   ;# rewrite some commits
  git push --force-with-lease=master:base master:master

That final operation will fail if somebody else pushed in the meantime. But obviously this workflow is a pain, because you have to manually mark the start of the unsafe operation with a tag.

If you haven't fetched in the meantime, then origin/master is a good approximation of "base". But if you have fetched, then it is worthless.

It would be nice if we could automatically deduce the real value of base. I don't think we could do it in a foolproof way, but I wonder if we could come close in some common circumstances. For instance, imagine that unsafe operations like rebase would note that "master" has an upstream of "origin/master", and would record a note saying "we took a lease for origin/master at sha1 X".

One trouble with that is that you may perform several unsafe operations. For example, imagine it takes you multiple rebases to achieve your final state:

  git fetch
  git rebase -i master
  git rebase -i master
  git push --force-with-lease=master

and that --force-with-lease now defaults to whatever lease-marker is left by rebase. Which marker should it respect? If the second one, then it's unsafe. But if the first one, then how do we deal with stale markers?

Perhaps it would be enough to reset the markers whenever the ref is pushed. I haven't thought it through well enough to know whether that just hits more corner cases.

-Peff

W dniu 08.04.2017 o 11:29, Jeff King pisze: [...]

I wonder if using a separate remote for pushing (with separate remote- -tracking branches) would be a good solution for this problem...

Document & test for cases where there are two remotes pointing to the same URL, and a background fetch & subsequent `git push --force-with-lease` shouldn't clobber un-updated references we haven't fetched.

Some editors like Microsoft's VSC have a feature to auto-fetch in the background, this bypasses the protections offered by --force-with-lease as noted in the documentation being added here.

See the 'Tools that do an automatic fetch defeat "git push --force-with-lease"' (<1491617750.2149.10.camel@mattmccutchen.net>) git mailing list thread for more details. Jakub Narębski suggested this method of adding another remote to bypass this edge case, document that & add a test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Apr 8, 2017 at 11:29 AM, Jeff King <peff@peff.net> wrote:

Yeah my whole suggestion is obviously dumb & useless. But I liked Jakub's suggestion to work around this, so here's docs & a test for that.

According to my eyeballing of the MS VSC code this should work, i.e. it seems to do a 'fetch' here, not a 'fetch --all': https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/git/node/git.lib.ts#L505

Of course another way is to just disable autofetching: https://github.com/Microsoft/vscode/blob/535a3de60023c81d75d0eac22044284f07dbcddf/extensions/git/src/autofetch.ts#L27

But having two remotes allows you to have your cake & eat it too without all the hassle of tag creation, which I've added to the docs though for completeness.

 Documentation/git-push.txt | 37 +++++++++++++++++++++++++++++++++++++
 t/t5533-push-cas.sh        | 29 +++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

Hello,

I like the documentation update and test.

On Sat, Apr 08, 2017 at 11:41:00AM +0000, Ævar Arnfjörð Bjarmason wrote:

I think this should be origin-push.

Regards Simon

On Sun, Apr 9, 2017 at 11:55 AM, Simon Ruderich <simon@ruderich.org> wrote:

Thanks! Well spotted. Will fix in v2 which I'll hold off on sending for now pending more comments.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

That sounds like an unfortunate mix of two "feature"s that are mutually incompatible. Perhaps those who thought auto-fetch was a good idea didn't think through the implications, and also it is understandable that those who never thought auto-fetch was a good idea would want --force-with-lease to default to the remote-tracking branch.

This description is not accurate. Only those who do not to specify what is expected and instead use the remote-tracking branch are affected (but these random "git fetch" clobbering the remote-tracking branch is sort of known and expected).

I do not think I would mind if these two new lines were added one paragraph above, i.e. where "--force-with-lease=<refname>" form is described. It clearly says "... as the remote-tracking branch we have for them." and that is the best place to say "This option interacts badly".

Document & test for cases where there are two remotes pointing to the same URL, and a background fetch & subsequent `git push --force-with-lease` shouldn't clobber un-updated references we haven't fetched.

Some editors like Microsoft's VSC have a feature to auto-fetch in the background, this bypasses the protections offered by --force-with-lease & --force-with-lease=<refname>, as noted in the documentation being added here.

See the 'Tools that do an automatic fetch defeat "git push --force-with-lease"' (<1491617750.2149.10.camel@mattmccutchen.net>) git mailing list thread for more details. Jakub Narębski suggested this method of adding another remote to bypass this edge case, document that & add a test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Apr 17, 2017 at 5:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

Well, to be fair to those people there's been no negative interaction with the everyday commands people use by aggressively fetching in the background until this feature came along, at least not that I can think of.

There's a lot of advice online and in our own docs to the effect of "pull is fetch + merge|rebase, if you just want to inspect the remote just safely fetch".

Now advice to that effect needs needs to be amended to say "...unless you were thinking of using the shorthand of --force-with-lease, then it'll subtly do the wrong thing".

To argue against the point I was making in <CACBZZX48RanjHsv1UsnxkbxRtqKRGgMcgmtVqQmR84H5j8awqQ@mail.gmail.com> saying basically "at least it sucks less than --force", I'm not so sure anymore. I think that this feature's bit too obscure to break the general "fetch is safe" advice in such a way, and we should probably change how it works by default to make that true again.

I think this addresses your concerns in a better way. I didn't want to add this huge multi-paragraph digression in the middle of where we're briefly explaining the various forms of --force-with-lease above, but I've reworded this so that it's clear that we're only talking about the shorthard forms now, not --force-with-lease=<ref>:<expect>.

A word-diff with version 1:
    
    diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
    index 2f2e9c078b..0a639664fd 100644
    --- a/Documentation/git-push.txt
    +++ b/Documentation/git-push.txt
    @@ -212,5 +212,17 @@ must not already exist.
    +
    [-This-]{+Note that all forms other than `--force-with-lease=<refname>:<expect>`+}
    {+that specifies the expected current value of the ref explicitly are+}
    {+still experimental and their semantics may change as we gain experience+}
    {+with this feature.+}
    {+++}
    {+"--no-force-with-lease" will cancel all the previous --force-with-lease on the+}
    {+command line.+}
    {+++}
    {+A general note on safety: supplying this+} option {+without an expected+}
    {+value, i.e. as `--force-with-lease` or `--force-with-lease=<refname>`+}
    interacts very badly with anything that implicitly runs `git fetch` on
    the remote to be pushed to in the [-background.-]{+background, e.g. `git fetch origin`+}
    {+on your repository in a cronjob.+}
    {+++}
    The protection it offers over `--force` is ensuring that subsequent
    changes your work wasn't based on aren't clobbered, but this is
    @@ -231,3 +243,3 @@ on `origin-push` won't be updated, and thus commands like:
    +
    	git push --force-with-lease [-origin-]{+origin-push+}
    +
    @@ -238,5 +250,5 @@ more tedious like:
    +
    	git fetch              [-;#-]{+#+} update 'master' from remote
    	git tag base master    [-;#-]{+#+} mark our base point
    	git rebase -i master   [-;#-]{+#+} rewrite some commits
    	git push --force-with-lease=master:base master:master
    @@ -248,10 +260,2 @@ force push changes to `master` if the remote version is still at
    updated to in the background.
    [-+-]
    [-Note that all forms other than `--force-with-lease=<refname>:<expect>`-]
    [-that specifies the expected current value of the ref explicitly are-]
    [-still experimental and their semantics may change as we gain experience-]
    [-with this feature.-]
    [-+-]
    [-"--no-force-with-lease" will cancel all the previous --force-with-lease on the-]
    [-command line.-]

 Documentation/git-push.txt | 41 +++++++++++++++++++++++++++++++++++++++++
 t/t5533-push-cas.sh        | 29 +++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)

On Sat, Apr 8, 2017 at 2:29 AM, Jeff King <peff@peff.net> wrote:

What if we added a separate command something like:

git create-lease

which you're expected to run at the start of a rewind-y operation and it creates a tag (or some other ref like a tag but in a different namespace) which is used by force-with-lease?

However, I think using origin/master works fine as long as you don't auto-fetch.

If you're doing it right, you can handle origin/master updates by checking that your rewind-y stuff is correct for the new origin/master RIGHT before you push. The tricky part here is that you have to remember to check after every fetch before you push. This is why I would always create my own tag or lease reference prior to the use.

It might be possible to generate these lease tags prior to operations which modify history and then maybe having a way to list them so you can select which one you meant when you try to use force-with-lease..

Thanks, Jake

On Sat, Apr 08, 2017 at 02:54:29PM -0700, Jacob Keller wrote:

So then you replace that "git tag" command above with "git create-lease". I think it's an incremental improvement because it would probably record which branch you're leasing. But I think the more important issue is that the user needs to remember to take the lease in the first place. A create-lease command doesn't help that.

So yeah, I think that is the more interesting direction. I hadn't considered resolving the multiple-operation ambiguity at push time. But I guess it would be something like "you did a rebase on sha1 X at time T, and then one on Y at time T+N", and you pick which one you're expecting. Or maybe it could even cull old leases that are still ancestors of your push (so old, already-pushed rebases aren't mentioned). I suspect that ends up being equivalent to "clear the leases when you push". And I think that may be converging on the "integrate" refs that Stefan is talking about elsewhere (or some isomorphism of it).

-Peff

On Sat, Apr 8, 2017 at 3:13 PM, Jeff King <peff@peff.net> wrote:

Well, if we don't mind backwards compatibility breaking, we could require that uesrs run create-lease, and refuse to accept anything that isn't a lease ref? That would force the user to have created a lease which should help? But that IS backwards incompatible...

Yea, I agree this sort of direction is nicer.

Thanks, Jake

Jeff King <peff@peff.net> wrote:

I think it's wrong to think about these leases as something that you take before you start a rewindy operation. That's the wrong time to take the lease; by that time, the remote tracking branch may already contain new things that you haven't seen yet, so using that as a lease at that time will overwrite those things later. You have to take the lease at a time where you know that your local branch and the remote tracking branch are up to date with each other, which is after pull and push. And if you do that, there's no multiple-operation ambiguity to deal with at all.

Does it make things clearer if we don't use the term "integrate", but call the config value in my proposal simply "branch.*.lease"?

On Sun, Apr 9, 2017 at 1:38 AM, Stefan Haller <haller@ableton.com> wrote:

Agreed. You "take" a lease whenever you push to the remote or when you pull from the remote and when you pull into the branch. It should store something that tracks both the branch and remote branch together so that you can generalize it to multiple remotes.

It doesn't necessarily track perfectly with a branch that contains extra work such as when doing pull --rebase, but maybe you have an idea about that?

Thanks, Jake

Yes, I think so.

Jacob Keller <jacob.keller@gmail.com> wrote:

I don't see why it has to support multiple remotes (but then I don't have much experience with workflows involving multiple remotes, so I may well be missing something). A local branch can only have one remote tracking branch on one remote, and in my view --force-with-lease without arguments works with that remote tracking branch only. Is this view too simple?

Maybe I wasn't clear enough about that in my proposal, but I propose to always store the commit hash of the remote tracking branch as a new lease after push and pull, not the local branch. This way it works nicely with pull --rebase and a branch that has extra local commits.

On Sun, Apr 9, 2017 at 4:00 AM, Stefan Haller <haller@ableton.com> wrote:

I think that's fine. Thinking in terms of only one remote at a time is easier.

Oh right. The main thing it doesn't give is that this doesn't enforce that your local branch *has* to have at one point prior to the push matched the remote branch that you're overwriting, which would be even more evidence that your changes are what you expect and aren't deleting anything unexpectedly. However, I think that's not strictly necessary to require that since it would also break pull-rebase workflow anyways.

So something like:

For a branch, also store its last known "lease" sha1 value, which updates once every time that we push that branch to the remote tracking branch or any time that we pull into the branch using the remote tracking branch.

This value is what we would default to, and we only need to store the latest one, since that's the last known good value. If the value is wrong then we will error and avoid deleting work, and if it's correct, then we know that the remote branch is correct for this specific push and is safe.

I think this is straight forward and reasonable approach to solve the problem, and makes using force-with-lease much nicer.

Thanks, Jake

On Mon, Apr 10, 2017 at 10:08 AM, Jacob Keller <jacob.keller@gmail.com> wrote:

Does this proposal require that all the things that can update a ref be hooked to maintain these lease values?

In lieu of patches it would be nice for us trying to follow along to at least get some partial list of things that would need to be hooked up, how the command workflow would look like, what things wouldn't work that do now, or work that don't now etc.

E.g. now if I:

     git fetch
     git show origin/master # manually note the sha1
     git checkout -b blah <that-sha1>
     git commit --amend
     git branch --set-upstream-to origin/master
     git push --force-with-lease

It'll work unless origin/master was updated in the meantime, but there's no hint in any log that the branch was created from origin/master to begin with, just from the sha it happened to point to.

I think this is a really important property of git, you don't have to go through some chosen UI that'll record things because you told it "I'd like to checkout stuff from that branch", you can just copy/paste the sha1.

How will this work in this proposed "lease" workflow? Will some lease metadata not get created and I'll need to manually retcon that with some new command?

I'm not just trying to come up with contrived scenarios, I've had to do several force pushes (on repos used by many people) and it's usually due to some giant commit being pushed. At that point the machine I'm investigating the situation on might not be the machine where I do the push from, so often I'm just copy/pasting a sha1 across machines.

To me the *main* feature of --force-with-lease is that it's less shitty than --force, without imposing too much UI overhead. We have to be really careful not to make --force-with-lease so complex by default that people just give u and go back to using --force, which would be worse than either whatever current problems there are with the current --force-with-lease behavior, or anything we replace it with.

On Mon, Apr 10, 2017 at 2:58 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

If you're already copying sha1s around you could use those as the --force-with-lease=branch:<commit>, no?

That's better guarantee than just using --force-with-lease alone.

Thanks, Jake

On Tue, Apr 11, 2017 at 8:33 AM, Jacob Keller <jacob.keller@gmail.com> wrote:

Absolutely. That was the _only_ way the feature was originally designed to be used sensibly. We really shouldn't have added that "lazy" option that assumed that most people used remote tracking branches only when they need to fetch to see what's there, without making sure the assumption is actually true. The "lazy" side of the feature ended up not being something that would work for most people; it instead has become something that only works for those with specific workflow (and a worse part is that those who step outside the assumed workflow won't even get a warning).

Perhaps we should deprecate that "lazy" feature and remove it over time, making sure that everybody feeds the explicit commit object name that was recorded by the user somewhere (e.g. the approach to tag the commit to record the expected remote tip, which Peff illustrated).

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

I agree that this is better than giving the user a false sense of security.

The problem is that manually recording the lease is painful. Like I illustrated, the assumption that you can "simply" do this:

  (... my working copy is in some ramdom state)

  (... now I decide I want to rewrite history)

  $ git tag lease origin/master
  $ git rebase -i
  $ git push --force-with-lease=master:lease

doesn't hold, because by the time you decide to rewrite the history it's already too late.

To solve this, I'd have to record the lease every time I pull or push; this is a lot of work, and easy to forget and error-prone. Hence my suggestion to have git do this automatically. I'd be interested in your thoughts about that proposal, Junio; you didn't say anything about that at all yet.

"git push --force-with-lease=<branch>:<expect>" makes sure that there is no unexpected changes to the branch at the remote while you prepare a rewrite based on the old state of the branch. This feature came with an experimental option that allows :<expect> part to be omitted by using the tip of remote-tracking branch that corresponds to the <branch>.

It turns out that some people use third-party tools that fetch from remote and update the remote-tracking branches behind users' back, defeating the safety relying on the stability of the remote-tracking branches. We have some warning text that was meant to be scary sounding in our documentation, but nevertheless people seem to be bitten. cf. https://public-inbox.org/git/1491617750.2149.10.camel@mattmccutchen.net/ for a recent example.

Let's disable the form that relies on the stability of remote-tracking branches by default, and allow users who _know_ their remote-tracking branches are stable to enable it with a configuration variable.

This problem was predicted from the very beginning; see 28f5d176 (remote.c: add command line option parser for "--force-with-lease", 2013-07-08).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a bit overdue safety fix that we should have done long
   time ago.  If we had this, I do not think it makes it riskier to
   forbid --force and tell people to use --force-with-lease.

 Documentation/config.txt   |  5 +++++
 Documentation/git-push.txt |  5 +++--
 builtin/send-pack.c        |  5 +++++
 remote.c                   | 16 ++++++++++++----
 remote.h                   |  2 ++
 send-pack.c                |  1 +
 t/t5533-push-cas.sh        | 19 +++++++++++++++++--
 transport-helper.c         |  5 +++++
 transport.c                |  5 +++++
 9 files changed, 55 insertions(+), 8 deletions(-)

On Thu, Jul 6, 2017 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:

This indicates that this feature is not 'experimental' any more, but disabled (for safety reasons as described below). This implies we will not change the heuristic for push.allowLazyForceWithLease easily.

Upon reading this documentation and the safety issue, I thought
one could have used the reflog to make it safer as well:
 "I have (manually) inspected the remote tracking branch
  just before lunch, so now I can safely push with lease after lunch"

would translate to e.g. "--force-with-lease-safe--inspection-time=1h", which would make sure that no reflog entries for the given branches exist in the last hour.

Just as food for thought.

This seems unrelated, so ideally it is a separate commit? Fine with me though.

Here I thought

    test_config -C dst ...

at the beginning might be useful, though ..

.. here the -C is not useful, so just using it once above may not be a good idea. For more dense and readable tests (also faster?), have you considered using passing the option via -c instead of setting and unsetting it?

Thanks, Stefan

Stefan Beller <sbeller@google.com> writes:

I actually wanted to say it was a failed experiment, but I see your point. Let's leave the "still experimental" label.

Yes, I looked for other things that may be good to fix while at it, and planned to make a separate clean-up patch if I found enough, but this was the only one.

I did not think test_config would work inside a subshell.

On Thu, Jul 6, 2017 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

It does not, it would need to be done outside the ( ), which is a bit subtle.

On Thu, Jul 6, 2017 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

After rethinking this feature and how to make it safer, we could actually *ask* the user to confirm the sha1:

    # implicit lease:
    $ git push --force-with-lease <remote/refspec>
    # either do an implicit fetch for the refspec first
    # or use the remote tracking branch:
    This would lose HEAD=27956ac767, including
    the following commits on <remote/refspec> :
    27956ac767 Merge branch 'js/rebase-i-final' into pu
    a1b1c5eb04 Merge branch 'sb/hashmap-cleanup' into pu
    ... and 13 more
    Confirm to lose commits by typing yes: yes
    ... normal push

But that may be more effort than this patch originally intended to be, but I would think this makes the lease effective.

Downside is the I/O (Have we any command that is taking user input as such? -p option for reset/add may come to mind) and the unfriendlyness to scripts, but scripting may rely on the non-lazy form of leases.

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

Third-party tools are not the only problem. They may make the problem more likely to occur, but it can also happen without them. (See below.)

I'm wondering if people who claim they know they are safe really do. Elsewhere in the other thread somebody said "I only ever explicitly fetch, so I know I'm safe". Are you sure?

Consider this example:

$ git checkout the-branch-i-am-collaborating-on-with-my-collegue $ git pull # make sure I have their latest work $ git rebase -i ... # do some history rewriting # OK, so as we need to force-push anyway, let's take the opportunity and # rebase onto the latest master: $ git fetch # get latest master $ git rebase origin/master $ git push --force-with-lease

This is a very common thing to do at my workplace. And it's unsafe, because the git fetch may move the remote-tracking branch of the branch I'm working on.

To make this safe, I guess you'd have to replace "git fetch" with something like $ git fetch refs/heads/master:refs/remotes/origin/master

Personally I have never used this form of fetch myself, and I'd be surprised if any of my coworkers even know it exists.

So know you could decide that _any_ fetch is unsafe, and never use it; only use git pull. You are still not safe:

$ git checkout the-branch-i-am-collaborating-on-with-my-collegue $ git pull $ git rebase -i # Now another collegue walks in and asks me to look at the regression # they just introduced on some other branch, so I do $ git checkout that-other-branch $ git pull $ <try to debug their problem> $ <can't find it either, giving up, shrug> # go back to what I was doing: $ git checkout the-branch-i-am-collaborating-on-with-my-collegue $ git push --force-with-lease

Again, the git pull may have moved the remote-tracking branch of the branch that I want to force-push. Again, it could be solved by given an explicit refspec to git pull, but few people ever do this in my experience, and I certainly never want to.

What I'm getting at is that there's a lot of things that you have to remember to not do in order to make --force-with-lease without parameter a useful tool.

On Fri, Jul 07, 2017 at 11:24:15AM +0200, Stefan Haller wrote:

Thanks, these are all really good examples.

I think another one is just:

  $ git fetch
  [time passes]
  $ git checkout branch
  $ git rebase -i
  [oops, I forgot to merge in the latest changes before rewriting]
  $ git push --force-with-lease

That doesn't even require a fetch/pull after you start working. It's simply a mismatch between reality and what the default assumes (that whatever you were working on incorporated the latest work from upstream).

-Peff

On Fri, Jul 07 2017, Stefan Haller jotted:

Both of your examples explicitly fetch. Yes this could be confusing to someone who doesn't understand that "git fetch" doesn't just fetch the current remote branch, but all branches.

Fully agreed, it's confusing, but it's less shitty than --force.

The concern I have with Junio's patch above (but I like Francesco Mazzoli's approach better) is that the safety of the various --force options, from least safe to most safe, is:

 1. --force: You blow away the remote history, no idea what's there, or
    if your local ref mirrors what you just wiped.

 2. --force-with-lease: Even if you have a `git fetch` in the
     background, at least if you wipe a remote ref you have a copy in a
     local reflog to restore it.

 3. --force-with-lease=master:origin/master: More explicit, but still
     subject to the caveat with background fetching.

 4. --force-with-lease=master:<manually copied sha1>: You know exactly
     what you're wiping, and have likely reviewed that exact commit.

Yes, #4 is the safest, #2 & #3 are similar but subject to various caveats with background fetching / users not realizing "git pull" fetches everything etc.

But I think we have to keep our eye on the ball here. Which is to enact a net increase in user safety.

Right now most users who want to force a remote branch just use --force. E.g. Stack Overflow shows >100k results for git + --force, but just 500 for git + --force-with-lease.

You and others are rightly pointing out that --force-with-lease has lots of caveats, but that as an argument-less flag is something we could (with Francesco patch) turn on by default as a --force replacement.

This would leave users better off than they were before, because now when they accidentally wipe something they at least have a local copy if they did the wrong thing.

Moving everyone from #1 to #2 would be a net increase in user safety without more complex UX. Not having #2 would, for a lot of users who'd otherwise be happy to use #2, mean they'll just use #1 (the least safe option!) instead of the more ideal #4.

Which is why I think we should take Francesco's patch (with fixes from feedback), instead of Junio's.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

The patch in this discussion is not meant as a replacement for the one from Francesco. It was meant as a companion patch.

As I view the form of the option that relies on the stability of remote-tracking branches strictly worse than the honest "--force" that loudly advertises itself as dangerous (as opposed to being advertised as a safer option, when it isn't), I consider the change to require users to opt into relying on remote-tracking branches as a prerequisite before we can recommend the form as a safer version of "--force".

On Fri, Jul 07 2017, Junio C. Hamano jotted:

[Re-flowing & re-quoting some of this for clarity]

Okey, so per the table I laid out in 8760f4bmig.fsf@gmail.com:

So with your patch you won't get #2 at all unless you set push.allowLazyForceWithLease=true.

Once Francesco's patch is also applied (or some version thereof) you can then set push.AlwaysForceWithElease to make --force mean --force-with-lease, which is disabled by default.

I think this is really crappy UX design. Now in some future version of Git I need to set a config option *and* type a longer option name to get behavior that's an improvement over --force.

To get the --force-with-lease behavior by default on --force I'll need to set two options, one to alias it, one to allow the option it'll be aliased to.

How is it being advertised as strictly safer without explaining the caveats after my f17d642d3b ("push: document & test --force-with-lease with multiple remotes", 2017-04-19)? I think we now do a very good job of describing the caveats involved, but maybe I missed something.

If you think the documentation is now over-promising can you point out what parts, so I can fix them.

I think we're really losing the forest for the trees here.

I've had to help a lot of people (mainly inexperienced people @ work) after they --force pushed something.

It would be a *huge* improvement if git shipped with a default such that I *knew* that whatever they just wiped away could be found in their local reflog, right now you need to ssh to the git server and dig around there to see what they wiped away.

Most of these people are not running some auto-fetch editor thingy the presence of which is is your motivation for removing the lazy --force-with-lease, and even if they were these people would also benefit from being able to run e.g. `git reflog refs/remotes/origin/topic` to see what they just nuked once someone more experienced shows up and tries to recover from their mistake.

So I wish we could make --force eventually mean --force-with-lease, but it sounds as though you'd like to hide that behind two config options.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

After thinking about this a bit longer, I changed my mind.

I do not think a configuration option that turns "push --force" with anything but "unconditionally force, I mean it" is acceptable; otherwise use of "--force" as the last resort that always does what the option means will be broken, and I do not think you can ask everybody who has scripts to do so to temporarily disable the configuration in them.

What we could do is to improve the "--force-with-lease" that does not say what exact object should be there at the remote for the non-ff push to go through. In another thread there was a discussion to improve the logic to ignore remote-tracking branch and instead use the reflog of the _source_ of the push, and I think that is a lot saner and safer heuristic than what we currently has.

After that happens, we probably could give it an even shorter synonym to the option. But even then, it is unwise to use "--force" as that shorter synonym.

Thanks.

On Thu, Jul 06 2017, Junio C. Hamano jotted:

Just a note on the implementation. Re what I mentioned in 871spxchvm.fsf@gmail.com it would be more consistent to add a --lazy-force-with-lease option, and have a corresponding push.LazyForceWithLease config, which we'd turn off by default.

Then if/when I polish the patch to make CLI options configurable this doesn't have to be handled by a special case, either by code or in the mind of users.

But perhaps adding new CLI options is a bit too much of a hassle to maintain such consistency.

Ævar Arnfjör? Bjarmason <avarab@gmail.com> wrote:

It is true that the proposal relies on people using git push and git pull, not some lower level approximation such as git fetch + git update-ref. Whether that's a valid assumption, I'm not sure yet. It does mean that there are GUI tools that will break the feature; e.g. SmartGit does use fetch + update-ref when you tell it to pull.

In general, I'm not too concerned with my proposal not supporting certain edge-cases such as the one you described later in your mail. I think it's fine if you have to fall back to using --force-with-lease with explicit arguments in these cases. The suggestion is really only to make the common case easier, which (for me) is working with a tracking branch, and using push and pull with no arguments.

Jacob Keller <jacob.keller@gmail.com> wrote:

I don't think it's a necessary requirement to enforce that the local branch has to *match* the remote branch (i.e. point at the exact same commit) prior to beginning a rewindy operation. All that matters is that the local branch is what I called "up to date" with the remote branch in earlier messages; a more precise way of saying this is that the remote branch must either be the same as the local branch, or be reachable from the local branch (in other words, local branch "contains" the remote branch but has more commits on top).

If we take the lease after every push and every successful pull, then this should be guaranteed, as far as I can see. (The "successful" here is a bit problematic, because if the pull fails with conflicts, then we need to wait until the next commit (if it was a merge) or the next "rebase --continue" to be able to tell if it was successful. More on that in a separate mail later.)

On Sun, Apr 09, 2017 at 10:38:42AM +0200, Stefan Haller wrote:

OK. I was assuming that you'd have just integrated before starting such a rebase, but I guess that doesn't have to be the case.

I agree that probably makes the multiple-operation stuff go away, which is nice. It does raise the question of when the integration point happens, and how we handle alternate paths through which commits may land in a local branch (e.g., if both you and upstream do a ff-merge of a particular branch). I think that would probably just end up with extra failures though (so erring on the side of caution). As long as those aren't too common (and this check would kick in only when you're doing --force-with-lease in the first place, so presumably not often), I think it'd be OK.

-Peff

Jeff King <peff@peff.net> wrote:

Are you talking about the case where the user doesn't say git pull, but instead says "git fetch && git merge --ff @{u}"? Just so that I understand the concern.

Yes, and I think this is a very important decision in general.

On Tue, Apr 11, 2017 at 02:37:27PM +0200, Stefan Haller wrote:

Yes, that (which is the main way that I merge changes). But also what happens with:

  git merge origin/other-branch
  git rebase origin/master

I think we only care when origin/master has independently merged other-branch, too. And even though we have taken its commits into account, we would fail (because "both sides did the same thing" is really out of scope for the concept of a lease). So that's OK.

-Peff

Jeff King <peff@peff.net> wrote:

OK; in my proposal I already mentioned that a few other commands besides push and pull may have to update the lease; examples I mentioned were

   git rebase @{u}
   git reset @{u}

and you add "git merge --ff @{u}" to that list now. There might be others that we can add to make the feature work better. (But this could happen incrementally later, as we learn about more use cases.)

I think there's nothing special to consider here; "git rebase origin/master" updates the lease (to origin/master), period. It doesn't matter whether origin/other-branch was merged before, or whether or not it was independently merged to origin/master too, or whether our local branch has cherry-picked all the commits of other-branch instead of merging it, or whatever. In all these cases, the local branch is "up to date" with origin/master after the rebase, so it's ok to update the lease at that point.

Jacob Keller <jacob.keller@gmail.com> wrote:

The problem with this is that it doesn't help to use "git create-lease" right before you start your rewind-y operation, because by that time you may already have fetched. You'd have to use "git create-lease" right after you pull or push. But at the time I pull I don't know yet whether I will later want to rewrite the branch, so to be sure I have to do this every time I pull or push, and then I'd prefer git to do it for me.

I'm not sure I understand what you mean by "checking that your rewind-y stuff is correct for the new origin/master"; does that mean manually inspecting origin/master to convince youself that you are not overwriting something new? If so, I don't think this is acceptable. It is probably ok to work this way if the other party only pushed commits on top; it's reasonable to expect that you will recognize new commits as ones that you haven't seen before. But what if the other party has rewritten the branch and squashed improvements into commits in the middle of it? The head commit will then look the same as before, and the only way to tell whether you are overwriting something new is by comparing the old and new hashes. So then we're back at having to remember what the old hash was.

On Sun, Apr 9, 2017 at 1:38 AM, Stefan Haller <lists@haller-berlin.de> wrote:

No, you don't set the sha1 as the tip of "origin/master" you set it as the tip of "master" after you've performed all the integration and are about to rewind history somehow.

You can do a diff rather than a check of the log.

Thanks, Jake

On Fri, Apr 7, 2017 at 7:15 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:

Isn't the point of force-with-lease to actually record a "commit" id, and not pass it a branch name, but actually the sha1 you intend the remote server to be at? Sure if you happen to pass it a branch or remote name it will interpret it for yuou, but you should be able to do something like

current=$(git rev-parse origin/branch) <verify current is correct and then do your rewind stuff> git push --force-with-lease=$current

and this will work regardless of when if if you fetch in between?

Thanks, Jake

On Sat, Apr 08, 2017 at 01:25:43AM -0700, Jacob Keller wrote:

That's definitely the _best way to do it (modulo using "branch:$current" in the final command). I think Matt's point is just that the default, to use origin/branch, is unsafe. It's convenient when you don't have extra fetches, but that convenience may not be worth the potential surprise.

-Peff

Jeff King <peff@peff.net> wrote:

I don't think "surprise" is the right word here. The point of --force-with-lease is to provide a guarantee, so if you can't trust the guarantee, it makes the feature rather pointless.

On Sat, Apr 08, 2017 at 05:03:06PM +0200, Stefan Haller wrote:

You can trust it under a certain set of conditions. If you break those conditions, it doesn't work. So that's why I said "surprise": most users aren't going to be aware of those conditions.

But I also used the word "unsafe". That, or "dangerous", is certainly accurate. And I don't at all disagree that the situation should be improved.

-Peff

Matt McCutchen <matt@mattmccutchen.net> wrote:

Here's a rough proposal for how I would imagine this to work.

For every local branch that has a remote tracking branch, git maintains a new config entry branch.*.integrated, which records the sha1 of the last upstream commit that was integrated into the local branch.

When --force-with-lease is used without an argument, it will use the values of "branch.*.remote:branch.*.integrated" as an argument. If either doesn't exist, the push fails (this is essential).

Initially the "integrated" entry is created at the same time that branch.*.merge is, i.e. with commits like "git checkout -b name-of-remote-branch", or "git branch --set-upstream-to" and the like, and it will be set to the sha1 that the tip of the remote tracking branch has at that time.

Then, every command that either integrates the remote tracking branch into the local branch, or updates the remote tracking branch to the local branch, will update the value of the "integrated" entry. The most obvious ones are "git pull" and "git push", or course; others that may have to be supported are "git rebase @{u}", "git rebase --onto @{u}", "git reset @{u}", and probably others. The nice thing about these is that initially they don't have to be supported for the feature to still be useful. After using one of them, push --force-with-lease will fail, and the user can then investigate the situation and either use push -f or manually update branch.*.integrated when they have convinced themselves that everything is fine.

I find it essential that --force-with-lease might fail erroneously, but never succeed erroneously, and I think this proposal would guarantee that.

On Sat, Apr 8, 2017 at 5:03 PM, Stefan Haller <lists@haller-berlin.de> wrote:

Can you elaborate on what "integrate" means in this context?

In some ways the entire point of this feature is that you're trying to push over history that you don't want to integrate.

I.e. you're trying to force push your unrelated X over remote history A, but not more recent arbitrary history B based on A which someone may have just pushed.

I'm having a hard time imagining how anything merge/rebase/whatever would place in branch.*.integrated wouldn't just be a roundabout way of recording the info we now record via the tracking branch, or in cases where that's auto-updated for some reason having another tracking branch as my "[PATCH] push: document & test --force-with-lease with multiple remotes" suggests.

But maybe I'm just missing something obvious...

Ævar Arnfjör? Bjarmason <avarab@gmail.com> wrote:

It doesn't matter whether the history you are overwriting is arbitrary, or whether the new history you are pushing is related or unrelated to what you are overwriting. What matters is whether you are aware of what you are overwriting.

I want to record all cases where the local branch is brought up to date with the tracking branch (or vice versa), i.e. mostly push and pull, because I know that after pushing or pulling, my local branch is up to date (in some way) with the tracking branch. If I then rewrite the local branch, I know it is safe to push it *if* the branch on the remote is still in the same state as what I recorded for last push or pull.

If the tracking branch is updated by fetch though, then my local branch is not brought up to date with the remote branch, so I may be overwriting stuff that appeared on the remote without me being aware of it.

It may well be that there are better names then "integrate"; suggestions welcome.

Your suggestion to use a second remote doesn't seem like a satisfactory solution to me, firstly because it's extra work and complexity for the user, and second because it doesn't solve the problem of working with more than one local branch (pulling one branch amounts to a fetch for the other).

Stefan Haller <lists@haller-berlin.de> wrote:

I thought a bit more about what this means, concretely. The problem is that only a *successful* pull must update the lease; an unsuccessful pull must leave it alone. Pull is either fetch+merge or fetch+rebase, and if the merge or rebase fails with conflicts, then we can only tell much later whether the pull was successful; in the case of merge only after the next commit (success) or after git merge --abort (failure), and in the case of rebase after the next rebase --continue (success), or rebase --abort (failure).

To implement this, git pull could set the value "branch.*.pending-lease" after fetch was successful (but leave "branch.*.lease" alone); then git merge and git rebase could look for that value, and if successful, set branch.*.lease to the value of branch.*.pending-lease, and delete pending-lease. If unsuccessful, they'd just delete the pending-lease entry. Other command may also have to delete the pending-lease entry, e.g. git reset.

I do realize that this is a lot of complexity, and it has the potential of missing some cases. However, this complexity is also the reason why I can't build my own wrappers around pull/push to implement the feature outside of git; alias mypull='git pull && git tag -f lease @{u}' just doesn't cut it.