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

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

[Matt McCutchen]

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

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

[Stefan Haller]

Matt McCutchen <matt@mattmccutchen.net> wrote:

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 8, 2017 at 4:15 AM, Matt McCutchen <matt@mattmccutchen.net> wrote:
> 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

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.

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

[Jeff King]

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

> Is it correct that you'd essentially want something that works like:
> 
>     git push --force-with-lease=master:master origin master:master

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.

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

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

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

[Jakub Narębski]

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

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

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

-- 
Jakub Narębski

 

[PATCH] push: document & test --force-with-lease with multiple remotes

[Ævar Arnfjörð Bjarmason]

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:
> On Sat, Apr 08, 2017 at 09:35:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Is it correct that you'd essentially want something that works like:
>>
>>     git push --force-with-lease=master:master origin master:master
>
> 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.
>

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

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 1624a35888..2f2e9c078b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -210,6 +210,43 @@ or we do not even have to have such a remote-tracking branch when
 this form is used).  If `<expect>` is the empty string, then the named ref
 must not already exist.
 +
+This option interacts very badly with anything that implicitly runs
+`git fetch` on the remote to be pushed to in the background. The
+protection it offers over `--force` is ensuring that subsequent
+changes your work wasn't based on aren't clobbered, but this is
+trivially defeated if some background process is updating refs in the
+background. We don't have anything except the remote tracking info to
+go by as a heuristic for refs you're expected to have seen & are
+willing to clobber.
++
+If your editor or some other system is running `git fetch` in the
+background for you a way to mitigate this is to simply set up another
+remote:
++
+	git remote add origin-push $(git config remote.origin.url)
+	git fetch origin-push
++
+Now when the background process runs `git fetch origin` the references
+on `origin-push` won't be updated, and thus commands like:
++
+	git push --force-with-lease origin
++
+Will fail unless you manually run `git fetch origin-push`. This method
+is of course entirely defeated by something that runs `git fetch
+--all`, in that case you'd need to either disable it or do something
+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
++
+I.e. create a `base` tag for versions of the upstream code that you've
+seen and are willing to overwrite, then rewrite history, and finally
+force push changes to `master` if the remote version is still at
+`base`, regardless of what your local `remotes/origin/master` has been
+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
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index a2c9e7439f..d38ecee217 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -229,4 +229,33 @@ test_expect_success 'new branch already exists' '
 	)
 '
 
+test_expect_success 'background updates of REMOTE can be mitigated with a non-updated REMOTE-push' '
+	rm -rf src dst &&
+	git init --bare src.bare &&
+	test_when_finished "rm -rf src.bare" &&
+	git clone --no-local src.bare dst &&
+	test_when_finished "rm -rf dst" &&
+	(
+		cd dst &&
+		test_commit G &&
+		git remote add origin-push ../src.bare &&
+		git push origin-push master:master
+	) &&
+	git clone --no-local src.bare dst2 &&
+	test_when_finished "rm -rf dst2" &&
+	(
+		cd dst2 &&
+		test_commit H &&
+		git push
+	) &&
+	(
+		cd dst &&
+		test_commit I &&
+		git fetch origin &&
+		test_must_fail git push --force-with-lease origin-push &&
+		git fetch origin-push &&
+		git push --force-with-lease origin-push
+	)
+'
+
 test_done
-- 
2.11.0

Re: [PATCH] push: document & test --force-with-lease with multiple remotes

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

Hello,

I like the documentation update and test.

On Sat, Apr 08, 2017 at 11:41:00AM +0000, Ævar Arnfjörð Bjarmason wrote:
> [snip]
>
> ++
> +Now when the background process runs `git fetch origin` the references
> +on `origin-push` won't be updated, and thus commands like:
> ++
> +	git push --force-with-lease origin

I think this should be origin-push.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] push: document & test --force-with-lease with multiple remotes

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 9, 2017 at 11:55 AM, Simon Ruderich <simon@ruderich.org> wrote:
> Hello,
>
> I like the documentation update and test.
>
> On Sat, Apr 08, 2017 at 11:41:00AM +0000, Ævar Arnfjörð Bjarmason wrote:
>> [snip]
>>
>> ++
>> +Now when the background process runs `git fetch origin` the references
>> +on `origin-push` won't be updated, and thus commands like:
>> ++
>> +     git push --force-with-lease origin
>
> I think this should be origin-push.

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

> Regards
> Simon
> --
> + privacy is necessary
> + using gnupg http://gnupg.org
> + public key id: 0x92FEFDB7E44C32F9

