{"thread":{"id":"61103","subject":"[PATCH 0/2] Optionally support push options on up-to-date branches","startedAt":"2024-03-12T21:55:11Z","lastAt":"2024-03-15T20:29:15Z","messageCount":13,"participants":["Christopher Lindee","Junio C Hamano","brian m. carlson","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490513","messageId":"SA1PR14MB4691B63C740F95D7D8A495628D2B2@SA1PR14MB4691.namprd14.prod.outlook.com","threadId":"61103","inReplyTo":null,"subject":"[PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Christopher Lindee","fromEmail":"christopher.lindee@webpros.com","sentAt":"2024-03-12T21:55:08Z","receivedAt":"2024-03-12T21:55:11Z","isPatch":true,"sender":{"key":"christopher.lindee@webpros.com","avatar":null},"body":"Some Git servers can take actions on a branch using push options; to do this,\nthe branch ref must be passed along with the push options.  However, if the\nbranch is up-to-date, then Git submits push options without any refs, so the\nserver is not given a branch to act upon and the push options become no-ops.\n\nThis means a user who does the following\n\n    $ git commit\n    $ git push -o option remote branch  # Performs option\n\nwill have a very different experience from a user who does\n\n    $ git commit\n    $ git push remote branch\n    $ git push -o option remote branch  # No-op\n\nThe latter could easily happen when a user forgets to add an appropriate option\non push, but could also happen intentionally when a user wants to back up their\nchanges to a remote and then later chooses to submit those changes with a push\noption (e.g. one that creates a pull request) to the same remote.\n\nTo work around this issue, the user needs to re-write history between the two\npush commands in some way, either through force pushing a temporary value to\nthe remote or by amending their commit to change the timestamp.  Either can\nimpact users who pull from the server, as well as automated systems triggered\nby a post-receive hook.  With these potential side-effects, this workaround can\nbe a bad option in some setups.\n\nThis changeset proposes to address this issue by adding an option to `push` and\n`send-pack` that, when specified, will send refs where the old-oid and new-oid\nare identical - instead of silently skipping these refs.  The first commit\nintroduces the `--send-up-to-date` option to toggle this behavior, while the\nsecond commit updates the commands to output an `(up-to-date)` notice for each\nbranch with an identical old-oid and new-oid.\n\nNotably, the `--force` option will not send a ref when the remote is up-to-date.\n\nChris Lindee (2):\n  Teach send-pack & push to --send-up-to-date refs\n  Add transport message for up-to-date references\n\n Documentation/git-push.txt      | 8 +++++++-\n Documentation/git-send-pack.txt | 7 +++++++\n builtin/push.c                  | 1 +\n builtin/send-pack.c             | 4 ++++\n send-pack.c                     | 2 +-\n send-pack.h                     | 3 ++-\n transport-helper.c              | 7 ++++++-\n transport.c                     | 3 +++\n transport.h                     | 1 +\n 9 files changed, 32 insertions(+), 4 deletions(-)\n        \n\nbase-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0\n-- \n2.38.1"},{"id":"490593","messageId":"xmqqa5n168nh.fsf@gitster.g","threadId":"61103","inReplyTo":"SA1PR14MB4691B63C740F95D7D8A495628D2B2@SA1PR14MB4691.namprd14.prod.outlook.com","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T22:58:58Z","receivedAt":"2024-03-13T22:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Lindee <christopher.lindee@webpros.com> writes:\n\n> Some Git servers can take actions on a branch using push options; to do this,\n> the branch ref must be passed along with the push options.  However, if the\n> branch is up-to-date, then Git submits push options without any refs, so the\n> server is not given a branch to act upon and the push options become no-ops.\n\nYeah, the send-pack/receive-pack were written in the simpler days\nback when \"pushing the same object again to the ref was an absolute\nno-op\", and push options that breaks the expectation did not exist.\n\nIt makes sense to allow a (seemingly no-op) push to go through with\nan option.\n\nAnd even without custom push option, recording this seemingly no-op\npush as a ref \"update\" does make sense--push certificate records\nwhat object the pusher wanted each target ref to have, and omitting\na ref from the record only because the ref was already pointing at\nthe desired object is losing information.\n\nSo I doubly agree with the reasoning beind this change.\n\n> This changeset proposes to address this issue by adding an option to `push` and\n> `send-pack` that, when specified, will send refs where the old-oid and new-oid\n\n\"where\" -> \"even if\"\n\n> are identical - instead of silently skipping these refs.  The first commit\n> introduces the `--send-up-to-date` option to toggle this behavior, while the\n> second commit updates the commands to output an `(up-to-date)` notice for each\n> branch with an identical old-oid and new-oid.\n>\n> Notably, the `--force` option will not send a ref when the remote is up-to-date.\n\nAnd it makes sense *not* to update `--force` to do the no-op push,\nbecaues you may not want to (accidentally) force push a ref that\ndoes not fast-forward.  As I already said, tying this with use of\nthe \"-o\" option is not sufficient.  So I agree we may want a new\noption to trigger this behaviour.\n\nA radical counter-proposal for the design is to update the client\nside to do this unconditionally, without needing any new option.\nFor an already up-to-date ref, its only contribution to the cost of\n\"git push\" is from its \"Finally, tell the other end!\" instruction,\nwhich is in the order of 100 bytes per ref, and it should not add to\nthe pack generation cost at all [*].\n\n    Side note: But we have to be careful---if the receiving end is\n    totally up-to-date and there is absolutely no ref, I think the\n    current code will not even call pack_objects(), and you'd\n    probably want a similar optimization to avoid the cost to spawn\n    pack_objects().\n\nI do not know if \"send-up-to-date\" is a great name for the option,\nthough.\n\n>\n> Chris Lindee (2):\n>   Teach send-pack & push to --send-up-to-date refs\n>   Add transport message for up-to-date references\n>\n>  Documentation/git-push.txt      | 8 +++++++-\n>  Documentation/git-send-pack.txt | 7 +++++++\n>  builtin/push.c                  | 1 +\n>  builtin/send-pack.c             | 4 ++++\n>  send-pack.c                     | 2 +-\n>  send-pack.h                     | 3 ++-\n>  transport-helper.c              | 7 ++++++-\n>  transport.c                     | 3 +++\n>  transport.h                     | 1 +\n>  9 files changed, 32 insertions(+), 4 deletions(-)\n>         \n>\n> base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0\n"},{"id":"490648","messageId":"ZfOAdVy9_UfTj3zE@tapette.crustytoothpaste.net","threadId":"61103","inReplyTo":"xmqqa5n168nh.fsf@gitster.g","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-14T22:55:49Z","receivedAt":"2024-03-14T22:55:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-13 at 22:58:58, Junio C Hamano wrote:\n> Christopher Lindee <christopher.lindee@webpros.com> writes:\n> > This changeset proposes to address this issue by adding an option to `push` and\n> > `send-pack` that, when specified, will send refs where the old-oid and new-oid\n> \n> \"where\" -> \"even if\"\n> \n> > are identical - instead of silently skipping these refs.  The first commit\n> > introduces the `--send-up-to-date` option to toggle this behavior, while the\n> > second commit updates the commands to output an `(up-to-date)` notice for each\n> > branch with an identical old-oid and new-oid.\n> >\n> > Notably, the `--force` option will not send a ref when the remote is up-to-date.\n> \n> And it makes sense *not* to update `--force` to do the no-op push,\n> becaues you may not want to (accidentally) force push a ref that\n> does not fast-forward.  As I already said, tying this with use of\n> the \"-o\" option is not sufficient.  So I agree we may want a new\n> option to trigger this behaviour.\n> \n> A radical counter-proposal for the design is to update the client\n> side to do this unconditionally, without needing any new option.\n\nI'm not sure that would be a great idea.  Since it's a push, that will\ntrigger authentication, which may prompt the user (e.g., for a password\nor token or for a YubiKey touch with FIDO2 SSH) and which they might be\nable to easily avoid.  As a server operator, I also expect that there\nare people doing lots of needless attempts at pushing in automated\nsystems (because with enough users, there will be at least some who do\nbizarre or inefficient things), and I would prefer to avoid serving\nthose requests if I don't need to.  (For example, for us, reference\nupdates need to go through a distributed commit protocol to update\nmultiple replicas of the repository, and if there's no ref updates, then\nwe cut out multiple services which we don't need to contact.)\n\nNote also that no-op ref updates cannot be simply omitted on the server\nside because we need to verify that the old value for the ref is\ncorrect, or we need to reject the operation as out of date.  While it is\n_unlikely_ that the ref has changed since we fetched it from the server,\nit's not guaranteed, especially if there's an expensive `pre-push` hook.\n\nI do think the proposed change is valuable, though, and I'm in favour of\nit with a suitable option.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490649","messageId":"SA1PR14MB46916648249AFA5D723369188D292@SA1PR14MB4691.namprd14.prod.outlook.com","threadId":"61103","inReplyTo":"xmqqa5n168nh.fsf@gitster.g","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Christopher Lindee","fromEmail":"christopher.lindee@webpros.com","sentAt":"2024-03-14T23:08:08Z","receivedAt":"2024-03-14T23:08:12Z","isPatch":true,"sender":{"key":"christopher.lindee@webpros.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Christopher Lindee <christopher.lindee@webpros.com> writes:\n> \n> > Some Git servers can take actions on a branch using push options; to do this,\n> > the branch ref must be passed along with the push options.  However, if the\n> > branch is up-to-date, then Git submits push options without any refs, so the\n> > server is not given a branch to act upon and the push options become no-ops.\n> \n> Yeah, the send-pack/receive-pack were written in the simpler days\n> back when \"pushing the same object again to the ref was an absolute\n> no-op\", and push options that breaks the expectation did not exist.\n> \n> It makes sense to allow a (seemingly no-op) push to go through with\n> an option.\n> \n> And even without custom push option, recording this seemingly no-op\n> push as a ref \"update\" does make sense--push certificate records\n> what object the pusher wanted each target ref to have, and omitting\n> a ref from the record only because the ref was already pointing at\n> the desired object is losing information.\n\nHmm... does this mean that push certificate records should imply this new\noption?\n\n> So I doubly agree with the reasoning beind this change.\n> \n> > This changeset proposes to address this issue by adding an option to `push` and\n> > `send-pack` that, when specified, will send refs where the old-oid and new-oid\n> \n> \"where\" -> \"even if\"\n\nExcellent clarification!\n\n> > are identical - instead of silently skipping these refs.  The first commit\n> > introduces the `--send-up-to-date` option to toggle this behavior, while the\n> > second commit updates the commands to output an `(up-to-date)` notice for each\n> > branch with an identical old-oid and new-oid.\n> >\n> > Notably, the `--force` option will not send a ref when the remote is up-to-date.\n> \n> And it makes sense *not* to update `--force` to do the no-op push,\n> becaues you may not want to (accidentally) force push a ref that\n> does not fast-forward.  As I already said, tying this with use of\n> the \"-o\" option is not sufficient.  So I agree we may want a new\n> option to trigger this behaviour.\n> \n> A radical counter-proposal for the design is to update the client\n> side to do this unconditionally, without needing any new option.\n> For an already up-to-date ref, its only contribution to the cost of\n> \"git push\" is from its \"Finally, tell the other end!\" instruction,\n> which is in the order of 100 bytes per ref, and it should not add to\n> the pack generation cost at all [*].\n\nAbout 100 bytes per ref can grow when using a ref glob; for example,\n`git push <remote> refs/tags/*:refs/tags/*` would push up ~1200 refs\nto the Git repository, or about 120 KiB across ~80 Ethernet frames.\nThat's not much compared to the amount of data fetched during a push,\nbut it seems like a lot of unnecessary data in most instances and it\nwould add a tiny bit of latency (more on that in a second).\n\n>     Side note: But we have to be careful---if the receiving end is\n>     totally up-to-date and there is absolutely no ref, I think the\n>     current code will not even call pack_objects(), and you'd\n>     probably want a similar optimization to avoid the cost to spawn\n>     pack_objects().\n\nGood point.  This got me thinking about other potential side-effects.\n\nWhat would happen if two actors push to the repository at the same\ntime with a lot of no-op branch changes?  For example,\n\nActors 1 & 2 (either actor could be first):\npush< aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\\0report-status ...\npush< bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/heads/branch\npush< ffffffffffffffffffffffffffffffffffffffff .have\npush< ...\npush< 0000 \n\nActor 1:\npush> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 5555555555555555555555555555555555555555 refs/heads/main\\0 report-status-v2 ...\npush> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb refs/heads/branch\npush> 0000\n\nActor 2:\npush> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\\0 report-status-v2 ...\npush> bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb 4444444444444444444444444444444444444444 refs/heads/branch\npush> 0000\n\nIf repository locking makes `push<` and `push>` a combined atomic operation,\nthen this could never happen.  Otherwise, I suspect Actor 2 would receive an\nerror (or at least a warning) since refs/heads/main is now 555... instead of\naaa....  Moreover, if an error and `--atomic` is specified, then everything\nfails to update.\n\nAnd, as mentioned before, with a large number of refs there will be\nadditional latency, which increases the chance of collisions.\n\n> I do not know if \"send-up-to-date\" is a great name for the option,\n> though.\n\nAgreed; names are difficult.\n\nI considered --always-send-refs, but thought it a bit vague since it\nimplies refs are sometimes not sent, but requires you to look at the\ndocumentation to understand under what conditions refs would not be\nsent.  I also feared there might be other conditions where refs are\nnot sent that I was unaware of.\n\nI think something with \"noop\" makes sense; thing is, which do we use?\n1) --force-nop\n2) --force-noop\n3) --force-no-op\n\nI've seen all 3 ways of writing \"no operation\".  Everyone has their\npreference (e.g. I did 1; Junio suggested 3 and later used 2) and if\nthe one we choose doesn't match up to a user's, typos seem inevitable.\nThough I suppose we already have this issue with color/colour.\n\nI also want to consider the audience.  To me, \"no-op\" feels low-level.\nMeanwhile, \"send-up-to-date\" uses the same nomenclature as the UI\ndisplays and feels more porcelain.  Frankly, this options seems more\nlow-level to me, since users probably want to consider the possible\nside-effects on server-side before using it.  However, I could see\nGitLab telling average users to use the option on their Push Options\n(https://docs.gitlab.com/ee/user/project/push_options.html) docs.\nI'm curious what others think.\n\n> >\n> > Chris Lindee (2):\n> >   Teach send-pack & push to --send-up-to-date refs\n> >   Add transport message for up-to-date references\n> >\n> >  Documentation/git-push.txt      | 8 +++++++-\n> >  Documentation/git-send-pack.txt | 7 +++++++\n> >  builtin/push.c                  | 1 +\n> >  builtin/send-pack.c             | 4 ++++\n> >  send-pack.c                     | 2 +-\n> >  send-pack.h                     | 3 ++-\n> >  transport-helper.c              | 7 ++++++-\n> >  transport.c                     | 3 +++\n> >  transport.h                     | 1 +\n> >  9 files changed, 32 insertions(+), 4 deletions(-)\n> >\n> >\n> > base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0"},{"id":"490650","messageId":"xmqqv85otmsb.fsf@gitster.g","threadId":"61103","inReplyTo":"ZfOAdVy9_UfTj3zE@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T23:29:40Z","receivedAt":"2024-03-14T23:29:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>> A radical counter-proposal for the design is to update the client\n>> side to do this unconditionally, without needing any new option.\n>\n> I'm not sure that would be a great idea.\n\nThanks.  I was looking for a push-back ;-)\n\n> Since it's a push, that will\n> trigger authentication, which may prompt the user (e.g., for a password\n> or token or for a YubiKey touch with FIDO2 SSH) and which they might be\n> able to easily avoid.\n\nDo you mean we first go unauthenticated to find out what commits are\nat the tip of refs at the destination repository, and then switch to\nauthenticated push after we find out there is something that is\nworth pushing?  I somehow thought we need an authenticated access\nonly for the initial ls-refs exchange.\n\n> As a server operator, I also expect that there\n> are people doing lots of needless attempts at pushing in automated\n> systems (because with enough users, there will be at least some who do\n> bizarre or inefficient things), and I would prefer to avoid serving\n> those requests if I don't need to.  (For example, for us, reference\n> updates need to go through a distributed commit protocol to update\n> multiple replicas of the repository, and if there's no ref updates, then\n> we cut out multiple services which we don't need to contact.)\n\nYes, but if you have an extra no-op ref update in the bunch, that\none is excluded from the set of changes to be synchronised across\nreplicas, no?\n\n> Note also that no-op ref updates cannot be simply omitted on the server\n> side because we need to verify that the old value for the ref is\n> correct, or we need to reject the operation as out of date.\n\nYes, but isn't that something the user would rather want to find out\nearlier rather than later?  Your push without no-op update may say\n\"ah, we are up to date\" when we are behind or worse diverged.  If we\ndo the no-op update more often, we'd have more chance to catch such\na situation where the worldview the end-user's repository has is out\nof sync with reality.\n\n> I do think the proposed change is valuable, though, and I'm in favour of\n> it with a suitable option.\n\nIn any case, I am OK with the feature.  I just was wondering if the\nend-user experience may become simpler and easier if we did not have\nto have a command line option.\n\nThanks.\n"},{"id":"490651","messageId":"CAPx1Gvez4Ema9Yi-7Ei8OOkq9t8AXmBYN=yy948k_C+ZBKdXmw@mail.gmail.com","threadId":"61103","inReplyTo":"xmqqv85otmsb.fsf@gitster.g","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-03-15T00:04:25Z","receivedAt":"2024-03-15T00:04:38Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Mar 14, 2024 at 4:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> In any case, I am OK with the feature.  I just was wondering if the\n> end-user experience may become simpler and easier if we did not have\n> to have a command line option.\n\nWould it make sense for the *server* to request the option?\n\nThat is, you run:\n\n    git push server-remote ref\n\nand if the server at server-remote \"wants\" to know about \"no-op\npushes\" because it will do something for this case, it says so.\nIf the server says nothing, the client doesn't bother with the no-op\npushes.\n\n(This could be either per-ref or global, too.)\n\nChris\n"},{"id":"490652","messageId":"PH0PR14MB4702D52DAA9AA81BEDB32D1E8D282@PH0PR14MB4702.namprd14.prod.outlook.com","threadId":"61103","inReplyTo":"ZfOAdVy9_UfTj3zE@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Christopher Lindee","fromEmail":"christopher.lindee@webpros.com","sentAt":"2024-03-15T00:11:07Z","receivedAt":"2024-03-15T00:11:09Z","isPatch":true,"sender":{"key":"christopher.lindee@webpros.com","avatar":null},"body":"brian m. carlson writes:\n\n> On 2024-03-13 at 22:58:58, Junio C Hamano wrote:\n> > Christopher Lindee <christopher.lindee@webpros.com> writes:\n> > > This changeset proposes to address this issue by adding an option to `push` and\n> > > `send-pack` that, when specified, will send refs where the old-oid and new-oid\n> > \n> > \"where\" -> \"even if\"\n> > \n> > > are identical - instead of silently skipping these refs.  The first commit\n> > > introduces the `--send-up-to-date` option to toggle this behavior, while the\n> > > second commit updates the commands to output an `(up-to-date)` notice for each\n> > > branch with an identical old-oid and new-oid.\n> > > \n> > > Notably, the `--force` option will not send a ref when the remote is up-to-date.\n> > \n> > And it makes sense *not* to update `--force` to do the no-op push,\n> > becaues you may not want to (accidentally) force push a ref that\n> > does not fast-forward.  As I already said, tying this with use of\n> > the \"-o\" option is not sufficient.  So I agree we may want a new\n> > option to trigger this behaviour.\n> > \n> > A radical counter-proposal for the design is to update the client\n> > side to do this unconditionally, without needing any new option.\n> \n> I'm not sure that would be a great idea.  Since it's a push, that will\n> trigger authentication, which may prompt the user (e.g., for a password\n> or token or for a YubiKey touch with FIDO2 SSH) and which they might be\n> able to easily avoid.  As a server operator, I also expect that there\n> are people doing lots of needless attempts at pushing in automated\n> systems (because with enough users, there will be at least some who do\n> bizarre or inefficient things), and I would prefer to avoid serving\n> those requests if I don't need to.  (For example, for us, reference\n> updates need to go through a distributed commit protocol to update\n> multiple replicas of the repository, and if there's no ref updates, then\n> we cut out multiple services which we don't need to contact.)\n\nI agree.  I could easily see a nightly cron that backs up all refs to\na server that would then trigger server-side action.\n\nI'm curious about authentication though, when I did a packet trace on\nan up-to-date ref, I noticed it ended with:\n\npush< ...\npush< 0000\npush> 0000\n\nThis suggests that something is sent back to the server.  Does that \nnull entry not require authentication?\n\n> Note also that no-op ref updates cannot be simply omitted on the server\n> side because we need to verify that the old value for the ref is\n> correct, or we need to reject the operation as out of date.  While it is\n> _unlikely_ that the ref has changed since we fetched it from the server,\n> it's not guaranteed, especially if there's an expensive `pre-push` hook.\n\nRegards,\nChris."},{"id":"490653","messageId":"PH0PR14MB4702D85D5D40F910A97B49E98D282@PH0PR14MB4702.namprd14.prod.outlook.com","threadId":"61103","inReplyTo":"xmqqv85otmsb.fsf@gitster.g","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Christopher Lindee","fromEmail":"christopher.lindee@webpros.com","sentAt":"2024-03-15T00:19:57Z","receivedAt":"2024-03-15T00:19:59Z","isPatch":true,"sender":{"key":"christopher.lindee@webpros.com","avatar":null},"body":"Junio C Hamano writes:\n\n> In any case, I am OK with the feature.  I just was wondering if the\n> end-user experience may become simpler and easier if we did not have\n> to have a command line option.\n\nI think we should have the command line option, if only for backwards \ncompatibility.  The question on my mind is: do we default to on or off?\n\nRegards,\nChris."},{"id":"490655","messageId":"ZfOYxnRHwyFdU4Y3@tapette.crustytoothpaste.net","threadId":"61103","inReplyTo":"xmqqv85otmsb.fsf@gitster.g","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-15T00:39:34Z","receivedAt":"2024-03-15T00:39:37Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-14 at 23:29:40, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> >> A radical counter-proposal for the design is to update the client\n> >> side to do this unconditionally, without needing any new option.\n> >\n> > I'm not sure that would be a great idea.\n> \n> Thanks.  I was looking for a push-back ;-)\n> \n> > Since it's a push, that will\n> > trigger authentication, which may prompt the user (e.g., for a password\n> > or token or for a YubiKey touch with FIDO2 SSH) and which they might be\n> > able to easily avoid.\n> \n> Do you mean we first go unauthenticated to find out what commits are\n> at the tip of refs at the destination repository, and then switch to\n> authenticated push after we find out there is something that is\n> worth pushing?  I somehow thought we need an authenticated access\n> only for the initial ls-refs exchange.\n\nIt depends on the server and the protocol.  Protocol v0 allows\nunauthenticated ref exchange, and then the push itself can be\nauthenticated.  It is customary to require authentication on both\npieces, but not mandatory.\n\n> > As a server operator, I also expect that there\n> > are people doing lots of needless attempts at pushing in automated\n> > systems (because with enough users, there will be at least some who do\n> > bizarre or inefficient things), and I would prefer to avoid serving\n> > those requests if I don't need to.  (For example, for us, reference\n> > updates need to go through a distributed commit protocol to update\n> > multiple replicas of the repository, and if there's no ref updates, then\n> > we cut out multiple services which we don't need to contact.)\n> \n> Yes, but if you have an extra no-op ref update in the bunch, that\n> one is excluded from the set of changes to be synchronised across\n> replicas, no?\n\nAs I said, we can't do that, because we have to verify that the old\nvalue is correct.  Even if we did perform that optimization, once we\nknow we have a write, we have to contact every replica (in case the user\ndoes indeed send us a pack, which is allowed by the protocol, even if\nbizarre), whereas with a ref request or a read, we can satisfy that with\na single read replica.  I'm sure I can satisfy at least one to two\norders of magnitude more no-op push attempts with the current\noptimization than I could if we didn't have it.  It might even be to the\npoint of three or more orders of magnitude.  (My guess is that reftable\nwill improve this even further, but I haven't yet measured.)\n\nI'm sure this is also true for most other implementations; serving\nreads, like ls-refs, are almost always substantially cheaper than writes\nand most server implementations are highly optimized for serving large\nnumbers of reads due to CI farms and such.\n\n> > Note also that no-op ref updates cannot be simply omitted on the server\n> > side because we need to verify that the old value for the ref is\n> > correct, or we need to reject the operation as out of date.\n> \n> Yes, but isn't that something the user would rather want to find out\n> earlier rather than later?  Your push without no-op update may say\n> \"ah, we are up to date\" when we are behind or worse diverged.  If we\n> do the no-op update more often, we'd have more chance to catch such\n> a situation where the worldview the end-user's repository has is out\n> of sync with reality.\n\nWell, we were up to date at the time the ls-refs operation completed, or\nthe push wouldn't have the same old and new OIDs.  The client has to\ndeclare the old ref value that it got from the ls-refs output in order\nfor the operation to succeed, since the server-side Git won't update the\nreferences if the old value doesn't match.  (This is how the client\nknows whether a push is a fast-forward or not: it computes it, and if\nthe remote side has an unknown value, then it's not.)  Other than\nupdating the push options, there's no functional benefit to making a\nsecond request, since the ls-refs output already told us where the\nbranch is.  That's why the current optimization is functionally correct.\n\nIn any event, there are alternate implementations of the Git protocol\nwhich did not implement our current optimization and did, for purposes\nof mirroring, make excessive numbers of no-op requests, and we did have\nto ask for that to be fixed.  I assure you, the no-op behaviour is\nbetter for users because it's faster and substantially more likely to\nsucceed (because it is effectively a read-only ls-refs request), whereas\na new ref update is less efficient, slower, and substantially more\nlikely to fail (due to functional limitations on write throughput).\n\nI also expect there will be poorly written `pre-push` and `pre-receive`\nhooks which will not like getting a no-op update and not handle it\ngracefully.  I'm pretty sure Git LFS will handle this just fine, but\nlots of people have various shell scripts that probably will not.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490656","messageId":"ZfOc9hrayymnJNPc@tapette.crustytoothpaste.net","threadId":"61103","inReplyTo":"CAPx1Gvez4Ema9Yi-7Ei8OOkq9t8AXmBYN=yy948k_C+ZBKdXmw@mail.gmail.com","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-15T00:57:26Z","receivedAt":"2024-03-15T00:57:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-15 at 00:04:25, Chris Torek wrote:\n> On Thu, Mar 14, 2024 at 4:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > In any case, I am OK with the feature.  I just was wondering if the\n> > end-user experience may become simpler and easier if we did not have\n> > to have a command line option.\n> \n> Would it make sense for the *server* to request the option?\n> \n> That is, you run:\n> \n>     git push server-remote ref\n> \n> and if the server at server-remote \"wants\" to know about \"no-op\n> pushes\" because it will do something for this case, it says so.\n> If the server says nothing, the client doesn't bother with the no-op\n> pushes.\n> \n> (This could be either per-ref or global, too.)\n\nThis is an interesting idea.  I don't think this can be per-ref because\nat the capabilities stage, we don't actually know what the user wants to\nsend, if anything.  v0 capabilities are too limited in size to declare\nper-ref options, although I suppose v2 could, in theory, support that.\n\nI do worry, though, that even if the server may be interested in\nadditional push options or push certificates, as Chris's series\nindicates, it may not generally care about no-op ref updates, and thus,\nit may not be granular enough to avoid the kinds of performance problems\nthat I've mentioned elsewhere in the thread.\n\nFor example, I know that GitLab uses push options (and GitHub has\nsupport for them as well in some cases) and thus the proposed feature\nwould be useful there, but I suspect we'd still want regular no-op\npushes (without push options or push certificates) to not be sent for\nperformance reasons.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490681","messageId":"SA1PR14MB46911007FDFE3FFADB4FB3A68D282@SA1PR14MB4691.namprd14.prod.outlook.com","threadId":"61103","inReplyTo":"ZfOYxnRHwyFdU4Y3@tapette.crustytoothpaste.net","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Christopher Lindee","fromEmail":"christopher.lindee@webpros.com","sentAt":"2024-03-15T08:58:35Z","receivedAt":"2024-03-15T08:58:37Z","isPatch":true,"sender":{"key":"christopher.lindee@webpros.com","avatar":null},"body":"brian m. carlson <sandals@crustytoothpaste.net> writes:\n\n> On 2024-03-14 at 23:29:40, Junio C Hamano wrote:\n> > \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> > > As a server operator, I also expect that there\n> > > are people doing lots of needless attempts at pushing in automated\n> > > systems (because with enough users, there will be at least some who do\n> > > bizarre or inefficient things), and I would prefer to avoid serving\n> > > those requests if I don't need to.  (For example, for us, reference\n> > > updates need to go through a distributed commit protocol to update\n> > > multiple replicas of the repository, and if there's no ref updates, then\n> > > we cut out multiple services which we don't need to contact.)\n> >\n> > Yes, but if you have an extra no-op ref update in the bunch, that\n> > one is excluded from the set of changes to be synchronised across\n> > replicas, no?\n> \n> \n> As I said, we can't do that, because we have to verify that the old\n> value is correct.  Even if we did perform that optimization, once we\n> know we have a write, we have to contact every replica (in case the user\n> does indeed send us a pack, which is allowed by the protocol, even if\n> bizarre), whereas with a ref request or a read, we can satisfy that with\n> a single read replica.  I'm sure I can satisfy at least one to two\n> orders of magnitude more no-op push attempts with the current\n> optimization than I could if we didn't have it.  It might even be to the\n> point of three or more orders of magnitude.  (My guess is that reftable\n> will improve this even further, but I haven't yet measured.)\n> \n> \n> I'm sure this is also true for most other implementations; serving\n> reads, like ls-refs, are almost always substantially cheaper than writes\n> and most server implementations are highly optimized for serving large\n> numbers of reads due to CI farms and such.\n> \n> \n> > > Note also that no-op ref updates cannot be simply omitted on the server\n> > > side because we need to verify that the old value for the ref is\n> > > correct, or we need to reject the operation as out of date.\n> >\n> > Yes, but isn't that something the user would rather want to find out\n> > earlier rather than later?  Your push without no-op update may say\n> > \"ah, we are up to date\" when we are behind or worse diverged.  If we\n> > do the no-op update more often, we'd have more chance to catch such\n> > a situation where the worldview the end-user's repository has is out\n> > of sync with reality.\n> \n> \n> Well, we were up to date at the time the ls-refs operation completed, or\n> the push wouldn't have the same old and new OIDs.  The client has to\n> declare the old ref value that it got from the ls-refs output in order\n> for the operation to succeed, since the server-side Git won't update the\n> references if the old value doesn't match.  (This is how the client\n> knows whether a push is a fast-forward or not: it computes it, and if\n> the remote side has an unknown value, then it's not.)  Other than\n> updating the push options, there's no functional benefit to making a\n> second request, since the ls-refs output already told us where the\n> branch is.  That's why the current optimization is functionally correct.\n> \n> \n> In any event, there are alternate implementations of the Git protocol\n> which did not implement our current optimization and did, for purposes\n> of mirroring, make excessive numbers of no-op requests, and we did have\n> to ask for that to be fixed.  I assure you, the no-op behaviour is\n> better for users because it's faster and substantially more likely to\n> succeed (because it is effectively a read-only ls-refs request), whereas\n> a new ref update is less efficient, slower, and substantially more\n> likely to fail (due to functional limitations on write throughput).\n> \n> \n> I also expect there will be poorly written `pre-push` and `pre-receive`\n> hooks which will not like getting a no-op update and not handle it\n> gracefully.  I'm pretty sure Git LFS will handle this just fine, but\n> lots of people have various shell scripts that probably will not.\n\nIs this a potential avenue for DoS?\n\nClearly, I'm not the first to send no-op ref updates, so this change\ndoesn't create the problem, but it could allow non-coders to exploit\nexisting problems much more easily.\n\nIf this could be a problem, we can take the time now to solve it.  A\nsimple limit on the number of no-op refs received before the server\nterminates with REF_STATUS_REMOTE_REJECT should be sufficient, right?\nIf we bake that expectation into the official Git implementation, then\nother implementations will be empowered to take similar steps.\n\nFor at least one service[1], GitLab only supports Push Options on, at\nmost, 10 refs.  If other server operators have similar limitations,\nthen this is an easy bud to nip.\n\nRegards,\nChris.\n\n[1] https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/services/merge_requests/push_options_handler_service.rb?ref_type=heads#L5"},{"id":"490694","messageId":"xmqqil1ntsas.fsf@gitster.g","threadId":"61103","inReplyTo":"PH0PR14MB4702D85D5D40F910A97B49E98D282@PH0PR14MB4702.namprd14.prod.outlook.com","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T15:42:51Z","receivedAt":"2024-03-15T15:45:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Lindee <christopher.lindee@webpros.com> writes:\n\n> Junio C Hamano writes:\n>\n>> In any case, I am OK with the feature.  I just was wondering if the\n>> end-user experience may become simpler and easier if we did not have\n>> to have a command line option.\n>\n> I think we should have the command line option, if only for backwards \n> compatibility.  The question on my mind is: do we default to on or off?\n\nIt seems like the concensus is we want this as an opt-in feature.\nDefaults off, with an explicit command line option.\n\nThanks.\n"},{"id":"490734","messageId":"ZfSvmOWrsT2dFnG5@tapette.crustytoothpaste.net","threadId":"61103","inReplyTo":"SA1PR14MB46911007FDFE3FFADB4FB3A68D282@SA1PR14MB4691.namprd14.prod.outlook.com","subject":"Re: [PATCH 0/2] Optionally support push options on up-to-date branches","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-15T20:29:12Z","receivedAt":"2024-03-15T20:29:15Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-15 at 08:58:35, Christopher Lindee wrote:\n> Is this a potential avenue for DoS?\n\nNo, it's not.  In our implementation, there is a functional limit on ref\nupdates and if you exceed it, the operation fails.  We also have rate\nlimiting that estimates future cost based on the cost of previous\nrequests and delays or fails requests which are projected to exceed a\nreasonable threshold.  (Thus, you can make many more cheap requests, or\nfewer expensive ones, your choice.)  All of this is per-repository, so\ngenerally only you (and maybe your colleagues or collaborators)\nexperience negative consequences if you attempt excessive use.\n\nI can't speak to other implementations, but robust rate limits are\ntypically common.  I'm sure all major implementations open to the public\nhave some sort of rate limiting because otherwise they'd be down a lot.\n\nThe difference is that failing operations and even well-explained,\nwell-documented rate limits cause a poor user experience, user\nfrustration, and user inquiries (e.g., support requests), as well as\npossibly noisy alerting and potentially pages for engineers.  Anytime a\nuser experiences rate limits, they have to make a change in their\nbehaviour, and people don't like change.\n\nI'd prefer we left the default to do the cheap no-op because, as I said,\nthat scales much better, and thus we allow users to do the obvious thing\nfor much longer and it just works for them.  That, in turn, provides\neveryone a better experience.\n\nCertainly, if people start using this option by default, then it will be\na problem if they engage in excessive use and server implementations\nwill probably scale worse, but usually users don't use non-default\noptions unless they need them, so I don't think your new proposed option\nis a problem.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"}]}