{"thread":{"id":"20915","subject":"git push --confirm ?","startedAt":"2009-09-12T17:51:37Z","lastAt":"2009-09-13T16:59:01Z","messageCount":10,"participants":["Owen Taylor","Jeff King","Daniel Barkalow","Junio C Hamano","Uri Okrent"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"122968","messageId":"1252777897.2974.24.camel@localhost.localdomain","threadId":"20915","inReplyTo":null,"subject":"git push --confirm ?","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-12T17:51:37Z","receivedAt":"2009-09-12T17:51:37Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"People sometimes push things they don't mean to. Depending on the\nworkflow and environment, that can be anywhere between a trivial\nnuisance to an embarrassing and awkward cleanup.\n\nIt would seem handy to me to have a --confirm option to git-push\n(and 'core.confirm-push' to turn it on be default), that would\nhave the following behavior:\n\n * An initial --dry-run pass is done but with more verbosity -\n   for updates of existing references, it would show what commits\n   were being added or removed in a one-line format.\n\n * The user is prompted if they want to proceed\n \n * If the user agrees, then the push is run without --dry-run\n\nI've attached a mockup of this as a porcelain 'git safe-push'.\n\n(Done not using 'git push --porcelain' because I wanted it to work\nwith existing released Git versions. I was hoping to be able to do \n'git config alias.push safe-push', but no facility for interactive \nonly aliases...)\n\nI think this wouldn't be too hard to add to 'git push', though\nI haven't tried to code it. Yes, it's not atomic without protocol\nchanges - I think that's OK:\n\n - If the push isn't being forced intermediate ref updates will\n   be caught as a non-fast-forward in the second pass.\n\n - If the push is being forced, you might overwrite someone else's\n   push anyways even without --confirm.\n\nThoughts on whether this makes sense as an addition?\n\n- Owen\n\n"},{"id":"122977","messageId":"20090912184342.GB20561@coredump.intra.peff.net","threadId":"20915","inReplyTo":"1252777897.2974.24.camel@localhost.localdomain","subject":"Re: git push --confirm ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-12T18:43:42Z","receivedAt":"2009-09-12T18:43:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 12, 2009 at 01:51:37PM -0400, Owen Taylor wrote:\n\n>  * An initial --dry-run pass is done but with more verbosity -\n>    for updates of existing references, it would show what commits\n>    were being added or removed in a one-line format.\n> \n>  * The user is prompted if they want to proceed\n>  \n>  * If the user agrees, then the push is run without --dry-run\n>\n> [...]\n>\n> I think this wouldn't be too hard to add to 'git push', though\n> I haven't tried to code it. Yes, it's not atomic without protocol\n> changes - I think that's OK:\n\nI have never wanted such a feature, so maybe I am a bad person to\ncomment, but I don't see much advantage from a UI standpoint over what\nwe have now. Which is \"git push --dry-run\", check to see if you like it,\nand then re-run without --dry-run. If you just want to see more output\nin the first --dry-run, then that is easy to do with an alternate\nformat.\n\nBut what _would_ be useful is doing it atomically. You can certainly do\nall three of those steps from within one \"git push\" invocation, and I\nthink that is enough without any protocol changes. The protocol already\nsends for each ref a line like:\n\n  <old-sha1> <new-sha1> <ref>\n\nand receive-pack will not proceed with the update unless the <old-sha1>\nmatches what is about to be changed.\n\n>  - If the push isn't being forced intermediate ref updates will\n>    be caught as a non-fast-forward in the second pass.\n> \n>  - If the push is being forced, you might overwrite someone else's\n>    push anyways even without --confirm.\n\nYeah, \"--force\" is not very fine-grained. I wonder if rather than a\ncomplete --confirm you would rather have something iterative like:\n\n  $ git push --interactive\n  Pushing to server:/path/to/repo.git\n    * [new branch]      topic -> topic\n  Push this branch [Yn]?\n      5ad9dce..cfc497a  topic -> topic\n  Push this branch [Yn]?\n      5ad9dce...cfc497a topic -> topic (non-fast forward)\n  Force this branch [yN]?\n\nwhere of course the actual output text and y/n defaults are subject to\ndebate. You could even have a 'v' option at each prompt to visualize the\ndifferences in gitk so you can easily get more information on what you\nmight be overwriting in a non-fast-forward scenario.\n\n-Peff\n"},{"id":"122988","messageId":"1252786266.2974.61.camel@localhost.localdomain","threadId":"20915","inReplyTo":"20090912184342.GB20561@coredump.intra.peff.net","subject":"Re: git push --confirm ?","fromName":"Owen Taylor","fromEmail":"otaylor@redhat.com","sentAt":"2009-09-12T20:11:06Z","receivedAt":"2009-09-12T20:11:06Z","isPatch":false,"sender":{"key":"otaylor@redhat.com","avatar":"https://gravatar.com/avatar/407bd6b1c26601547f8e8dca44e191ddf414516a9536d822400bfab5ddc4ba69?d=mp&s=160"},"body":"On Sat, 2009-09-12 at 14:43 -0400, Jeff King wrote:\n> On Sat, Sep 12, 2009 at 01:51:37PM -0400, Owen Taylor wrote:\n> \n> >  * An initial --dry-run pass is done but with more verbosity -\n> >    for updates of existing references, it would show what commits\n> >    were being added or removed in a one-line format.\n> > \n> >  * The user is prompted if they want to proceed\n> >  \n> >  * If the user agrees, then the push is run without --dry-run\n> >\n> > [...]\n> >\n> > I think this wouldn't be too hard to add to 'git push', though\n> > I haven't tried to code it. Yes, it's not atomic without protocol\n> > changes - I think that's OK:\n> \n> I have never wanted such a feature, so maybe I am a bad person to\n> comment, but I don't see much advantage from a UI standpoint over what\n> we have now. Which is \"git push --dry-run\", check to see if you like it,\n> and then re-run without --dry-run. If you just want to see more output\n> in the first --dry-run, then that is easy to do with an alternate\n> format.\n\nThe main UI advantage is that you can adjust the default with 'git\nconfig' it on and leave it on. The time you screw up is not when you are\nworried that you are going to push the wrong thing. It's when you are\nyou know exactly what 'git push' is going to do and it does something\ndifferent.\n\nSecondarily, I don't really find the output of 'git push --dry-run'\nthat great - it's pretty good for finding out what branches you are\ngoing to push... that you correctly understood the syntax of git push\nand the relationship to your branch configuration, but not so good at\nseeing what's going to be pushed,\n\nIf it shows:\n\n 72b3142..1fa3134 my-topic -> my-topic\n 12a31aa..34f2621 master -> master\n\nThat doesn't necessarily warn you that along with the bug fix you think\nyou are pushing you have a big merge into master sitting there that you\nhaven't finished testing. For updates, showing a commit count and (a\nprobably limited number of) commit subjects would avoid having to\ncut-and-paste the update summary into git log.\n\nAs you say, maybe that's something that just needs to be fixed\nwith a better format for --dry-run. But that doesn't negate the main UI\nadvantage.\n\n> But what _would_ be useful is doing it atomically. You can certainly do\n> all three of those steps from within one \"git push\" invocation, and I\n> think that is enough without any protocol changes. The protocol already\n> sends for each ref a line like:\n> \n>   <old-sha1> <new-sha1> <ref>\n> \n> and receive-pack will not proceed with the update unless the <old-sha1>\n> matches what is about to be changed.\n\nHmm, yeah, I've certainly looked at git-receive-pack(1) before but\nhadn't internalized that --force was client side. Certainly doing it\nwith a single atomic pass is the better way to do it.\n\n(Wouldn't work for rsync and http pushes, right? A simple \"Not\nsupported\" perhaps.)\n\n> >  - If the push isn't being forced intermediate ref updates will\n> >    be caught as a non-fast-forward in the second pass.\n> > \n> >  - If the push is being forced, you might overwrite someone else's\n> >    push anyways even without --confirm.\n> \n> Yeah, \"--force\" is not very fine-grained. I wonder if rather than a\n> complete --confirm you would rather have something iterative like:\n> \n>   $ git push --interactive\n>   Pushing to server:/path/to/repo.git\n>     * [new branch]      topic -> topic\n>   Push this branch [Yn]?\n>       5ad9dce..cfc497a  topic -> topic\n>   Push this branch [Yn]?\n\nHmm, of two minds about this. Doing it as a pick-and-choose\n--interactive does integrate it conceptually with other parts of Git.\nAnd probably is occasionally useful.\n\nBut it makes it considerably less convenient to just config on.\nBecause any time you want to push more than 2-3 refs at once you'll have\nto add --no-interactive.\n\nIt also increases the amount of reading - if I see all the branches at\nonce that are being pushed I can immediately notice that I'm pushing two\nbranches when I thought I was pushing one, without actually having to\nread the branch names.\n\nMy conception of the feature is as a safety harness. That some people\nwill be willing to pay a keystroke or two for that double check that\ntheir mental model matches reality.\n\n      5ad9dce...cfc497a topic -> topic (non-fast forward) \n> Force this branch [yN]?\n\nThis one is a disaster waiting to happen. Even with the reversed\ndefaults you may well have the 'y<return>' habit going. Unless the\nnon-fast-forward looks completely different (Red and Blinky) you\nprobably are going to go right past it.\n\n- Owen\n"},{"id":"122991","messageId":"20090912204905.GA31427@coredump.intra.peff.net","threadId":"20915","inReplyTo":"1252786266.2974.61.camel@localhost.localdomain","subject":"Re: git push --confirm ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-12T20:49:06Z","receivedAt":"2009-09-12T20:49:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[cc'd Daniel; I think this proposal for a \"confirm push\" might interact\nwith your foreign VCS work a bit. I'm not sure anymore what would be the\nright level for inserting this code.]\n\nOn Sat, Sep 12, 2009 at 04:11:06PM -0400, Owen Taylor wrote:\n\n> The main UI advantage is that you can adjust the default with 'git\n> config' it on and leave it on. The time you screw up is not when you are\n> worried that you are going to push the wrong thing. It's when you are\n> you know exactly what 'git push' is going to do and it does something\n> different.\n\nThat makes sense. I would not want to use such a feature, but I see the\nuse case you are talking about (see, I told you I was bad person to\ncomment. ;) ).\n\nIt should be pretty straightforward to implement for the git protocol.\nPushing goes something like:\n\n  1. Get the list of refs from the remote.\n\n  2. Using the desired refspecs (either configured or from the command\n     line), make a list of src/dst pairs of refs to be pushed.\n\n  3. For each ref pair, send the \"<old> <new> <name>\" triple to the\n     remote (or not, if it is already up-to-date, a non-fast-forward,\n     etc).\n\n  4. Send the packed objects.\n\n  5. For each ref pair, print the status in a summary table.\n\nSo you would just want a \"2.5\" where you show something similar to the\nsummary table and get some confirmation (or abort). An iterative \"do you\nwant to push this ref\" strategy would be similar; just mark the refs you\ndo and don't want to push.\n\nThe tricky thing will be handling different transports. Some of that\ncode has been factored out, but I haven't looked at the details. On top\nof that, I think Daniel is working in this area for his support\nof foreign VCS helpers (and other transports like libcurl are getting\npushed out into their own helpers). So he may have a better idea of how\nto go about this sanely.\n\n> haven't finished testing. For updates, showing a commit count and (a\n> probably limited number of) commit subjects would avoid having to\n> cut-and-paste the update summary into git log.\n> \n> As you say, maybe that's something that just needs to be fixed\n> with a better format for --dry-run. But that doesn't negate the main UI\n> advantage.\n\nSure. I just think the two concepts are somewhat orthogonal (though you\nwould probably want to enable them together for your particular\nworkflow). It sounds like you want something like (and obviously you\ncould have a config option to avoid typing --log-changes each time):\n\n  $ git push --dry-run --log-changes\n  To server:/path/to/repo.git\n    5ad9dce..cfc497a  topic -> topic\n      abcd123: commit subject 1\n      cfc497a: commit subject 2\n\nThat can potentially get long, though. I'm not sure if you would want to\nabbreviate it in some way, and if so, how.\n\n> Hmm, yeah, I've certainly looked at git-receive-pack(1) before but\n> hadn't internalized that --force was client side. Certainly doing it\n> with a single atomic pass is the better way to do it.\n\nThere is actually a server-side analogue, which is the\n\"receive.denyNonFastForwards\" config option, and it defaults to \"off\".\nThe \"--force\" option is a client side way of helping you be polite. The\nreceive config option is about actual policy.\n\n> (Wouldn't work for rsync and http pushes, right? A simple \"Not\n> supported\" perhaps.)\n\nRsync, at least, is already non-atomic because there is no way to do\nlocking. So you wouldn't make anything worse by supporting this feature\n(though you do widen the gap for the race condition by waiting for user\ninput). I'm not sure anyone really cares about rsync these days, though.\n\nI believe http-push actually does some kind of DAV locking. So there's\nno reason you this couldn't work for http push.\n\n> Hmm, of two minds about this. Doing it as a pick-and-choose\n> --interactive does integrate it conceptually with other parts of Git.\n> And probably is occasionally useful.\n> \n> But it makes it considerably less convenient to just config on.\n> Because any time you want to push more than 2-3 refs at once you'll have\n> to add --no-interactive.\n> \n> It also increases the amount of reading - if I see all the branches at\n> once that are being pushed I can immediately notice that I'm pushing two\n> branches when I thought I was pushing one, without actually having to\n> read the branch names.\n\nI think it really depends on your workflow, and how many refs you are\ntypically pushing. So yeah, I can see that the iterative asking is not\nreally a replacement for what you are asking for.\n\n-Peff\n"},{"id":"122997","messageId":"alpine.LNX.2.00.0909121739480.28290@iabervon.org","threadId":"20915","inReplyTo":"20090912204905.GA31427@coredump.intra.peff.net","subject":"Re: git push --confirm ?","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-09-12T21:55:15Z","receivedAt":"2009-09-12T21:55:15Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 12 Sep 2009, Jeff King wrote:\n\n> [cc'd Daniel; I think this proposal for a \"confirm push\" might interact\n> with your foreign VCS work a bit. I'm not sure anymore what would be the\n> right level for inserting this code.]\n> \n> On Sat, Sep 12, 2009 at 04:11:06PM -0400, Owen Taylor wrote:\n> \n> > The main UI advantage is that you can adjust the default with 'git\n> > config' it on and leave it on. The time you screw up is not when you are\n> > worried that you are going to push the wrong thing. It's when you are\n> > you know exactly what 'git push' is going to do and it does something\n> > different.\n> \n> That makes sense. I would not want to use such a feature, but I see the\n> use case you are talking about (see, I told you I was bad person to\n> comment. ;) ).\n> \n> It should be pretty straightforward to implement for the git protocol.\n> Pushing goes something like:\n> \n>   1. Get the list of refs from the remote.\n> \n>   2. Using the desired refspecs (either configured or from the command\n>      line), make a list of src/dst pairs of refs to be pushed.\n> \n>   3. For each ref pair, send the \"<old> <new> <name>\" triple to the\n>      remote (or not, if it is already up-to-date, a non-fast-forward,\n>      etc).\n> \n>   4. Send the packed objects.\n> \n>   5. For each ref pair, print the status in a summary table.\n> \n> So you would just want a \"2.5\" where you show something similar to the\n> summary table and get some confirmation (or abort). An iterative \"do you\n> want to push this ref\" strategy would be similar; just mark the refs you\n> do and don't want to push.\n> \n> The tricky thing will be handling different transports. Some of that\n> code has been factored out, but I haven't looked at the details. On top\n> of that, I think Daniel is working in this area for his support\n> of foreign VCS helpers (and other transports like libcurl are getting\n> pushed out into their own helpers). So he may have a better idea of how\n> to go about this sanely.\n\nThe status used to be that each method of pushing implemented \napproximately the rules you give, but implemented it separately. Now \nthere's a common implementation of those rules, with (1) being a method \ncall, (3&4) being a method, and the rest being in transport_push(). \nHowever, rsync and curl have not yet been converted to the new style. When \nthere is support for push with helpers, it will only use the new style \n(because it would be pointlessly annoying to implement the git rules for \nrefspecs in the helpers).\n\nSo it should be easy to put something into transport_push to do a step 2.5 \nconfirmation, and a bit more work (which ought to get done anyway) to make \nit apply to rsync and http URLs.\n\n(Furthermore, currently, http-push is a separate program from the fetch \ncode, which is moving from the main git executable to git-remote-curl; the \nhttp push code should probably actually move to git-remote-curl, so that \nthere is a single external program taking care of all operations on such \nURLs and there is less complexity in how the curl-using code is \nstructured.)\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"123016","messageId":"7vvdjn8ymk.fsf@alter.siamese.dyndns.org","threadId":"20915","inReplyTo":"20090912184342.GB20561@coredump.intra.peff.net","subject":"Re: git push --confirm ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-13T00:41:23Z","receivedAt":"2009-09-13T00:41:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But what _would_ be useful is doing it atomically. You can certainly do\n> all three of those steps from within one \"git push\" invocation, and I\n> think that is enough without any protocol changes. The protocol already\n> sends for each ref a line like:\n>\n>   <old-sha1> <new-sha1> <ref>\n>\n> and receive-pack will not proceed with the update unless the <old-sha1>\n> matches what is about to be changed.\n\nBe careful that using that information and doing things in one session\nwon't give you atomicity in the sense that it may still fail after you\nsaid \"yes that is what I want to push, really\" to the confirmation\nquestion.\n\nIt does save you an extra connection, compared to separate invocations\nwithout and then with --dry-run, so it still is a plus.\n\nI do not think this is an unreasonable option to have.  Just please don't\njustify this change based on atomicity argument, but justify it as a mere\nconvenience feature.\n"},{"id":"123041","messageId":"20090913093324.GB14438@coredump.intra.peff.net","threadId":"20915","inReplyTo":"7vvdjn8ymk.fsf@alter.siamese.dyndns.org","subject":"Re: git push --confirm ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-13T09:33:24Z","receivedAt":"2009-09-13T09:33:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 12, 2009 at 05:41:23PM -0700, Junio C Hamano wrote:\n\n> > But what _would_ be useful is doing it atomically. You can certainly do\n> > all three of those steps from within one \"git push\" invocation, and I\n> > think that is enough without any protocol changes. The protocol already\n> > sends for each ref a line like:\n> >\n> >   <old-sha1> <new-sha1> <ref>\n> >\n> > and receive-pack will not proceed with the update unless the <old-sha1>\n> > matches what is about to be changed.\n> \n> Be careful that using that information and doing things in one session\n> won't give you atomicity in the sense that it may still fail after you\n> said \"yes that is what I want to push, really\" to the confirmation\n> question.\n\nOf course, but that issue exists already. It is just that the window\nbetween receiving the refs and then asking them to be updated is much\nsmaller when there is no human input in the loop (and since we haven't\nactually _shown_ the list to the user, it appears atomic to them).\n\nI think this type of atomicity is fine for this application. The point\nof this is to err on the side of caution. So it is OK to say \"Push\nthis?\" and then after the user has confirmed say \"Oops, somebody pushed\nsomething else while we were waiting for your input. Try again.\" The\nimportant thing is to not say \"Push this?\", have the user confirm that\nwhat they are pushing over is OK, and then end up pushing over something\ndifferent (which is what can happen with separate push invocations).\n\nThe only way to get true atomicity across the confirmation and push\nwould be to take a lock at the beginning of the push session. Which is\ntoo coarse-grained in the first place (it disallows simultaneous update\nof unrelated refs), but would also require protocol updates.\n\n> It does save you an extra connection, compared to separate invocations\n> without and then with --dry-run, so it still is a plus.\n> \n> I do not think this is an unreasonable option to have.  Just please don't\n> justify this change based on atomicity argument, but justify it as a mere\n> convenience feature.\n\nI don't agree. Making sure we use the _same_ <old-sha1> in the\nconfirmation output we show to the user and in the ref update we send to\nthe remote is critical for this to be safe.\n\n-Peff\n"},{"id":"123047","messageId":"7vljkjuo43.fsf@alter.siamese.dyndns.org","threadId":"20915","inReplyTo":"20090913093324.GB14438@coredump.intra.peff.net","subject":"Re: git push --confirm ?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-13T10:37:32Z","receivedAt":"2009-09-13T10:37:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I do not think this is an unreasonable option to have.  Just please don't\n>> justify this change based on atomicity argument, but justify it as a mere\n>> convenience feature.\n>\n> I don't agree. Making sure we use the _same_ <old-sha1> in the\n> confirmation output we show to the user and in the ref update we send to\n> the remote is critical for this to be safe.\n\nOK.\n\nYou may be giving stale info to the user if somebody else is pushing from\nsideways anyway, and the difference between a separate --dry-run and real\npush when that happens is where and how the human waits.\n\nWith a separate --dry-run, the wait happens while the output is examined\noffline.  The user may run \"git log --oneline old...new\" himself before\ndeciding to run the real push.\n\nWith --confirm, the wait happens while the --confirm waits for the human,\nand perhaps the command does \"git log --oneline old...new\" as convenience.\nWhile all this is happening, the TCP connection to the remote end is still\nkept open.  We do not lock anything, but if somebody else pushed from\nsideways, at the end of this session we would notice that, and the push\nwill be aborted.\n\nThis somewhat makes me worry about DoS point of view, but it does make it\nsomewhat safer.\n\nI think the largest practical safety would come from the fact that this\nwould make it convenient (i.e. a single command \"push --confirm\") than\nhaving to run two separate ones with manual inspection in between.  A\nsafety feature that is cumbersome to use won't add much to safety, as that\nis unlikely to be used in the first place.\n"},{"id":"123050","messageId":"20090913105247.GA21750@coredump.intra.peff.net","threadId":"20915","inReplyTo":"7vljkjuo43.fsf@alter.siamese.dyndns.org","subject":"Re: git push --confirm ?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-13T10:52:47Z","receivedAt":"2009-09-13T10:52:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 13, 2009 at 03:37:32AM -0700, Junio C Hamano wrote:\n\n> With --confirm, the wait happens while the --confirm waits for the human,\n> and perhaps the command does \"git log --oneline old...new\" as convenience.\n> While all this is happening, the TCP connection to the remote end is still\n> kept open.  We do not lock anything, but if somebody else pushed from\n> sideways, at the end of this session we would notice that, and the push\n> will be aborted.\n> \n> This somewhat makes me worry about DoS point of view, but it does make it\n> somewhat safer.\n\nI don't see how it makes a DoS any worse. A malicious attacker can\nalways open the TCP connection and let it sit; we are changing only the\nclient code, after all.\n\nIt does increase the possibility of _accidentally_ wasting a TCP\nconnection. I don't know if that is a real-world problem or not. I would\nthink heavily-utilized sites might put a time-out on the connection to\navoid such a DoS in the first place.\n\nHowever, such a timeout is perhaps reason for us to be concerned with\nimplementing this feature with a single session. Will users looking at\nthe commits for confirmation delay enough to hit configured timeouts,\ndropping their connection and forcing them to start again?\n\nOne other way to implement this would be with two TCP connections:\n\n  1. git push --dry-run, recording <old-sha1> for each ref to be pushed.\n     Afterwards, drop the TCP connection.\n\n  2. Get confirmation from the user.\n\n  3. Do the push again, confirming that the <old-sha1> values sent by\n     the server match what we showed the user for confirmation. If not,\n     abort the push.\n\nBesides being a lot more annoying to implement, there is one big\ndownside: in many cases the single TCP connection is a _feature_. If you\nare pushing via ssh and providing a password manually, it is a\nsignificant usability regression to have to input it twice.\n\nAlso, given that ssh is going to be by far the biggest transport for\npushing via the git protocol, I suspect any timeouts are set for\n_before_ the authentication phase (i.e., SSH times you out if you don't\nactually log in). So in that sense it may not be worth worrying about\nhow long we take during the push itself.\n\n> I think the largest practical safety would come from the fact that this\n> would make it convenient (i.e. a single command \"push --confirm\") than\n> having to run two separate ones with manual inspection in between.  A\n> safety feature that is cumbersome to use won't add much to safety, as that\n> is unlikely to be used in the first place.\n\nSure. But that is about packaging it up as a single session for the\nuser. If there is no concern about atomicity, you could do that with a\nsimple wrapper script.\n\n-Peff\n"},{"id":"123067","messageId":"4AAD24D5.1010504@gmail.com","threadId":"20915","inReplyTo":"20090913105247.GA21750@coredump.intra.peff.net","subject":"Re: git push --confirm ?","fromName":"Uri Okrent","fromEmail":"uokrent@gmail.com","sentAt":"2009-09-13T16:59:01Z","receivedAt":"2009-09-13T16:59:01Z","isPatch":false,"sender":{"key":"uokrent@gmail.com","avatar":"https://gravatar.com/avatar/7788ed2d4f1bfefc11082b08b0234fd2d752113623753bda4a032a2ae9687acf?d=mp&s=160"},"body":"Jeff King wrote:\n[snip]\n> Besides being a lot more annoying to implement, there is one big\n> downside: in many cases the single TCP connection is a _feature_. If you\n> are pushing via ssh and providing a password manually, it is a\n> significant usability regression to have to input it twice.\n> \n> Also, given that ssh is going to be by far the biggest transport for\n> pushing via the git protocol, I suspect any timeouts are set for\n> _before_ the authentication phase (i.e., SSH times you out if you don't\n> actually log in). So in that sense it may not be worth worrying about\n> how long we take during the push itself.\n\nThat doesn't seem like a huge hurdle to overcome. Most ssh clients support some\nsort of ServerAliveInterval parameter for just this reason. Sending a keep alive\npacket every 60 seconds or so while waiting for user confirmation doesn't seem\nall that egregious.\n-- \n    Uri\n\nPlease consider the environment before printing this message.\nhttp://www.panda.org/how_you_can_help/\n"}]}