Re: [PATCH] push: document & test --force-with-lease with multiple remotes

[Junio C Hamano]

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

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

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.

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 1624a35888..2f2e9c078b 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -210,6 +210,43 @@ or we do not even have to have such a remote-tracking branch when
>  this form is used).  If `<expect>` is the empty string, then the named ref
>  must not already exist.
>  +
> +This option interacts very badly with anything that implicitly runs
> +`git fetch` on the remote to be pushed to in the background. The

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

[PATCH v2] push: document & test --force-with-lease with multiple remotes

[Ævar Arnfjörð Bjarmason]

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:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> 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, 

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.

> 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.
>
>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index 1624a35888..2f2e9c078b 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -210,6 +210,43 @@ or we do not even have to have such a remote-tracking branch when
>>  this form is used).  If `<expect>` is the empty string, then the named ref
>>  must not already exist.
>>  +
>> +This option interacts very badly with anything that implicitly runs
>> +`git fetch` on the remote to be pushed to in the background. The
>
> 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".

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

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 1624a35888..0a639664fd 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -217,6 +217,47 @@ 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, 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
+trivially defeated if some background process is updating refs in the
+background. We don't have anything except the remote tracking info to
+go by as a heuristic for refs you're expected to have seen & are
+willing to clobber.
++
+If your editor or some other system is running `git fetch` in the
+background for you a way to mitigate this is to simply set up another
+remote:
++
+	git remote add origin-push $(git config remote.origin.url)
+	git fetch origin-push
++
+Now when the background process runs `git fetch origin` the references
+on `origin-push` won't be updated, and thus commands like:
++
+	git push --force-with-lease origin-push
++
+Will fail unless you manually run `git fetch origin-push`. This method
+is of course entirely defeated by something that runs `git fetch
+--all`, in that case you'd need to either disable it or do something
+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
++
+I.e. create a `base` tag for versions of the upstream code that you've
+seen and are willing to overwrite, then rewrite history, and finally
+force push changes to `master` if the remote version is still at
+`base`, regardless of what your local `remotes/origin/master` has been
+updated to in the background.
 
 -f::
 --force::
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index a2c9e7439f..d38ecee217 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -229,4 +229,33 @@ test_expect_success 'new branch already exists' '
 	)
 '
 
+test_expect_success 'background updates of REMOTE can be mitigated with a non-updated REMOTE-push' '
+	rm -rf src dst &&
+	git init --bare src.bare &&
+	test_when_finished "rm -rf src.bare" &&
+	git clone --no-local src.bare dst &&
+	test_when_finished "rm -rf dst" &&
+	(
+		cd dst &&
+		test_commit G &&
+		git remote add origin-push ../src.bare &&
+		git push origin-push master:master
+	) &&
+	git clone --no-local src.bare dst2 &&
+	test_when_finished "rm -rf dst2" &&
+	(
+		cd dst2 &&
+		test_commit H &&
+		git push
+	) &&
+	(
+		cd dst &&
+		test_commit I &&
+		git fetch origin &&
+		test_must_fail git push --force-with-lease origin-push &&
+		git fetch origin-push &&
+		git push --force-with-lease origin-push
+	)
+'
+
 test_done
-- 
2.11.0

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

[Jacob Keller]

On Sat, Apr 8, 2017 at 2:29 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 08, 2017 at 09:35:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Is it correct that you'd essentially want something that works like:
>>
>>     git push --force-with-lease=master:master origin master:master
>
> 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.
>
>> 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.
>
> 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

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

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

[Jeff King]

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

> > 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
> [...]
> 
> 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?

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.

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

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

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

[Jacob Keller]

On Sat, Apr 8, 2017 at 3:13 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Apr 08, 2017 at 02:54:29PM -0700, Jacob Keller wrote:
>
>> > 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
>> [...]
>>
>> 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?
>
> 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.
>

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

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

Yea, I agree this sort of direction is nicer.

Thanks,
Jake

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

[Stefan Haller]

Jeff King <peff@peff.net> wrote:

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

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.

> And I think that may be converging on the "integrate" refs that Stefan is
> talking about elsewhere (or some isomorphism of it).

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


-- 
Stefan Haller
Ableton
http://www.ableton.com/

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

[Jacob Keller]

On Sun, Apr 9, 2017 at 1:38 AM, Stefan Haller <haller@ableton.com> wrote:
> Jeff King <peff@peff.net> wrote:
>
>> > 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..
>>
>> 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.
>
> 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.

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

