* Tools that do an automatic fetch defeat "git push --force-with-lease" @ 2017-04-08 2:15 Matt McCutchen 2017-04-08 7:24 ` Stefan Haller ` (3 more replies) 0 siblings, 4 replies; 49+ messages in thread From: Matt McCutchen @ 2017-04-08 2:15 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen @ 2017-04-08 7:24 ` Stefan Haller 2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 3 siblings, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-08 7:24 UTC (permalink / raw) To: Matt McCutchen, git 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen 2017-04-08 7:24 ` Stefan Haller @ 2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason 2017-04-08 9:29 ` Jeff King 2017-04-08 8:25 ` Jacob Keller 2017-04-08 15:03 ` Stefan Haller 3 siblings, 1 reply; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-08 7:35 UTC (permalink / raw) To: Matt McCutchen; +Cc: git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason @ 2017-04-08 9:29 ` Jeff King 2017-04-08 10:10 ` Jakub Narębski 2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller 0 siblings, 2 replies; 49+ messages in thread From: Jeff King @ 2017-04-08 9:29 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 9:29 ` Jeff King @ 2017-04-08 10:10 ` Jakub Narębski 2017-04-08 11:41 ` [PATCH] push: document & test --force-with-lease with multiple remotes Ævar Arnfjörð Bjarmason 2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller 1 sibling, 1 reply; 49+ messages in thread From: Jakub Narębski @ 2017-04-08 10:10 UTC (permalink / raw) To: Jeff King, Ævar Arnfjörð Bjarmason Cc: Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH] push: document & test --force-with-lease with multiple remotes 2017-04-08 10:10 ` Jakub Narębski @ 2017-04-08 11:41 ` Ævar Arnfjörð Bjarmason 2017-04-09 9:55 ` Simon Ruderich 2017-04-17 3:56 ` Junio C Hamano 0 siblings, 2 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-08 11:41 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Jakub Narębski, Jacob Keller, Matt McCutchen, Æ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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] push: document & test --force-with-lease with multiple remotes 2017-04-08 11:41 ` [PATCH] push: document & test --force-with-lease with multiple remotes Ævar Arnfjörð Bjarmason @ 2017-04-09 9:55 ` Simon Ruderich 2017-04-09 11:40 ` Ævar Arnfjörð Bjarmason 2017-04-17 3:56 ` Junio C Hamano 1 sibling, 1 reply; 49+ messages in thread From: Simon Ruderich @ 2017-04-09 9:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Jeff King, Jakub Narębski, Jacob Keller, Matt McCutchen [-- 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 --] ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: document & test --force-with-lease with multiple remotes 2017-04-09 9:55 ` Simon Ruderich @ 2017-04-09 11:40 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-09 11:40 UTC (permalink / raw) To: Simon Ruderich Cc: Git Mailing List, Junio C Hamano, Jeff King, Jakub Narębski, Jacob Keller, Matt McCutchen 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: document & test --force-with-lease with multiple remotes 2017-04-08 11:41 ` [PATCH] push: document & test --force-with-lease with multiple remotes Ævar Arnfjörð Bjarmason 2017-04-09 9:55 ` Simon Ruderich @ 2017-04-17 3:56 ` Junio C Hamano 2017-04-19 9:22 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 49+ messages in thread From: Junio C Hamano @ 2017-04-17 3:56 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Jakub Narębski, Jacob Keller, Matt McCutchen Æ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". ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2] push: document & test --force-with-lease with multiple remotes 2017-04-17 3:56 ` Junio C Hamano @ 2017-04-19 9:22 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-19 9:22 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Jakub Narębski, Jacob Keller, Matt McCutchen, Simon Ruderich, Æ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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 9:29 ` Jeff King 2017-04-08 10:10 ` Jakub Narębski @ 2017-04-08 21:54 ` Jacob Keller 2017-04-08 22:13 ` Jeff King 2017-04-09 8:38 ` Stefan Haller 1 sibling, 2 replies; 49+ messages in thread From: Jacob Keller @ 2017-04-08 21:54 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller @ 2017-04-08 22:13 ` Jeff King 2017-04-08 22:21 ` Jacob Keller 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:38 ` Stefan Haller 1 sibling, 2 replies; 49+ messages in thread From: Jeff King @ 2017-04-08 22:13 UTC (permalink / raw) To: Jacob Keller Cc: Ævar Arnfjörð Bjarmason, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 22:13 ` Jeff King @ 2017-04-08 22:21 ` Jacob Keller 2017-04-09 8:38 ` Stefan Haller 1 sibling, 0 replies; 49+ messages in thread From: Jacob Keller @ 2017-04-08 22:21 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 22:13 ` Jeff King 2017-04-08 22:21 ` Jacob Keller @ 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:49 ` Jacob Keller 2017-04-10 18:31 ` Jeff King 1 sibling, 2 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-09 8:38 UTC (permalink / raw) To: Jeff King, Jacob Keller Cc: Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-09 8:38 ` Stefan Haller @ 2017-04-09 8:49 ` Jacob Keller 2017-04-09 11:00 ` Stefan Haller 2017-04-10 18:31 ` Jeff King 1 sibling, 1 reply; 49+ messages in thread From: Jacob Keller @ 2017-04-09 8:49 UTC (permalink / raw) To: Stefan Haller Cc: Jeff King, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-09 8:49 ` Jacob Keller @ 2017-04-09 11:00 ` Stefan Haller 2017-04-10 8:08 ` Jacob Keller 0 siblings, 1 reply; 49+ messages in thread From: Stefan Haller @ 2017-04-09 11:00 UTC (permalink / raw) To: Jacob Keller Cc: Jeff King, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-09 11:00 ` Stefan Haller @ 2017-04-10 8:08 ` Jacob Keller 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason 2017-04-11 12:37 ` Stefan Haller 0 siblings, 2 replies; 49+ messages in thread From: Jacob Keller @ 2017-04-10 8:08 UTC (permalink / raw) To: Stefan Haller Cc: Jeff King, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 8:08 ` Jacob Keller @ 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason 2017-04-10 23:33 ` Jacob Keller 2017-04-11 12:37 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Stefan Haller 2017-04-11 12:37 ` Stefan Haller 1 sibling, 2 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-10 9:58 UTC (permalink / raw) To: Jacob Keller Cc: Stefan Haller, Jeff King, Matt McCutchen, git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason @ 2017-04-10 23:33 ` Jacob Keller 2017-04-11 8:51 ` Junio C Hamano 2017-04-11 12:37 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Stefan Haller 1 sibling, 1 reply; 49+ messages in thread From: Jacob Keller @ 2017-04-10 23:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Stefan Haller, Jeff King, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 23:33 ` Jacob Keller @ 2017-04-11 8:51 ` Junio C Hamano 2017-04-12 9:11 ` Stefan Haller 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano 0 siblings, 2 replies; 49+ messages in thread From: Junio C Hamano @ 2017-04-11 8:51 UTC (permalink / raw) To: Jacob Keller Cc: Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, git 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). ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-11 8:51 ` Junio C Hamano @ 2017-04-12 9:11 ` Stefan Haller 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano 1 sibling, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-12 9:11 UTC (permalink / raw) To: Junio C Hamano, Jacob Keller Cc: Ævar Arnfjör? Bjarmason, Jeff King, Matt McCutchen, git 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH] push: disable lazy --force-with-lease by default 2017-04-11 8:51 ` Junio C Hamano 2017-04-12 9:11 ` Stefan Haller @ 2017-07-06 18:56 ` Junio C Hamano 2017-07-06 19:38 ` Stefan Beller ` (2 more replies) 1 sibling, 3 replies; 49+ messages in thread From: Junio C Hamano @ 2017-07-06 18:56 UTC (permalink / raw) To: git Cc: Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli "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 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano @ 2017-07-06 19:38 ` Stefan Beller 2017-07-06 22:39 ` Junio C Hamano 2017-07-07 9:24 ` Stefan Haller 2017-07-07 9:39 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 49+ messages in thread From: Stefan Beller @ 2017-07-06 19:38 UTC (permalink / raw) To: Junio C Hamano Cc: git, Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 19:38 ` Stefan Beller @ 2017-07-06 22:39 ` Junio C Hamano 2017-07-06 22:42 ` Stefan Beller 2017-07-10 22:32 ` Stefan Beller 0 siblings, 2 replies; 49+ messages in thread From: Junio C Hamano @ 2017-07-06 22:39 UTC (permalink / raw) To: Stefan Beller Cc: git, Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 22:39 ` Junio C Hamano @ 2017-07-06 22:42 ` Stefan Beller 2017-07-10 22:32 ` Stefan Beller 1 sibling, 0 replies; 49+ messages in thread From: Stefan Beller @ 2017-07-06 22:42 UTC (permalink / raw) To: Junio C Hamano Cc: git, Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 22:39 ` Junio C Hamano 2017-07-06 22:42 ` Stefan Beller @ 2017-07-10 22:32 ` Stefan Beller 1 sibling, 0 replies; 49+ messages in thread From: Stefan Beller @ 2017-07-10 22:32 UTC (permalink / raw) To: Junio C Hamano Cc: git, Ævar Arnfjörð Bjarmason, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano 2017-07-06 19:38 ` Stefan Beller @ 2017-07-07 9:24 ` Stefan Haller 2017-07-07 9:42 ` Jeff King 2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason 2017-07-07 9:39 ` Ævar Arnfjörð Bjarmason 2 siblings, 2 replies; 49+ messages in thread From: Stefan Haller @ 2017-07-07 9:24 UTC (permalink / raw) To: Junio C Hamano, git Cc: Ævar Arnfjör? Bjarmason, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-07 9:24 ` Stefan Haller @ 2017-07-07 9:42 ` Jeff King 2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 49+ messages in thread From: Jeff King @ 2017-07-07 9:42 UTC (permalink / raw) To: Stefan Haller Cc: Junio C Hamano, git, Ævar Arnfjör? Bjarmason, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-07 9:24 ` Stefan Haller 2017-07-07 9:42 ` Jeff King @ 2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason 2017-07-07 15:15 ` Junio C Hamano 1 sibling, 1 reply; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-07 9:54 UTC (permalink / raw) To: Stefan Haller Cc: Junio C Hamano, git, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason @ 2017-07-07 15:15 ` Junio C Hamano 2017-07-15 10:45 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 49+ messages in thread From: Junio C Hamano @ 2017-07-07 15:15 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Stefan Haller, git, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli Æ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". ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-07 15:15 ` Junio C Hamano @ 2017-07-15 10:45 ` Ævar Arnfjörð Bjarmason 2017-07-17 17:28 ` Junio C Hamano 0 siblings, 1 reply; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-15 10:45 UTC (permalink / raw) To: Junio C Hamano Cc: Stefan Haller, git, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-15 10:45 ` Ævar Arnfjörð Bjarmason @ 2017-07-17 17:28 ` Junio C Hamano 0 siblings, 0 replies; 49+ messages in thread From: Junio C Hamano @ 2017-07-17 17:28 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Stefan Haller, git, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli Æ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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH] push: disable lazy --force-with-lease by default 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano 2017-07-06 19:38 ` Stefan Beller 2017-07-07 9:24 ` Stefan Haller @ 2017-07-07 9:39 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-07 9:39 UTC (permalink / raw) To: Junio C Hamano Cc: git, Stefan Haller, Jeff King, Matt McCutchen, Jacob Keller, Mike Rappazzo, Francesco Mazzoli 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. ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason 2017-04-10 23:33 ` Jacob Keller @ 2017-04-11 12:37 ` Stefan Haller 1 sibling, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-11 12:37 UTC (permalink / raw) To: Ævar Arnfjör? Bjarmason, Jacob Keller Cc: Jeff King, Matt McCutchen, git, Junio C Hamano Æ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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 8:08 ` Jacob Keller 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason @ 2017-04-11 12:37 ` Stefan Haller 1 sibling, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-11 12:37 UTC (permalink / raw) To: Jacob Keller Cc: Jeff King, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:49 ` Jacob Keller @ 2017-04-10 18:31 ` Jeff King 2017-04-11 12:37 ` Stefan Haller 1 sibling, 1 reply; 49+ messages in thread From: Jeff King @ 2017-04-10 18:31 UTC (permalink / raw) To: Stefan Haller Cc: Jacob Keller, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-10 18:31 ` Jeff King @ 2017-04-11 12:37 ` Stefan Haller 2017-04-11 12:50 ` Jeff King 0 siblings, 1 reply; 49+ messages in thread From: Stefan Haller @ 2017-04-11 12:37 UTC (permalink / raw) To: Jeff King Cc: Jacob Keller, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-11 12:37 ` Stefan Haller @ 2017-04-11 12:50 ` Jeff King 2017-04-12 9:11 ` Stefan Haller 0 siblings, 1 reply; 49+ messages in thread From: Jeff King @ 2017-04-11 12:50 UTC (permalink / raw) To: Stefan Haller Cc: Jacob Keller, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-11 12:50 ` Jeff King @ 2017-04-12 9:11 ` Stefan Haller 0 siblings, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-12 9:11 UTC (permalink / raw) To: Jeff King Cc: Jacob Keller, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller 2017-04-08 22:13 ` Jeff King @ 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:46 ` Jacob Keller 1 sibling, 1 reply; 49+ messages in thread From: Stefan Haller @ 2017-04-09 8:38 UTC (permalink / raw) To: Jacob Keller, Jeff King Cc: Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-09 8:38 ` Stefan Haller @ 2017-04-09 8:46 ` Jacob Keller 0 siblings, 0 replies; 49+ messages in thread From: Jacob Keller @ 2017-04-09 8:46 UTC (permalink / raw) To: Stefan Haller Cc: Jeff King, Ævar Arnfjör? Bjarmason, Matt McCutchen, git, Junio C Hamano 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen 2017-04-08 7:24 ` Stefan Haller 2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason @ 2017-04-08 8:25 ` Jacob Keller 2017-04-08 9:31 ` Jeff King 2017-04-08 15:03 ` Stefan Haller 3 siblings, 1 reply; 49+ messages in thread From: Jacob Keller @ 2017-04-08 8:25 UTC (permalink / raw) To: Matt McCutchen; +Cc: git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 8:25 ` Jacob Keller @ 2017-04-08 9:31 ` Jeff King 2017-04-08 15:03 ` Stefan Haller 0 siblings, 1 reply; 49+ messages in thread From: Jeff King @ 2017-04-08 9:31 UTC (permalink / raw) To: Jacob Keller; +Cc: Matt McCutchen, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 9:31 ` Jeff King @ 2017-04-08 15:03 ` Stefan Haller 2017-04-08 22:03 ` Jeff King 0 siblings, 1 reply; 49+ messages in thread From: Stefan Haller @ 2017-04-08 15:03 UTC (permalink / raw) To: Jeff King, Jacob Keller; +Cc: Matt McCutchen, git 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 15:03 ` Stefan Haller @ 2017-04-08 22:03 ` Jeff King 0 siblings, 0 replies; 49+ messages in thread From: Jeff King @ 2017-04-08 22:03 UTC (permalink / raw) To: Stefan Haller; +Cc: Jacob Keller, Matt McCutchen, git 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 ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen ` (2 preceding siblings ...) 2017-04-08 8:25 ` Jacob Keller @ 2017-04-08 15:03 ` Stefan Haller 2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason 2017-04-12 9:11 ` Stefan Haller 3 siblings, 2 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-08 15:03 UTC (permalink / raw) To: Matt McCutchen, git 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 15:03 ` Stefan Haller @ 2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason 2017-04-08 17:28 ` Stefan Haller 2017-04-12 9:11 ` Stefan Haller 1 sibling, 1 reply; 49+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-04-08 16:04 UTC (permalink / raw) To: Stefan Haller; +Cc: Matt McCutchen, git 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... ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason @ 2017-04-08 17:28 ` Stefan Haller 0 siblings, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-08 17:28 UTC (permalink / raw) To: Ævar Arnfjör? Bjarmason; +Cc: Matt McCutchen, git Æ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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: Tools that do an automatic fetch defeat "git push --force-with-lease" 2017-04-08 15:03 ` Stefan Haller 2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason @ 2017-04-12 9:11 ` Stefan Haller 1 sibling, 0 replies; 49+ messages in thread From: Stefan Haller @ 2017-04-12 9:11 UTC (permalink / raw) To: Matt McCutchen, git 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/ ^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2017-07-17 17:29 UTC | newest] Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen 2017-04-08 7:24 ` Stefan Haller 2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason 2017-04-08 9:29 ` Jeff King 2017-04-08 10:10 ` Jakub Narębski 2017-04-08 11:41 ` [PATCH] push: document & test --force-with-lease with multiple remotes Ævar Arnfjörð Bjarmason 2017-04-09 9:55 ` Simon Ruderich 2017-04-09 11:40 ` Ævar Arnfjörð Bjarmason 2017-04-17 3:56 ` Junio C Hamano 2017-04-19 9:22 ` [PATCH v2] " Ævar Arnfjörð Bjarmason 2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller 2017-04-08 22:13 ` Jeff King 2017-04-08 22:21 ` Jacob Keller 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:49 ` Jacob Keller 2017-04-09 11:00 ` Stefan Haller 2017-04-10 8:08 ` Jacob Keller 2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason 2017-04-10 23:33 ` Jacob Keller 2017-04-11 8:51 ` Junio C Hamano 2017-04-12 9:11 ` Stefan Haller 2017-07-06 18:56 ` [PATCH] push: disable lazy --force-with-lease by default Junio C Hamano 2017-07-06 19:38 ` Stefan Beller 2017-07-06 22:39 ` Junio C Hamano 2017-07-06 22:42 ` Stefan Beller 2017-07-10 22:32 ` Stefan Beller 2017-07-07 9:24 ` Stefan Haller 2017-07-07 9:42 ` Jeff King 2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason 2017-07-07 15:15 ` Junio C Hamano 2017-07-15 10:45 ` Ævar Arnfjörð Bjarmason 2017-07-17 17:28 ` Junio C Hamano 2017-07-07 9:39 ` Ævar Arnfjörð Bjarmason 2017-04-11 12:37 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Stefan Haller 2017-04-11 12:37 ` Stefan Haller 2017-04-10 18:31 ` Jeff King 2017-04-11 12:37 ` Stefan Haller 2017-04-11 12:50 ` Jeff King 2017-04-12 9:11 ` Stefan Haller 2017-04-09 8:38 ` Stefan Haller 2017-04-09 8:46 ` Jacob Keller 2017-04-08 8:25 ` Jacob Keller 2017-04-08 9:31 ` Jeff King 2017-04-08 15:03 ` Stefan Haller 2017-04-08 22:03 ` Jeff King 2017-04-08 15:03 ` Stefan Haller 2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason 2017-04-08 17:28 ` Stefan Haller 2017-04-12 9:11 ` Stefan Haller
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).