>
>> And I think that may be converging on the "integrate" refs that Stefan is
>> talking about elsewhere (or some isomorphism of it).
>
> Does it make things clearer if we don't use the term "integrate", but
> call the config value in my proposal simply "branch.*.lease"?
>

Yes, I think so.

>
> --
> Stefan Haller
> Ableton
> http://www.ableton.com/

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

[Stefan Haller]

Jacob Keller <jacob.keller@gmail.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.

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?

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

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.


-- 
Stefan Haller
Ableton
http://www.ableton.com/

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

[Jacob Keller]

On Sun, Apr 9, 2017 at 4:00 AM, Stefan Haller <haller@ableton.com> wrote:
> Jacob Keller <jacob.keller@gmail.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.
>
> 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?
>

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

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

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

> --
> Stefan Haller
> Ableton
> http://www.ableton.com/

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 10, 2017 at 10:08 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Apr 9, 2017 at 4:00 AM, Stefan Haller <haller@ableton.com> wrote:
>> Jacob Keller <jacob.keller@gmail.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.
>>
>> 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?
>>
>
> I think that's fine. Thinking in terms of only one remote at a time is easier.
>
>>> 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?
>>
>> 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.
>>
>>
>
> 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.

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.

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

[Jacob Keller]

On Mon, Apr 10, 2017 at 2:58 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 10:08 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Sun, Apr 9, 2017 at 4:00 AM, Stefan Haller <haller@ableton.com> wrote:
>>> Jacob Keller <jacob.keller@gmail.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.
>>>
>>> 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?
>>>
>>
>> I think that's fine. Thinking in terms of only one remote at a time is easier.
>>
>>>> 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?
>>>
>>> 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.
>>>
>>>
>>
>> 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.
>
> 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.

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

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

[Junio C Hamano]

On Tue, Apr 11, 2017 at 8:33 AM, Jacob Keller <jacob.keller@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.

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

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

[Stefan Haller]

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

> On Tue, Apr 11, 2017 at 8:33 AM, Jacob Keller <jacob.keller@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.
> 
> Absolutely. That was the _only_ way the feature was originally designed
> to be used sensibly. We really shouldn't have added that "lazy" option [...]
> 
> 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).

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

[PATCH] push: disable lazy --force-with-lease by default

[Junio C Hamano]

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 06898a7498..2f929315a2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2588,6 +2588,11 @@ new default).
 
 --
 
+push.allowLazyForceWithLease::
+	If set to true, allow the `--force-with-lease` option
+	without the expected object name (i.e. expecting the objects
+	at the tip of corresponding remote-tracking branches).
+
 push.followTags::
 	If set to true enable `--follow-tags` option by default.  You
 	may override this configuration at time of push by specifying
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 0a639664fd..1fa01210a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -212,8 +212,9 @@ must not already exist.
 +
 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.
+disabled by default.  Read the note on safety below, and if you think
+you are not affected, enable it with the `push.allowLazyForceWithLease`
+configuration option (cf. linkgit:git-config[1]).
 +
 "--no-force-with-lease" will cancel all the previous --force-with-lease on the
 command line.
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 633e0c3cdd..c008f5b60f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -68,6 +68,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "stale info";
 			break;
 
+		case REF_STATUS_REJECT_LAZY_CAS:
+			res = "error";
+			msg = "lazy force-with-error";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/remote.c b/remote.c
index d87482573d..2d3ee6020f 100644
--- a/remote.c
+++ b/remote.c
@@ -1517,7 +1517,9 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			     int force_update)
 {
 	struct ref *ref;
+	int allow_lazy_cas = 0;
 
+	git_config_get_bool("push.allowLazyForceWithLease", &allow_lazy_cas);
 	for (ref = remote_refs; ref; ref = ref->next) {
 		int force_ref_update = ref->force || force_update;
 		int reject_reason = 0;
@@ -1544,7 +1546,9 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 * branch.
 		 */
 		if (ref->expect_old_sha1) {
-			if (oidcmp(&ref->old_oid, &ref->old_oid_expect))
+			if (!allow_lazy_cas && ref->lazy_cas)
+				reject_reason = REF_STATUS_REJECT_LAZY_CAS;
+			else if (oidcmp(&ref->old_oid, &ref->old_oid_expect))
 				reject_reason = REF_STATUS_REJECT_STALE;
 			else
 				/* If the ref isn't stale then force the update. */
@@ -2341,10 +2345,13 @@ static void apply_cas(struct push_cas_option *cas,
 		if (!refname_match(entry->refname, ref->name))
 			continue;
 		ref->expect_old_sha1 = 1;
-		if (!entry->use_tracking)
+		if (!entry->use_tracking) {
 			hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
-		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
-			oidclr(&ref->old_oid_expect);
+		} else {
+			ref->lazy_cas = 1;
+			if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
+				oidclr(&ref->old_oid_expect);
+		}
 		return;
 	}
 
@@ -2353,6 +2360,7 @@ static void apply_cas(struct push_cas_option *cas,
 		return;
 
 	ref->expect_old_sha1 = 1;
+	ref->lazy_cas = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
 		oidclr(&ref->old_oid_expect);
 }
diff --git a/remote.h b/remote.h
index 6c28cd3e4b..22190f4e91 100644
--- a/remote.h
+++ b/remote.h
@@ -89,6 +89,7 @@ struct ref {
 		force:1,
 		forced_update:1,
 		expect_old_sha1:1,
+		lazy_cas:1,
 		deletion:1;
 
 	enum {
@@ -118,6 +119,7 @@ struct ref {
 		REF_STATUS_REJECT_FETCH_FIRST,
 		REF_STATUS_REJECT_NEEDS_FORCE,
 		REF_STATUS_REJECT_STALE,
+		REF_STATUS_REJECT_LAZY_CAS,
 		REF_STATUS_REJECT_SHALLOW,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/send-pack.c b/send-pack.c
index 11d6f3d983..94f0ad2b14 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -248,6 +248,7 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar
 	case REF_STATUS_REJECT_FETCH_FIRST:
 	case REF_STATUS_REJECT_NEEDS_FORCE:
 	case REF_STATUS_REJECT_STALE:
+	case REF_STATUS_REJECT_LAZY_CAS:
 	case REF_STATUS_REJECT_NODELETE:
 		return CHECK_REF_STATUS_REJECTED;
 	case REF_STATUS_UPTODATE:
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index d38ecee217..0527832ff5 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -17,7 +17,8 @@ test_expect_success setup '
 	# create template repository
 	test_commit A &&
 	test_commit B &&
-	test_commit C
+	test_commit C &&
+	git config --global push.allowLazyForceWithLease true
 '
 
 test_expect_success 'push to update (protected)' '
@@ -78,7 +79,6 @@ test_expect_success 'push to update (protected, tracking, forced)' '
 	(
 		cd dst &&
 		test_commit E &&
-		git ls-remote . refs/remotes/origin/master >expect &&
 		git push --force --force-with-lease=master origin master
 	) &&
 	git ls-remote dst refs/heads/master >expect &&
@@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' '
 	(
 		cd dst &&
 		test_commit D &&
+		git config push.allowLazyForceWithLease false &&
 		git push --force-with-lease=master:master^ origin master
 	) &&
 	git ls-remote dst refs/heads/master >expect &&
@@ -103,6 +104,10 @@ test_expect_success 'push to update (allowed, tracking)' '
 	(
 		cd dst &&
 		test_commit D &&
+		git config push.allowLazyForceWithLease false &&
+		test_must_fail git push --force-with-lease=master origin master 2>err &&
+		grep "lazy force-with-lease" err &&
+		git config --unset push.allowLazyForceWithLease &&
 		git push --force-with-lease=master origin master 2>err &&
 		! grep "forced update" err
 	) &&
@@ -151,6 +156,10 @@ test_expect_success 'push to delete (allowed)' '
 	setup_srcdst_basic &&
 	(
 		cd dst &&
+		git config push.allowLazyForceWithLease false &&
+		test_must_fail git push --force-with-lease=master origin :master 2>err &&
+		grep "lazy force-with-lease" err &&
+		git config --unset push.allowLazyForceWithLease &&
 		git push --force-with-lease=master origin :master 2>err &&
 		grep deleted err
 	) &&
@@ -183,6 +192,9 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
 	(
 		cd dst &&
 		git fetch &&
+		git config push.allowLazyForceWithLease false &&
+		test_must_fail git push --force-with-lease origin master master:naster &&
+		git config --unset push.allowLazyForceWithLease &&
 		git push --force-with-lease origin master master:naster
 	) &&
 	git ls-remote dst refs/heads/master |
@@ -196,6 +208,9 @@ test_expect_success 'new branch covered by force-with-lease' '
 	(
 		cd dst &&
 		git branch branch master &&
+		git config push.allowLazyForceWithLease false &&
+		test_must_fail git push --force-with-lease=branch origin branch &&
+		git config --unset push.allowLazyForceWithLease &&
 		git push --force-with-lease=branch origin branch
 	) &&
 	git ls-remote dst refs/heads/branch >expect &&
diff --git a/transport-helper.c b/transport-helper.c
index 33cff38cc0..e36ea5f578 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -736,6 +736,10 @@ static int push_update_ref_status(struct strbuf *buf,
 			status = REF_STATUS_REJECT_STALE;
 			FREE_AND_NULL(msg);
 		}
+		else if (!strcmp(msg, "lazy force-with-error")) {
+			status = REF_STATUS_REJECT_LAZY_CAS;
+			FREE_AND_NULL(msg);
+		}
 		else if (!strcmp(msg, "forced update")) {
 			forced = 1;
 			FREE_AND_NULL(msg);
@@ -847,6 +851,7 @@ static int push_refs_with_push(struct transport *transport,
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 		case REF_STATUS_REJECT_STALE:
+		case REF_STATUS_REJECT_LAZY_CAS:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
diff --git a/transport.c b/transport.c
index d75ff0514d..25eeb99a36 100644
--- a/transport.c
+++ b/transport.c
@@ -418,6 +418,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 				 "stale info", porcelain, summary_width);
 		break;
+	case REF_STATUS_REJECT_LAZY_CAS:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+				 "lazy force-with-lease", porcelain, summary_width);
+		break;
 	case REF_STATUS_REJECT_SHALLOW:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 				 "new shallow roots not allowed",
@@ -934,6 +938,7 @@ static int run_pre_push_hook(struct transport *transport,
 		if (!r->peer_ref) continue;
 		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
 		if (r->status == REF_STATUS_REJECT_STALE) continue;
+		if (r->status == REF_STATUS_REJECT_LAZY_CAS) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;
 
 		strbuf_reset(&buf);
-- 
2.13.2-765-ge5d8a7f768

Re: [PATCH] push: disable lazy --force-with-lease by default

[Stefan Beller]

On Thu, Jul 6, 2017 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "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(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 06898a7498..2f929315a2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2588,6 +2588,11 @@ new default).
>
>  --
>
> +push.allowLazyForceWithLease::
> +       If set to true, allow the `--force-with-lease` option
> +       without the expected object name (i.e. expecting the objects
> +       at the tip of corresponding remote-tracking branches).
> +
>  push.followTags::
>         If set to true enable `--follow-tags` option by default.  You
>         may override this configuration at time of push by specifying
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 0a639664fd..1fa01210a2 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -212,8 +212,9 @@ must not already exist.
>  +
>  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

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.

>  test_expect_success 'push to update (protected)' '
> @@ -78,7 +79,6 @@ test_expect_success 'push to update (protected, tracking, forced)' '
>         (
>                 cd dst &&
>                 test_commit E &&
> -               git ls-remote . refs/remotes/origin/master >expect &&

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

> @@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' '
>         (
>                 cd dst &&
>                 test_commit D &&
> +               git config push.allowLazyForceWithLease false &&

Here I thought

    test_config -C dst ...

at the beginning might be useful, though ..

> @@ -103,6 +104,10 @@ test_expect_success 'push to update (allowed, tracking)' '
>         (
>                 cd dst &&
>                 test_commit D &&
> +               git config push.allowLazyForceWithLease false &&
> +               test_must_fail git push --force-with-lease=master origin master 2>err &&
> +               grep "lazy force-with-lease" err &&
> +               git config --unset push.allowLazyForceWithLease &&

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

Re: [PATCH] push: disable lazy --force-with-lease by default

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index 0a639664fd..1fa01210a2 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -212,8 +212,9 @@ must not already exist.
>>  +
>>  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
>
> 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.

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

>>  test_expect_success 'push to update (protected)' '
>> @@ -78,7 +79,6 @@ test_expect_success 'push to update (protected, tracking, forced)' '
>>         (
>>                 cd dst &&
>>                 test_commit E &&
>> -               git ls-remote . refs/remotes/origin/master >expect &&
>
> This seems unrelated, so ideally it is a separate commit?

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.

>> @@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' '
>>         (
>>                 cd dst &&
>>                 test_commit D &&
>> +               git config push.allowLazyForceWithLease false &&
>
> Here I thought
>
>     test_config -C dst ...
>
> at the beginning might be useful, though ..

I did not think test_config would work inside a subshell.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Stefan Beller]

On Thu, Jul 6, 2017 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> @@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' '
>>>         (
>>>                 cd dst &&
>>>                 test_commit D &&
>>> +               git config push.allowLazyForceWithLease false &&
>>
>> Here I thought
>>
>>     test_config -C dst ...
>>
>> at the beginning might be useful, though ..
>
> I did not think test_config would work inside a subshell.

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

Re: [PATCH] push: disable lazy --force-with-lease by default

[Stefan Beller]

On Thu, Jul 6, 2017 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>>> index 0a639664fd..1fa01210a2 100644
>>> --- a/Documentation/git-push.txt
>>> +++ b/Documentation/git-push.txt
>>> @@ -212,8 +212,9 @@ must not already exist.
>>>  +
>>>  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
>>
>> 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.
>
> I actually wanted to say it was a failed experiment, but I see your
> point.  Let's leave the "still experimental" label.
>

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.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Stefan Haller]

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

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

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

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: [PATCH] push: disable lazy --force-with-lease by default

[Jeff King]

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

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

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

Re: [PATCH] push: disable lazy --force-with-lease by default

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 07 2017, Stefan Haller jotted:

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

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.

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

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.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Junio C Hamano]

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

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

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

Re: [PATCH] push: disable lazy --force-with-lease by default

[Ævar Arnfjörð Bjarmason]

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

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Which is why I think we should take Francesco's patch (with fixes from
>> feedback), instead of Junio's.
>
> The patch in this discussion is not meant as a replacement for the
> one from Francesco.  It was meant as a companion patch.

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

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

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.

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

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.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Junio C Hamano]

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

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

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.

Re: [PATCH] push: disable lazy --force-with-lease by default

[Ævar Arnfjörð Bjarmason]

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

> "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(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 06898a7498..2f929315a2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2588,6 +2588,11 @@ new default).
>
>  --
>
> +push.allowLazyForceWithLease::
> +	If set to true, allow the `--force-with-lease` option
> +	without the expected object name (i.e. expecting the objects
> +	at the tip of corresponding remote-tracking branches).
> +

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.

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

[Stefan Haller]

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

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Stefan Haller]

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

> On Sun, Apr 9, 2017 at 4:00 AM, Stefan Haller <haller@ableton.com> wrote:
>
> > 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.
> 
> 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.

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


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Jeff King]

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

> Jeff King <peff@peff.net> wrote:
> 
> > > 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..
> > 
> > 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.
> 
> 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.

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

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

[Stefan Haller]

Jeff King <peff@peff.net> wrote:

> On Sun, Apr 09, 2017 at 10:38:42AM +0200, Stefan Haller 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.
> 
> 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).

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.

> I think that would probably just end up with extra
> failures though (so erring on the side of caution). 

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


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Jeff King]

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

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

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

[Stefan Haller]

Jeff King <peff@peff.net> wrote:

> On Tue, Apr 11, 2017 at 02:37:27PM +0200, Stefan Haller 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, that (which is the main way that I merge changes).

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

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Stefan Haller]

Jacob Keller <jacob.keller@gmail.com> 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?

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.

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Jacob Keller]

On Sun, Apr 9, 2017 at 1:38 AM, Stefan Haller <lists@haller-berlin.de> wrote:
> Jacob Keller <jacob.keller@gmail.com> 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?
>
> 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.
>

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.

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

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

Thanks,
Jake

>
> --
> Stefan Haller
> Berlin, Germany
> http://www.haller-berlin.de/

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

[Jacob Keller]

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

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

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

[Jeff King]

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

> On Fri, Apr 7, 2017 at 7:15 PM, Matt McCutchen <matt@mattmccutchen.net> wrote:
> > 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.
> >
> 
> 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?

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

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

[Stefan Haller]

Jeff King <peff@peff.net> wrote:

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Jeff King]

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

> Jeff King <peff@peff.net> wrote:
> 
> > 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.
> 
> 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.

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

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

[Stefan Haller]

Matt McCutchen <matt@mattmccutchen.net> wrote:

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

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

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

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 8, 2017 at 5:03 PM, Stefan Haller <lists@haller-berlin.de> wrote:
> Matt McCutchen <matt@mattmccutchen.net> wrote:
>
>> 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).
>
> 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.

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

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

[Stefan Haller]

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

> On Sat, Apr 8, 2017 at 5:03 PM, Stefan Haller <lists@haller-berlin.de> 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.
> 
> 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.

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
Berlin, Germany
http://www.haller-berlin.de/

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

[Stefan Haller]

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

> 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 "lease" entry. The most
> obvious ones are "git pull" and "git push", or course;

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.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/