{"thread":{"id":"61068","subject":"[RFC/PATCH 0/2] some transport-helper \"option object-format\" confusion","startedAt":"2024-03-07T08:47:36Z","lastAt":"2024-03-27T09:48:42Z","messageCount":21,"participants":["Jeff King","brian m. carlson","Eric W. Biederman","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490146","messageId":"20240307084735.GA2072130@coredump.intra.peff.net","threadId":"61068","inReplyTo":null,"subject":"[RFC/PATCH 0/2] some transport-helper \"option object-format\" confusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-07T08:47:35Z","receivedAt":"2024-03-07T08:47:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be looking at the output of t5801 for an unrelated\nproblem, and I noticed our git-remote-testgit spewing a bunch of shell\nerrors. It turns out that its expectations do not quite match what the\ntransport-helper code produces.\n\nThis series brings the test and documentation in line with how the\ntransport-helper code behaves. But I'm not sure if we should be going\nthe other way (see the comments on patch 2 especially), and bringing the\ntransport-helper code in line with the others. Hence the RFC.\n\n  [1/2]: t5801: fix object-format handling in git-remote-testgit\n  [2/2]: doc/gitremote-helpers: match object-format option docs to code\n\n Documentation/gitremote-helpers.txt | 7 ++-----\n t/t5801/git-remote-testgit          | 6 ++++--\n 2 files changed, 6 insertions(+), 7 deletions(-)\n\n-Peff\n"},{"id":"490147","messageId":"20240307085158.GA2072294@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240307084735.GA2072130@coredump.intra.peff.net","subject":"[PATCH 1/2] t5801: fix object-format handling in git-remote-testgit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-07T08:51:58Z","receivedAt":"2024-03-07T08:51:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our fake remote helper tries to handle the object-format capability,\ncourtesy of 3716d50dd5 (remote-testgit: adapt for object-format,\n2020-06-19). But its parsing isn't quite right; it expects to receive\n\"option object-format true\", but the transport-helper code just sends\n\"option object-format\" with no value.\n\nAs a result, we never set the $object_format variable to \"true\". And\nworse, because $val is used unquoted, this confuses the shell's \"test\"\ncommand, which prints something like:\n\n  .../git/t/t5801/git-remote-testgit: 150: test: =: unexpected operator\n\nIt all turns out to be harmless, though, because we never look at\n$object_format after that!\n\nThe Git-side behavior comes from 8b85ee4f47 (transport-helper: implement\nobject-format extensions, 2020-05-25). It is a bit unlike other \"option\"\nvariables, which always say \"true\" or \"false\". But in this case, there's\nnot really any need to do so. As I understand it from that commit, the\nsequence is something like:\n\n  1. the remote helper in its capabilities list says \"object-format\" to\n     tell Git that it understands the object-format option.\n\n  2. Git then tells the helper \"option object-format\" to tell it that it\n     too understands object-formats.\n\n  3. when the remote helper lists refs, it sends a special\n     \":object-format\" line that tells Git which object format it is\n     using. But it presumably should only do this if we found out that\n     the other side supports object-formats in step (2).\n\nSo let's improve our remote-testgit helper a bit:\n\n  - when we see an object-format line, just set object_format=true;\n    that's the only useful thing to take away from it\n\n  - make sure that object_format is set before sending the special\n    \":object-format\" line. Since we're always testing against a version\n    of Git recent enough to have sent us the object-format option, this\n    is mostly a noop. But it confirms that the transport-helper code is\n    correctly sending us the option (if we fail to send the line, then\n    the test will fail when run with GIT_TEST_DEFAULT_HASH=sha256).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe only other helper we ship that knows about object-format is\nremote-curl. And there it _does_ expect \"true\" or an algorithm.\nCuriously the \"true\" thing works because the remote-curl code silently\nrewrites \"option foo\" to be the same as \"option foo true\". And even\nthough it understands receiving a specific algorithm, I'm not sure it\nwould do anything useful (whatever the caller says is generally\noverwritten by the info/refs response).\n\nSo I dunno.\n\n t/t5801/git-remote-testgit | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex 1544d6dc6b..b348608847 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -25,6 +25,7 @@ GIT_DIR=\"$url/.git\"\n export GIT_DIR\n \n force=\n+object_format=\n \n mkdir -p \"$dir\"\n \n@@ -56,7 +57,8 @@ do\n \t\techo\n \t\t;;\n \tlist)\n-\t\techo \":object-format $(git rev-parse --show-object-format=storage)\"\n+\t\ttest -n \"$object_format\" &&\n+\t\t\techo \":object-format $(git rev-parse --show-object-format=storage)\"\n \t\tgit for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'\n \t\thead=$(git symbolic-ref HEAD)\n \t\techo \"@$head HEAD\"\n@@ -142,7 +144,7 @@ do\n \t\t\techo \"ok\"\n \t\t\t;;\n \t\tobject-format)\n-\t\t\ttest $val = \"true\" && object_format=\"true\" || object_format=\n+\t\t\tobject_format=true\n \t\t\techo \"ok\"\n \t\t\t;;\n \t\t*)\n-- \n2.44.0.463.g71abcb3a9f\n\n"},{"id":"490148","messageId":"20240307085632.GB2072294@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240307084735.GA2072130@coredump.intra.peff.net","subject":"[PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-07T08:56:32Z","receivedAt":"2024-03-07T08:56:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git's transport-helper code has always sent \"option object-format\\n\",\nand never provided the \"true\" or \"algorithm\" arguments. While the\n\"algorithm\" request is something we might need or want to eventually\nsupport, it probably makes sense for now to document the actual\nbehavior, especially as it has been in place for several years, since\n8b85ee4f47 (transport-helper: implement object-format extensions,\n2020-05-25).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs I discussed in patch 1, remote-curl does handle the \"true\" thing\ncorrectly. And that's really the helper that matters in practice (it's\npossible some third party helper is looking for the explicit \"true\", but\npresumably they'd have reported their confusion to the list). So we\ncould probably just start tacking on the \"true\" in transport-helper.c\nand leave that part of the documentation untouched.\n\nI'm less sure of the specific-algorithm thing, just because it seems\nlike remote-curl would never make use of it anyway (preferring instead\nto match whatever algorithm is used by the http remote). But maybe there\nare pending interoperability plans that depend on this?\n\nI guess it would not hurt to leave it in place even if transport-helper\nnever produces it. On the other hand, any helper which advertises the\n\"object-format\" capability is supposed to support it, and without the\ntransport-helper side being implemented, I don't know how any helper\nprogram can claim that.\n\n Documentation/gitremote-helpers.txt | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 07c8439a6f..12dffbf383 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -542,13 +542,10 @@ set by Git if the remote helper has the 'option' capability.\n \ttransaction.  If successful, all refs will be updated, or none will.  If the\n \tremote side does not support this capability, the push will fail.\n \n-'option object-format' {'true'|algorithm}::\n-\tIf 'true', indicate that the caller wants hash algorithm information\n+'option object-format'::\n+\tIndicate that the caller wants hash algorithm information\n \tto be passed back from the remote.  This mode is used when fetching\n \trefs.\n-+\n-If set to an algorithm, indicate that the caller wants to interact with\n-the remote side using that algorithm.\n \n SEE ALSO\n --------\n-- \n2.44.0.463.g71abcb3a9f\n"},{"id":"490228","messageId":"Zeo9oAkL6kxZRugN@tapette.crustytoothpaste.net","threadId":"61068","inReplyTo":"20240307085632.GB2072294@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-07T22:20:16Z","receivedAt":"2024-03-07T22:20:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-07 at 08:56:32, Jeff King wrote:\n> Git's transport-helper code has always sent \"option object-format\\n\",\n> and never provided the \"true\" or \"algorithm\" arguments. While the\n> \"algorithm\" request is something we might need or want to eventually\n> support, it probably makes sense for now to document the actual\n> behavior, especially as it has been in place for several years, since\n> 8b85ee4f47 (transport-helper: implement object-format extensions,\n> 2020-05-25).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> As I discussed in patch 1, remote-curl does handle the \"true\" thing\n> correctly. And that's really the helper that matters in practice (it's\n> possible some third party helper is looking for the explicit \"true\", but\n> presumably they'd have reported their confusion to the list). So we\n> could probably just start tacking on the \"true\" in transport-helper.c\n> and leave that part of the documentation untouched.\n> \n> I'm less sure of the specific-algorithm thing, just because it seems\n> like remote-curl would never make use of it anyway (preferring instead\n> to match whatever algorithm is used by the http remote). But maybe there\n> are pending interoperability plans that depend on this?\n\nIt was designed to allow indicating that we know how to support both\nSHA-1 and SHA-256 and we want one or the other (so we don't need to do\nan expensive conversion).  However, if it's not implemented, I agree we\nshould document what's implemented, and then extend it when interop\ncomes.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"490450","messageId":"20240312074513.GA47852@coredump.intra.peff.net","threadId":"61068","inReplyTo":"Zeo9oAkL6kxZRugN@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-12T07:45:13Z","receivedAt":"2024-03-12T07:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2024 at 10:20:16PM +0000, brian m. carlson wrote:\n\n> > As I discussed in patch 1, remote-curl does handle the \"true\" thing\n> > correctly. And that's really the helper that matters in practice (it's\n> > possible some third party helper is looking for the explicit \"true\", but\n> > presumably they'd have reported their confusion to the list). So we\n> > could probably just start tacking on the \"true\" in transport-helper.c\n> > and leave that part of the documentation untouched.\n> > \n> > I'm less sure of the specific-algorithm thing, just because it seems\n> > like remote-curl would never make use of it anyway (preferring instead\n> > to match whatever algorithm is used by the http remote). But maybe there\n> > are pending interoperability plans that depend on this?\n> \n> It was designed to allow indicating that we know how to support both\n> SHA-1 and SHA-256 and we want one or the other (so we don't need to do\n> an expensive conversion).  However, if it's not implemented, I agree we\n> should document what's implemented, and then extend it when interop\n> comes.\n\nI guess my reservation is that when it _does_ come time to extend, we'll\nhave to introduce a new capability. The capability \"object-format\" has a\ndocumented meaning now, and what we send is currently a subset of that\n(sort of[1]). If we later start sending an explicit algorithm, then in\ntheory they're supposed to handle that, too, if they implemented against\nthe docs.\n\nWhereas if we roll back the explicit-algorithm part of the docs, now we\ncan't assume any helper claiming \"object-format\" will understand it. And\nwe'll need them to say \"object-format-extended\" or something. That's\nboth more work, and delays adoption for helpers which implemented what\nthe current docs say.\n\nSo I guess my question was more of: are we thinking this explicit\nalgorithm thing is coming very soon? If so, it might be worth keeping it\nin the docs. But if not, and it's just a hypothetical future, it may be\nbetter to clean things up now. And I ask you as the person who mostly\njuggles possible future algorithm plans in his head. ;) Of course if the\nanswer is some combination of \"I don't really remember what the plan\nwas\" and \"I don't have time to work on it anytime soon\" that's OK, too.\n\n-Peff\n\n[1] In the above I'm really just talking about the explicit-algorithm\n    part. The \"sort of\" is that we claim to send \"object-format true\"\n    but actually just send \"object-format\". There I'm more inclined to\n    just align the docs with practice, as the two are equivalent.\n"},{"id":"490581","messageId":"ZfIWkJieqcPv5jA8@tapette.crustytoothpaste.net","threadId":"61068","inReplyTo":"20240312074513.GA47852@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-13T21:11:44Z","receivedAt":"2024-03-13T21:11:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-12 at 07:45:13, Jeff King wrote:\n> So I guess my question was more of: are we thinking this explicit\n> algorithm thing is coming very soon? If so, it might be worth keeping it\n> in the docs. But if not, and it's just a hypothetical future, it may be\n> better to clean things up now. And I ask you as the person who mostly\n> juggles possible future algorithm plans in his head. ;) Of course if the\n> answer is some combination of \"I don't really remember what the plan\n> was\" and \"I don't have time to work on it anytime soon\" that's OK, too.\n\nThe answer is that I'm not planning on doing the SHA-1/SHA-256 interop\nwork except as part of my employment, since I'm kinda out of energy in\nthat area and it's a lot of work, and I don't believe that my employer\nis planning to have me do that anytime soon.  Thus, if nobody else is\nplanning on doing it in short order, it probably won't be getting done.\n\nI know Eric was working on some of the interop work, so perhaps he can\nspeak to whether he's planning on working in this area soonish.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490618","messageId":"87ttl99e0b.fsf@gmail.froward.int.ebiederm.org","threadId":"61068","inReplyTo":"ZfIWkJieqcPv5jA8@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2024-03-14T12:47:16Z","receivedAt":"2024-03-14T12:47:21Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2024-03-12 at 07:45:13, Jeff King wrote:\n>> So I guess my question was more of: are we thinking this explicit\n>> algorithm thing is coming very soon? If so, it might be worth keeping it\n>> in the docs. But if not, and it's just a hypothetical future, it may be\n>> better to clean things up now. And I ask you as the person who mostly\n>> juggles possible future algorithm plans in his head. ;) Of course if the\n>> answer is some combination of \"I don't really remember what the plan\n>> was\" and \"I don't have time to work on it anytime soon\" that's OK, too.\n\nGiven the rest of the conversation I thought something about the\nobject-format option was going to depend upon work that I am doing.\n\nReading up on object-format this seems to be something that should\nbe sorted out now.\n\nFundamentally the object-format code is about a client representing a\nSHA256 repository encountering a server representing a SHA1 repository\nand detecting and handling that case cleanly.  Or the other way around.\n\nThis is a current concern as SHA1 and SHA256 repositories are both\ncurrently supported.\n\nThe only future concern is what happens when a client for a SHA256\nrepository encounters a server serving a SHA1 repository and wants to\nswitch into a compatibility mode, before it starts sending something\nthat will confuse the server.\n\nThat said I think a lot of think we do a lot of that today in practice\nby simply detecting the length of the hash.\n\nIn general the plan is that all of the multiple hash interop work\nhappens on the client and the server worries about handling a single\nhash efficiently.\n\nThat said I haven't worked with the git protocol so I don't know\nwhat is needed in detail for a client to figure out what the server\nis speaking and cleanly abort, or quickly switch to the servers\nlanguage.  Jeff do you have any insight into that?\n\n> The answer is that I'm not planning on doing the SHA-1/SHA-256 interop\n> work except as part of my employment, since I'm kinda out of energy in\n> that area and it's a lot of work, and I don't believe that my employer\n> is planning to have me do that anytime soon.  Thus, if nobody else is\n> planning on doing it in short order, it probably won't be getting done.\n>\n> I know Eric was working on some of the interop work, so perhaps he can\n> speak to whether he's planning on working in this area soonish.\n\nSoon-ish.\n\nGetting the SHA1/SHA256 interop working is something that I feel pretty\nstrongly about.  So once I can set aside some time I am going to\npush forward with it.\n\nI have code doing with pretty much everything else working and tested\nexcept the actual interop working at this point.  That is I have code\nfor bi-hash repositories.\n\nBreaking everything into small enough chunks that people don't feel\ndaunted looking at the code has been a bit of a challenge.  My current\nplan is to write some ``unit tests'' (that is tests that test a single\nabstraction in the code at a time), so I can feel comfortable feeding\nthings in much smaller pieces.\n\nOnce the core infrastructure is merged for bi-hash repositories then\nI plan to work on the actual interop between the repositories.  With\nthe challenging technical problem I have been looking at is quickly\nand efficiently writing a pack in the repository hash, while\nretaining a translation to it's original hash.\n\nOnce the translation is done the rest is fiddly bits that should come\nfairly quickly and should be comparatively easy to review.  AKA things\nlike the client detecting the other end is using a different hash\nalgorithm and using that information to send heads in a format the\nserver understands.\n\n\nThat said I will be happy to help sort out object-format now.\nThat is maintenance and it has no dependencies that I am aware\nof.\n\n\n...\n\nThat said.  Sorting out object-format has no dependencies on anything\nelse I have been doing.  I will be happy to help sort that out right\nnow.\n\nEric\n\n\n\n\n"},{"id":"490620","messageId":"xmqqv85ozv3v.fsf@gitster.g","threadId":"61068","inReplyTo":"ZfIWkJieqcPv5jA8@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T15:33:24Z","receivedAt":"2024-03-14T15:33:28Z","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> The answer is that I'm not planning on doing the SHA-1/SHA-256 interop\n> work except as part of my employment, since I'm kinda out of energy in\n> that area and it's a lot of work, and I don't believe that my employer\n> is planning to have me do that anytime soon.\n\nIt is sad to hear that it is depriotised, even though it is one of\nthe larger areas with high importance for the longer term.  Thank\nyou very much for the progress in this area so far..\n\n> Thus, if nobody else is\n> planning on doing it in short order, it probably won't be getting done.\n>\n> I know Eric was working on some of the interop work, so perhaps he can\n> speak to whether he's planning on working in this area soonish.\n\n"},{"id":"490644","messageId":"ZfNqVowQBy47_92m@tapette.crustytoothpaste.net","threadId":"61068","inReplyTo":"87ttl99e0b.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-14T21:21:26Z","receivedAt":"2024-03-14T21:21:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-14 at 12:47:16, Eric W. Biederman wrote:\n> That said I think a lot of think we do a lot of that today in practice\n> by simply detecting the length of the hash.\n\nThat's only true for the dumb HTTP protocol.  Everything else should not\ndo that and we specifically want to avoid doing that, since we may very\nwell end up with SHA-3-256 or another 256-bit hash instead of SHA-256 if\nthere are sufficient cryptographic advances.\n\nIn fact, if we're going to support reftables via the dumb HTTP protocol,\nthen we should add some sort of capability advertisement that tells the\nremote side what functionality is supported, and simply specify the hash\nin that format.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490646","messageId":"ZfNyKIEOvnRKjL5O@tapette.crustytoothpaste.net","threadId":"61068","inReplyTo":"xmqqv85ozv3v.fsf@gitster.g","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-03-14T21:54:48Z","receivedAt":"2024-03-14T21:54:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-03-14 at 15:33:24, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > The answer is that I'm not planning on doing the SHA-1/SHA-256 interop\n> > work except as part of my employment, since I'm kinda out of energy in\n> > that area and it's a lot of work, and I don't believe that my employer\n> > is planning to have me do that anytime soon.\n> \n> It is sad to hear that it is depriotised, even though it is one of\n> the larger areas with high importance for the longer term.  Thank\n> you very much for the progress in this area so far..\n\nI don't want to claim that my employer is not prioritizing SHA-256, it's\njust that the focus right now is not having me write the interop code.\nOther work is ongoing which has and probably will in the future result\nin Git contributions, although not necessarily directly related to the\ninterop work.  Some of our work porting away from from libgit2 to Git to\nget better SHA-256 support has resulted in us writing new features which\nwe upstream.\n\nAs far as my personal contributions, I'm focusing on other, smaller\nGit-related things right now[0], and I'm just writing less code in C\n(and effectively no code in C other than Git).  And I'm also doing other\nthings in my life which leave me less time to work on Git.\n\n[0] Including, hopefully soon, some credential helper improvements.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"490692","messageId":"87msqzo63f.fsf@gmail.froward.int.ebiederm.org","threadId":"61068","inReplyTo":"ZfNqVowQBy47_92m@tapette.crustytoothpaste.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2024-03-15T15:41:24Z","receivedAt":"2024-03-15T15:41:28Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2024-03-14 at 12:47:16, Eric W. Biederman wrote:\n>> That said I think a lot of think we do a lot of that today in practice\n>> by simply detecting the length of the hash.\n>\n> That's only true for the dumb HTTP protocol.  Everything else should not\n> do that and we specifically want to avoid doing that, since we may very\n> well end up with SHA-3-256 or another 256-bit hash instead of SHA-256 if\n> there are sufficient cryptographic advances.\n\nMy apologies.  I thought Jeff King was reporting that object-format\nextension did not work, and that had been masked by a test.\n\nI see you saying and a quick grep through the code supports that the\nobject-format extension is implemented, and that the primary problem\nis that the Documentation varies slightly from what is implemented.\n\n\nLooking at the code I am left with the question:\n Is the object-format extension properly implemented in all cases?\n\n\nIf the object-format extension is properly implemented such that a\nclient and server mismatch can be detected I am for just Documenting\nwhat is currently implemented and calling it good.\n\nThe reason for that is\nDocumentation/technical/hash-function-transition.txt does not expect\nservers to support more than hash function.  I don't have a perspective\nthat differs.  So detecting what the client and server support and\nfailing if they differ should be good enough.\n\n\n\nI am concerned that the current code may not report it's hash function\nin all of the cases it needs to, to be able to detect a mismatch.\n\nI look at commit 8b85ee4f47aa (\"transport-helper: implement\nobject-format extensions\") and I don't see anything that generates\n\":object-format=\" after it has been asked for except the code\nin remote-curl.c added in commit 7f60501775b2 (\"remote-curl: implement\nobject-format extensions\").\n\nMaybe I am mistaken but a name like remote-curl has me strongly\nsuspecting that it does not cover all of the cases that git supports\nthat implement protocol v2.\n\nI think I see some omissions in updating the protocol v2 Documentation.\n\n\nCan some folks who understand how git protocol v2 is implemented better\nthat I do, tell me if I am seeing things or if it indeed looks like\nthere are some omissions in the object-format implementation?\n\nEric\n"},{"id":"490763","messageId":"20240316060427.GB32145@coredump.intra.peff.net","threadId":"61068","inReplyTo":"87msqzo63f.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-16T06:04:27Z","receivedAt":"2024-03-16T06:04:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 15, 2024 at 10:41:24AM -0500, Eric W. Biederman wrote:\n\n> I see you saying and a quick grep through the code supports that the\n> object-format extension is implemented, and that the primary problem\n> is that the Documentation varies slightly from what is implemented.\n> \n> \n> Looking at the code I am left with the question:\n>  Is the object-format extension properly implemented in all cases?\n> \n> \n> If the object-format extension is properly implemented such that a\n> client and server mismatch can be detected I am for just Documenting\n> what is currently implemented and calling it good.\n> \n> The reason for that is\n> Documentation/technical/hash-function-transition.txt does not expect\n> servers to support more than hash function.  I don't have a perspective\n> that differs.  So detecting what the client and server support and\n> failing if they differ should be good enough.\n\nAFAIK the code all works correctly, and there are no cases where we fail\nto notice a mismatch. The two code/doc inconsistencies (and bearing in\nmind this is for the transport-helper protocol, not the v2 protocol\nitself) are:\n\n  - the docs say \"object-format true\", but the code just says\n    \"object-format\". They're semantically equivalent, so it's just a\n    minor syntax issue.\n\n  - the docs say that Git may write \"object-format sha256\" to the\n    helper, but the code will never do that.\n\nSo my big question is for the second case: is that something that we'll\nneed to be able to do (possibly to support interop, but possibly for\nsome other case)? If not, we should probably just fix the docs. If so,\nthen we need to either fix the code, or accept that we'll need to add a\nnew capability/extension later.\n\n> I am concerned that the current code may not report it's hash function\n> in all of the cases it needs to, to be able to detect a mismatch.\n> \n> I look at commit 8b85ee4f47aa (\"transport-helper: implement\n> object-format extensions\") and I don't see anything that generates\n> \":object-format=\" after it has been asked for except the code\n> in remote-curl.c added in commit 7f60501775b2 (\"remote-curl: implement\n> object-format extensions\").\n> \n> Maybe I am mistaken but a name like remote-curl has me strongly\n> suspecting that it does not cover all of the cases that git supports\n> that implement protocol v2.\n\nThat all sounds right. We are talking just about the transport-helper\nprotocol here, where Git speaks to a separate program that actually\ncontacts the remote server. And the main helper we ship is remote-curl\n(which handles https, http, etc). Everything else is linked directly and\ndoes not need to use a separate process (we use a separate process to\navoid linking curl, openssl, etc into the main Git binary).\n\nWe do ship remote-fd and remote-ext, but they don't support most options\n(and probably don't need to, because they're mostly pass-throughs that\njust use the \"connect\" feature).\n\nThe other major helpers people tend to use are adapters to other version\ncontrol systems (e.g., remote-hg, cinnabar). We don't ship any of those\nourselves. They'll obviously need to learn about the transport-helper\nobject-format capability before they're ready to handle sha256 repos,\nbut I suspect that works has not really started.\n\n> I think I see some omissions in updating the protocol v2 Documentation.\n\nIf you mean from the commits listed above, I don't think so; they are\njust touching the transport-helper protocol, not the v2 wire protocol.\n\n-Peff\n"},{"id":"490823","messageId":"87v85k4mcp.fsf@gmail.froward.int.ebiederm.org","threadId":"61068","inReplyTo":"20240316060427.GB32145@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2024-03-17T20:47:18Z","receivedAt":"2024-03-17T20:47:21Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 15, 2024 at 10:41:24AM -0500, Eric W. Biederman wrote:\n>\n>> I see you saying and a quick grep through the code supports that the\n>> object-format extension is implemented, and that the primary problem\n>> is that the Documentation varies slightly from what is implemented.\n>> \n>> \n>> Looking at the code I am left with the question:\n>>  Is the object-format extension properly implemented in all cases?\n>> \n>> \n>> If the object-format extension is properly implemented such that a\n>> client and server mismatch can be detected I am for just Documenting\n>> what is currently implemented and calling it good.\n>> \n>> The reason for that is\n>> Documentation/technical/hash-function-transition.txt does not expect\n>> servers to support more than hash function.  I don't have a perspective\n>> that differs.  So detecting what the client and server support and\n>> failing if they differ should be good enough.\n>\n> AFAIK the code all works correctly, and there are no cases where we fail\n> to notice a mismatch. The two code/doc inconsistencies (and bearing in\n> mind this is for the transport-helper protocol, not the v2 protocol\n> itself)\n\nThank you for the explanation of the transport-helper vs the v2 helper\nprotocol explanation below.\n\n> are:\n>\n>   - the docs say \"object-format true\", but the code just says\n>     \"object-format\". They're semantically equivalent, so it's just a\n>     minor syntax issue.\n\nI am a bit confused on this point after having read the code.  It\nappears that when \"object-format\" is sent remote-curl\nexperiences \"object-format true\".\n\nAssuming remote-curl is the only remote helper that currently implements\nthe object-format capability.  I think we ant to fix transport-helper to\nsend \"object-format true\" just to be consistent with all of the other\noptions.\n\nAmong other things that will allow using the set_helper_option helper\nfunction, and it will generally keep the code robust as then the code\ndoesn't develop a special case for the one option that doesn't take an\noption value.\n\n>   - the docs say that Git may write \"object-format sha256\" to the\n>     helper, but the code will never do that.\n\nIt looks like remote_curl will get confused in that case when it\nprocesses \"object-format sha256\" as well.  As it stores that value in\noptions.hash_algo, which in all other cases is used to store what the\nhash algorithm computed from the remote side.\n\n> So my big question is for the second case: is that something that we'll\n> need to be able to do (possibly to support interop, but possibly for\n> some other case)? If not, we should probably just fix the docs. If so,\n> then we need to either fix the code, or accept that we'll need to add a\n> new capability/extension later.\n\nSince this is the transport helper understanding this enough\nto give a good reply is challenging.\n\nAs I read things the happy path for most connections is either going to\nturn into git protocol v2, git-fast-export, or git-fast-import.\nUnless I am misunderstanding something all of those will bypass\nthe code paths the remote helper object-format capability affects.\nIt is only when the remote helper send \"fallback\" during connect\nthat the remote helper format capability might be used.\n\nThe only practical need I can imagine for this is if the client\nis going to send oids before asking the remote side what it's oids\nare.  The only case I can imagine doing this is the initial push\nof a repository.\n\nMy sense is that unless we can find a current case that was overlooked\nduring the initial conversion we should remove \"object-format\n<hash-function>\" support from the code and the documentation.\n\nAny new cases that are not currently implemented will almost\ncertainly be handled by the \"smart\" protocols.\n\nLooking at the code in transport-helper.c:push_refs it appears the one\nuse case I can think of is explicitly not supported. The code says:\n>\tif (!remote_refs) {\n>\t\tfprintf(stderr,\n>\t\t\t_(\"No refs in common and none specified; doing nothing.\\n\"\n>\t\t\t  \"Perhaps you should specify a branch.\\n\"));\n>\t\treturn 0;\n>\t}\n\n\n>> I think I see some omissions in updating the protocol v2 Documentation.\n>\n> If you mean from the commits listed above, I don't think so; they are\n> just touching the transport-helper protocol, not the v2 wire protocol.\n\nThis just proves I haven't dug through these protocol bits enough to\nhave a good understanding of how they operate yet.\n\nSo I think at the end of the day we just want to do something\nthe diff below.\n\nMostly it deletes and simplifies code, but I found one case where\na malfunctioning remote helper could confuse us, so I added a check\nto ensure :object-format is sent when we expect it to be sent.\n\nDoes that jive with how you are reading the situation?\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex ed8da428c98b..47e5bb2cc925 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -542,7 +542,7 @@ set by Git if the remote helper has the 'option' capability.\n \ttransaction.  If successful, all refs will be updated, or none will.  If the\n \tremote side does not support this capability, the push will fail.\n \n-'option object-format' {'true'|algorithm}::\n+'option object-format' {'true'}::\n \tIf 'true', indicate that the caller wants hash algorithm information\n \tto be passed back from the remote.  This mode is used when fetching\n \trefs.\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1161dc7fed68..6f4cb3467458 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -213,12 +213,8 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"object-format\")) {\n \t\tint algo;\n \t\toptions.object_format = 1;\n-\t\tif (strcmp(value, \"true\")) {\n-\t\t\talgo = hash_algo_by_name(value);\n-\t\t\tif (algo == GIT_HASH_UNKNOWN)\n-\t\t\t\tdie(\"unknown object format '%s'\", value);\n-\t\t\toptions.hash_algo = &hash_algos[algo];\n-\t\t}\n+\t\tif (strcmp(value, \"true\"))\n+\t\t\tdie(\"unknown object format '%s'\", value);\n \t\treturn 0;\n \t} else {\n \t\treturn 1 /* unsupported */;\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b660b7942f9f..e648f136287d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1206,13 +1206,13 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \tstruct ref **tail = &ret;\n \tstruct ref *posn;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tbool received_object_format = false;\n \n \tdata->get_refs_list_called = 1;\n \thelper = get_helper(transport);\n \n \tif (data->object_format) {\n-\t\twrite_str_in_full(helper->in, \"option object-format\\n\");\n-\t\tif (recvline(data, &buf) || strcmp(buf.buf, \"ok\"))\n+\t\tif (set_helper_option(transport, \"object-format\", \"true\"))\n \t\t\texit(128);\n \t}\n \n@@ -1236,9 +1236,13 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \t\t\t\t\tdie(_(\"unsupported object format '%s'\"),\n \t\t\t\t\t    value);\n \t\t\t\ttransport->hash_algo = &hash_algos[algo];\n+\t\t\t\treceived_hash_algo = true;\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\telse if (data->object_format && !received_object_format) {\n+\t\t\tdie(_(\"missing :object-format\"));\n+\t\t}\n \n \t\teov = strchr(buf.buf, ' ');\n \t\tif (!eov)\n\nEric\n"},{"id":"490840","messageId":"20240318084937.GB602575@coredump.intra.peff.net","threadId":"61068","inReplyTo":"87v85k4mcp.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH 2/2] doc/gitremote-helpers: match object-format option docs to code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-18T08:49:37Z","receivedAt":"2024-03-18T08:49:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 17, 2024 at 03:47:18PM -0500, Eric W. Biederman wrote:\n\n> >   - the docs say \"object-format true\", but the code just says\n> >     \"object-format\". They're semantically equivalent, so it's just a\n> >     minor syntax issue.\n> \n> I am a bit confused on this point after having read the code.  It\n> appears that when \"object-format\" is sent remote-curl\n> experiences \"object-format true\".\n\nRight, this is due to this code in remote-curl.c:\n\n                  } else if (skip_prefix(buf.buf, \"option \", &arg)) {\n                          char *value = strchr(arg, ' ');\n                          int result;\n  \n                          if (value)\n                                  *value++ = '\\0';\n                          else\n                                  value = \"true\";\n\nwhich goes way back to the beginning of remote-curl, even though I don't\nthink Git ever sends a value-less option. Anyway, that's presumably why\nnobody noticed that \"option object-format\" is unusual.\n\n> Assuming remote-curl is the only remote helper that currently implements\n> the object-format capability.  I think we ant to fix transport-helper to\n> send \"object-format true\" just to be consistent with all of the other\n> options.\n\nWe could be breaking third-party helpers that we don't know about. Of\ncourse, those helpers would have to have ignored the documentation. And\nI suspect they simply don't exist, or somebody would have showed up and\nasked about it (coupled with how new and relatively obscure the hash\nalgorithm work has been so far).\n\nSo maybe we can get away with fixing it now. We should definitely break\nit out into its own patch so we can decide independently, though.\n\n> >   - the docs say that Git may write \"object-format sha256\" to the\n> >     helper, but the code will never do that.\n> \n> It looks like remote_curl will get confused in that case when it\n> processes \"object-format sha256\" as well.  As it stores that value in\n> options.hash_algo, which in all other cases is used to store what the\n> hash algorithm computed from the remote side.\n\nYeah, it ends up in the same variable. I _suspect_ it would simply be\noverwritten by the remote repo's idea of the hash. I'm not sure if\nthat's a bug (if the specific algorithm given by the main process is\nsupposed to take precedence) or a feature (if it's just a suggestion,\nand then the helper says \"tough luck, the remote is using sha1\"). It's\nhard to tell because Git never sends it. ;)\n\n> As I read things the happy path for most connections is either going to\n> turn into git protocol v2, git-fast-export, or git-fast-import.\n> Unless I am misunderstanding something all of those will bypass\n> the code paths the remote helper object-format capability affects.\n> It is only when the remote helper send \"fallback\" during connect\n> that the remote helper format capability might be used.\n\nYeah, I suspect that is true for remote-curl. It may not be for other\nhelpers which don't support \"connect\".\n\n> The only practical need I can imagine for this is if the client\n> is going to send oids before asking the remote side what it's oids\n> are.  The only case I can imagine doing this is the initial push\n> of a repository.\n\nHmm, I _think_ we are OK there in practice. Even if there are no refs on\nthe remote repo (running git-receive-pack), it will still issue a\ncapability line with its object-format. And then the helper (say,\nremote-curl) will report that back to the caller (git-push) who might\nsay \"hey, wait, there's a mismatch\". And indeed, it seems to work in\npractice with remote-curl, where the push yields:\n\n  fatal: the receiving end does not support this repository's hash algorithm\n\nIn theory I suppose Git could directly issue a \"push\" command to the\nhelper (which would then specify oids along with refs to push) without\never issuing \"list for-push\" (which is what causes the helper to contact\nthe remote to discover and report back the object format). But it\ndoesn't do that, and I don't see why it ever would.\n\nThis is all neglecting dumb protocols that don't even know how to figure\nout the object format of the other side, but I think that's an\northogonal problem. Either it remains unsolved, or whatever solution we\ncome up with then gets pushed back over the transport-helper protocol in\nthe same way.\n\n> My sense is that unless we can find a current case that was overlooked\n> during the initial conversion we should remove \"object-format\n> <hash-function>\" support from the code and the documentation.\n\nYeah, if you don't have any plans to use it for interop work, then I\nthink we can declare it useless. I'll rework my patch series a bit to\nremove the useless sending-side code, and then add a patch on top to\nswitch the \"true\" syntax as discussed above.\n\n> Looking at the code in transport-helper.c:push_refs it appears the one\n> use case I can think of is explicitly not supported. The code says:\n> >\tif (!remote_refs) {\n> >\t\tfprintf(stderr,\n> >\t\t\t_(\"No refs in common and none specified; doing nothing.\\n\"\n> >\t\t\t  \"Perhaps you should specify a branch.\\n\"));\n> >\t\treturn 0;\n> >\t}\n\nThat only triggers if you didn't ask to push anything. You might have a\nsha256 ref locally and say \"git push origin my-branch\", and then we'd\nneed to communicate the ref/oid combo for my-branch to the helper. But\nas above, I think by that point the helper will have discovered and\nreported back the object-format to push.\n\n> Mostly it deletes and simplifies code, but I found one case where\n> a malfunctioning remote helper could confuse us, so I added a check\n> to ensure :object-format is sent when we expect it to be sent.\n\nThat's probably a reasonable thing to check. We should update the docs\nto indicate that it's required to send back \":object-format\" if the\nhelper negotiated that capability. I'll add a patch to do that.\n\n> Does that jive with how you are reading the situation?\n\nYep, I think I have a good sense how to proceed. It may be a day or so\nbefore I produce a series. Thanks for the discussion!\n\n-Peff\n"},{"id":"491002","messageId":"20240320093226.GA2445531@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240307084735.GA2072130@coredump.intra.peff.net","subject":"[PATCH 0/3] some transport-helper \"option object-format\" confusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T09:32:26Z","receivedAt":"2024-03-20T09:32:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2024 at 03:47:35AM -0500, Jeff King wrote:\n\n> I happened to be looking at the output of t5801 for an unrelated\n> problem, and I noticed our git-remote-testgit spewing a bunch of shell\n> errors. It turns out that its expectations do not quite match what the\n> transport-helper code produces.\n> \n> This series brings the test and documentation in line with how the\n> transport-helper code behaves. But I'm not sure if we should be going\n> the other way (see the comments on patch 2 especially), and bringing the\n> transport-helper code in line with the others. Hence the RFC.\n> \n>   [1/2]: t5801: fix object-format handling in git-remote-testgit\n>   [2/2]: doc/gitremote-helpers: match object-format option docs to code\n\nHere's a non-RFC v2 based on the discussion thus far (thanks brian and\nEric).\n\nThe big change is that instead of changing the docs to match true-less\n\"option object-format\", the code is changed to match the docs. That\nhappens in patch 3 (which subsumes the original patch 1). We continue to\ndrop the documentation for the \"option object-format sha256\" form. But\nnow the commit message justifies it better, and we clean up the stale\ncode in remote-curl.c.\n\nPatch 1 is a small fix for debugging output that I noticed after getting\nconfused. :-/ It's not strictly related and could be taken separately.\n\nEric mentioned having Git check that the helpers never say\n\":object-format\" unless it was negotiated. I stopped short of that. One,\nit's a bit tricky to test (since Git will always ask for object-format,\nyou have to teach remote-testgit to optionally send broken output). And\ntwo, I'm not sure that being strict has much value here. It keeps remote\nhelpers honest, but the real losers are old versions that do not\nunderstand :object-format, which would fail against such a remote. So I\ndunno. It isn't any harder to do it on top later if we want to.\n\n  [1/3]: transport-helper: use write helpers more consistently\n  [2/3]: transport-helper: drop \"object-format <algo>\" option\n  [3/3]: transport-helper: send \"true\" value for object-format option\n\n Documentation/gitremote-helpers.txt |  7 ++-----\n remote-curl.c                       |  9 ++-------\n t/t5801/git-remote-testgit          |  4 +++-\n transport-helper.c                  | 11 ++++-------\n 4 files changed, 11 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"491003","messageId":"20240320093417.GA2445682@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240320093226.GA2445531@coredump.intra.peff.net","subject":"[PATCH 1/3] transport-helper: use write helpers more consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T09:34:17Z","receivedAt":"2024-03-20T09:34:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The transport-helper code provides some functions for writing to the\nhelper process, but there are a few spots that don't use them. We should\ndo so consistently because:\n\n  1. They detect errors on write (though in practice this means the\n     helper process went away, and we'd see the problem as soon as we\n     try to read the response).\n\n  2. They dump the written bytes to the GIT_TRANSPORT_HELPER_DEBUG\n     stream. It's doubly confusing to miss some writes but not others,\n     as you see a partial conversation.\n\nThe \"list\" ones go all the way back to the beginning of the transport\nhelper code; they were just missed when most writes were converted in\nbf3c523c3f (Add remote helper debug mode, 2009-12-09). The nearby\n\"object-format\" write presumably just cargo-culted them, as it's only a\nfew lines away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also find the output kind of verbose (especially the constant\n\"waiting\" lines), and because it's not GIT_TRACE_TRANSPORT_HELPER, it's\nannoying to use with the test scripts (it gets eaten by the test\nharness, and you can't even redirect it to an alternative file).\nSo I was tempted to convert it, but it felt like too deep a rabbit hole\nfor today.\n\n transport-helper.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b660b7942f..7f6bbd06bb 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1211,15 +1211,15 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \thelper = get_helper(transport);\n \n \tif (data->object_format) {\n-\t\twrite_str_in_full(helper->in, \"option object-format\\n\");\n+\t\twrite_constant(helper->in, \"option object-format\\n\");\n \t\tif (recvline(data, &buf) || strcmp(buf.buf, \"ok\"))\n \t\t\texit(128);\n \t}\n \n \tif (data->push && for_push)\n-\t\twrite_str_in_full(helper->in, \"list for-push\\n\");\n+\t\twrite_constant(helper->in, \"list for-push\\n\");\n \telse\n-\t\twrite_str_in_full(helper->in, \"list\\n\");\n+\t\twrite_constant(helper->in, \"list\\n\");\n \n \twhile (1) {\n \t\tchar *eov, *eon;\n-- \n2.44.0.650.g4615f65fe0\n\n"},{"id":"491004","messageId":"20240320093740.GB2445682@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240320093226.GA2445531@coredump.intra.peff.net","subject":"[PATCH 2/3] transport-helper: drop \"object-format <algo>\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T09:37:40Z","receivedAt":"2024-03-20T09:37:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation in gitremote-helpers.txt claims that helpers should\naccept an object-format option from Git whose value is either:\n\n  1. \"true\", in which case the helper is merely told that Git\n     understands the special \":object-format\" response, and will send it\n\n  2. an algorithm name that the helper should use\n\nHowever, Git has never sent the second form, and it's not clear if it\nwould ever be useful.\n\nWhen interacting with a remote Git repository, we generally discover\nwhat _their_ object format is, and then decide what to do with a\nmismatch (where that is currently just \"bail out\", but could eventually\nbe on-the-fly conversion and interop). And that is true for native\nprotocols, but also for transport helpers like remote-curl that talk to\nremote Git repositories.  There we send back an \":object-format\" line\ntelling Git what remote-curl detected on the other side.\n\nAnd this is true even for pushes (since we get it via receive-pack's\nadvertisement). And it is even true for dumb-http, as we guess at the\nalgorithm based on the hash size, due to ac093d0790 (remote-curl: detect\nalgorithm for dumb HTTP by size, 2020-06-19).\n\nThe one case where it _isn't_ true is dumb-http talking to an empty\nrepository. There we have no clue what the remote hash is, so\nremote-curl just sends back its default. If we kept the \"object-format\n<algo>\" form then in theory Git could say \"object-format sha256\" to\nchange that default. But it doesn't really accomplish anything. We still\nmay or may not be mis-matched with the other side. For a fetch that's\nOK, since it's by definition a noop. For a push into an empty\nrepository, it might matter (though the dumb http-push DAV code seems\nhappy to clobber a remote sha256 info/refs and corrupt the repository).\nIf we want to pursue making this work, I think we'd be better off\nimproving detection of the object format of empty repositories over\ndumb-http (e.g., an \"info/object-format\" file).\n\nBut what about helpers that _aren't_ talking to another Git repo?\nConsider something like git-cinnabar, which is converting on the fly\nto/from hg. Most of the heavy lifting is done by fast-import/export, but\nsome oids may still pass between Git and the helper. Could\n\"object-format <algo>\" be useful to tell the helper what oids we expect\nto see?\n\nPossibly, but in practice this isn't necessary. Git-cinnabar for example\nalready peeks at the local-repo .git/config to check its object-format\n(and currently just bails if it is sha256).\n\nSo I think the \"object-format\" extension really is only useful for the\nhelper telling Git what object-format it found, and not the other way\naround.\n\nNote that this patch can't break any remote helpers; we're not changing\nthe code on the Git side at all, but just bringing the documentation in\nline with what Git has always done. It does remove the receiving support\nin remote-curl.c, but that code was never actually triggered.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/gitremote-helpers.txt | 7 ++-----\n remote-curl.c                       | 9 ++-------\n 2 files changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 07c8439a6f..0d3f4f37c2 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -542,13 +542,10 @@ set by Git if the remote helper has the 'option' capability.\n \ttransaction.  If successful, all refs will be updated, or none will.  If the\n \tremote side does not support this capability, the push will fail.\n \n-'option object-format' {'true'|algorithm}::\n-\tIf 'true', indicate that the caller wants hash algorithm information\n+'option object-format true'::\n+\tIndicate that the caller wants hash algorithm information\n \tto be passed back from the remote.  This mode is used when fetching\n \trefs.\n-+\n-If set to an algorithm, indicate that the caller wants to interact with\n-the remote side using that algorithm.\n \n SEE ALSO\n --------\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1161dc7fed..31b02b8840 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -211,14 +211,9 @@ static int set_option(const char *name, const char *value)\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n \t} else if (!strcmp(name, \"object-format\")) {\n-\t\tint algo;\n \t\toptions.object_format = 1;\n-\t\tif (strcmp(value, \"true\")) {\n-\t\t\talgo = hash_algo_by_name(value);\n-\t\t\tif (algo == GIT_HASH_UNKNOWN)\n-\t\t\t\tdie(\"unknown object format '%s'\", value);\n-\t\t\toptions.hash_algo = &hash_algos[algo];\n-\t\t}\n+\t\tif (strcmp(value, \"true\"))\n+\t\t\tdie(_(\"unknown value for object-format: %s\"), value);\n \t\treturn 0;\n \t} else {\n \t\treturn 1 /* unsupported */;\n-- \n2.44.0.650.g4615f65fe0\n\n"},{"id":"491005","messageId":"20240320094103.GC2445682@coredump.intra.peff.net","threadId":"61068","inReplyTo":"20240320093226.GA2445531@coredump.intra.peff.net","subject":"[PATCH 3/3] transport-helper: send \"true\" value for object-format option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T09:41:03Z","receivedAt":"2024-03-20T09:41:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation in gitremote-helpers.txt claims that after a helper\nhas advertised the \"object-format\" capability, Git may then send \"option\nobject-format true\" to indicate that it would like to hear which object\nformat the helper is using when it returns refs.\n\nHowever, the code implementing this has always written just \"option\nobject-format\", without the extra \"true\" value. Nobody noticed in\npractice or in the tests because the only two helpers we ship are:\n\n  - remote-curl, which quietly converts missing values into \"true\". This\n    goes all the way back to ef08ef9ea0 (remote-helpers: Support custom\n    transport options, 2009-10-30), despite the fact that I don't think\n    any other option has ever made use of it.\n\n  - remote-testgit in t5801 does insist on having a \"true\" value. But\n    since it sends the \":object-format\" response regardless of whether\n    it thinks the caller asked for it (technically breaking protocol),\n    everything just works, albeit with an extra shell error:\n\n      .../git/t/t5801/git-remote-testgit: 150: test: =: unexpected operator\n\n    printed to stderr, which you can see running t5801 with --verbose.\n    (The problem is that $val is the empty string, and since we don't\n    double-quote it in \"test $val = true\", we invoke \"test = true\"\n    instead).\n\nWhen the documentation and code do not match, it is often good to fix\nthe documentation rather than break compatibility. And in this case, we\nhave had the mis-match since 8b85ee4f47 (transport-helper: implement\nobject-format extensions, 2020-05-25). However, the sha256 feature was\nlisted as experimental until 8e42eb0e9a (doc: sha256 is no longer\nexperimental, 2023-07-31).\n\nIt's possible there are some third party helpers that tried to follow\nthe documentation, and are broken. Changing the code will fix them. It's\nalso possible that there are ones that follow the code and will be\nbroken if we change it. I suspect neither is the case given that no\nhelper authors have brought this up as an issue (I only noticed it\nbecause I was running t5801 in verbose mode for other reasons and\nwondered about the weird shell error). That, coupled with the relative\nnew-ness of sha256, makes me think nobody has really worked on helpers\nfor it yet, which gives us an opportunity to correct the code before too\nmuch time passes.\n\nAnd doing so has some value: it brings \"object-format\" in line with the\nsyntax of other options, making the protocol more consistent. It also\nlets us use set_helper_option(), which has better error reporting.\n\nNote that we don't really need to allow any other values like \"false\"\nhere. The point is for Git to tell the helper that it understands\n\":object-format\" lines coming back as part of the ref listing. There's\nno point in future versions saying \"no, I don't understand that\".\n\nTo make sure everything works as expected, we can improve the\nremote-testgit helper from t5801 to send the \":object-format\" line only\nif the other side correctly asked for it (which modern Git will always\ndo). With that test change and without the matching code fix here, t5801\nwill fail when run with GIT_TEST_DEFAULT_HASH=sha256.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5801/git-remote-testgit | 4 +++-\n transport-helper.c         | 7 ++-----\n 2 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex bcfb358c51..c5b10f5775 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -30,6 +30,7 @@ GIT_DIR=\"$url/.git\"\n export GIT_DIR\n \n force=\n+object_format=\n \n mkdir -p \"$dir\"\n \n@@ -61,7 +62,8 @@ do\n \t\techo\n \t\t;;\n \tlist)\n-\t\techo \":object-format $(git rev-parse --show-object-format=storage)\"\n+\t\ttest -n \"$object_format\" &&\n+\t\t\techo \":object-format $(git rev-parse --show-object-format=storage)\"\n \t\tgit for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'\n \t\thead=$(git symbolic-ref HEAD)\n \t\techo \"@$head HEAD\"\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 7f6bbd06bb..8d284b24d5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1210,11 +1210,8 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \tdata->get_refs_list_called = 1;\n \thelper = get_helper(transport);\n \n-\tif (data->object_format) {\n-\t\twrite_constant(helper->in, \"option object-format\\n\");\n-\t\tif (recvline(data, &buf) || strcmp(buf.buf, \"ok\"))\n-\t\t\texit(128);\n-\t}\n+\tif (data->object_format)\n+\t\tset_helper_option(transport, \"object-format\", \"true\");\n \n \tif (data->push && for_push)\n \t\twrite_constant(helper->in, \"list for-push\\n\");\n-- \n2.44.0.650.g4615f65fe0\n"},{"id":"491025","messageId":"87y1ac3kb6.fsf@gmail.froward.int.ebiederm.org","threadId":"61068","inReplyTo":"20240320093226.GA2445531@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] some transport-helper \"option object-format\" confusion","fromName":"Eric W. Biederman","fromEmail":"ebiederm@gmail.com","sentAt":"2024-03-20T17:05:49Z","receivedAt":"2024-03-20T17:05:52Z","isPatch":true,"sender":{"key":"ebiederm@gmail.com","avatar":null},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 07, 2024 at 03:47:35AM -0500, Jeff King wrote:\n>\n>> I happened to be looking at the output of t5801 for an unrelated\n>> problem, and I noticed our git-remote-testgit spewing a bunch of shell\n>> errors. It turns out that its expectations do not quite match what the\n>> transport-helper code produces.\n>> \n>> This series brings the test and documentation in line with how the\n>> transport-helper code behaves. But I'm not sure if we should be going\n>> the other way (see the comments on patch 2 especially), and bringing the\n>> transport-helper code in line with the others. Hence the RFC.\n>> \n>>   [1/2]: t5801: fix object-format handling in git-remote-testgit\n>>   [2/2]: doc/gitremote-helpers: match object-format option docs to code\n>\n> Here's a non-RFC v2 based on the discussion thus far (thanks brian and\n> Eric).\n>\n> The big change is that instead of changing the docs to match true-less\n> \"option object-format\", the code is changed to match the docs. That\n> happens in patch 3 (which subsumes the original patch 1). We continue to\n> drop the documentation for the \"option object-format sha256\" form. But\n> now the commit message justifies it better, and we clean up the stale\n> code in remote-curl.c.\n>\n> Patch 1 is a small fix for debugging output that I noticed after getting\n> confused. :-/ It's not strictly related and could be taken separately.\n>\n> Eric mentioned having Git check that the helpers never say\n> \":object-format\" unless it was negotiated. I stopped short of that. One,\n> it's a bit tricky to test (since Git will always ask for object-format,\n> you have to teach remote-testgit to optionally send broken output). And\n> two, I'm not sure that being strict has much value here. It keeps remote\n> helpers honest, but the real losers are old versions that do not\n> understand :object-format, which would fail against such a remote. So I\n> dunno. It isn't any harder to do it on top later if we want to.\n\nYour sentence has what I was asking for backwards.  It would be healthy\nif the code fails when \"object-format\" has been advertised by the\nremote, requested by the transport-helper, and the remote does not send\n\":object-format\".\n\nThe check is cheap and should prevent buggy remotes from appearing in\nthe wild.  I am probably biased but I rather want the information\non what hash algorithm the remote is using when I ask for it.\n\nI can totally imagine someone during development forgetting to send\n:object-format and either not noticing something was wrong, or spending\na fair amount of time debugging that they forgot to send it.\n\nIt is the kind of bug I can imagine someone making when they are called\naway from the keyboard at the wrong moment.\n\nThe implementation should just be:\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b660b7942f9f..e648f136287d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1206,6 +1206,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \tstruct ref **tail = &ret;\n \tstruct ref *posn;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tbool received_object_format = false;\n \n \tdata->get_refs_list_called = 1;\n \thelper = get_helper(transport);\n@@ -1236,9 +1236,13 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n \t\t\t\t\tdie(_(\"unsupported object format '%s'\"),\n \t\t\t\t\t    value);\n \t\t\t\ttransport->hash_algo = &hash_algos[algo];\n+\t\t\t\treceived_object_format = true;\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n+\t\telse if (data->object_format && !received_object_format) {\n+\t\t\tdie(_(\"missing :object-format\"));\n+\t\t}\n \n \t\teov = strchr(buf.buf, ' ');\n \t\tif (!eov)\n\nAm I missing something that makes a bad implementation?\n\nHmm.  I thought gitremote-helpers.txt said the key value pairs\nwould precede everything else from a list command.\ngitremote-helpers.txt does not mention that.  That looks like\na Documentation oversight.\n\nHowever remote-curl.c in output_refs prints :object-format before\nanything else, and transport-helper.c will malfunction if :object-format\nis sent after any of the refs.  As transport->hash_algop is used by\nget_oid_hex_algop is used to parse the oids of the refs.\n\nWe can probably fix the Documentation like:\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex ed8da428c98b..b6ca29a245f3 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -268,6 +268,8 @@ Support for this command is mandatory.\n \tref. A space-separated list of attributes follows the name;\n \tunrecognized attributes are ignored. The list ends with a\n \tblank line.\n+\n+\tKeywords should precede everything else in the list.\n +\n See REF LIST ATTRIBUTES for a list of currently defined attributes.\n See REF LIST KEYWORDS for a list of currently defined keywords.\n\nI do agree that the sanity check can be added to your series, so if you\nwould prefer I can do that.\n\nEric\n"},{"id":"491026","messageId":"xmqq1q84hl6d.fsf@gitster.g","threadId":"61068","inReplyTo":"20240320094103.GC2445682@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] transport-helper: send \"true\" value for object-format option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-20T17:23:22Z","receivedAt":"2024-03-20T17:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   - remote-curl, which quietly converts missing values into \"true\". This\n>     goes all the way back to ef08ef9ea0 (remote-helpers: Support custom\n>     transport options, 2009-10-30), despite the fact that I don't think\n>     any other option has ever made use of it.\n\nInteresting.\n\n> When the documentation and code do not match, it is often good to fix\n> the documentation rather than break compatibility. And in this case, we\n> have had the mis-match since 8b85ee4f47 (transport-helper: implement\n> object-format extensions, 2020-05-25). However, the sha256 feature was\n> listed as experimental until 8e42eb0e9a (doc: sha256 is no longer\n> experimental, 2023-07-31).\n> ...\n> And doing so has some value: it brings \"object-format\" in line with the\n> syntax of other options, making the protocol more consistent. It also\n> lets us use set_helper_option(), which has better error reporting.\n\nI suspect that this may have been an attempt to mimick the\nvalue-less true in the configuration syntax, but I agree with the\nconclusion of this patch.  Boolean \"true\" in the context of the\ntransport options may be fairly common, but unlike configuration\nfiles, it is not something we have users write manually, and there\nis not much point giving a special short form.\n\nThanks for a pleasant read.  Will queue.\n"},{"id":"491679","messageId":"20240327094840.GA857435@coredump.intra.peff.net","threadId":"61068","inReplyTo":"87y1ac3kb6.fsf@gmail.froward.int.ebiederm.org","subject":"Re: [PATCH 0/3] some transport-helper \"option object-format\" confusion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-27T09:48:40Z","receivedAt":"2024-03-27T09:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 20, 2024 at 12:05:49PM -0500, Eric W. Biederman wrote:\n\n> Your sentence has what I was asking for backwards.  It would be healthy\n> if the code fails when \"object-format\" has been advertised by the\n> remote, requested by the transport-helper, and the remote does not send\n> \":object-format\".\n\nAh, I see. That is probably reasonable, under the assumption that nobody\nwould have implemented \"object-format\" so far and _not_ sent it. It\nmight be worth clarifying the documentation at the same time.\n\n> The implementation should just be:\n> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index b660b7942f9f..e648f136287d 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1206,6 +1206,7 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n>  \tstruct ref **tail = &ret;\n>  \tstruct ref *posn;\n>  \tstruct strbuf buf = STRBUF_INIT;\n> +\tbool received_object_format = false;\n>  \n>  \tdata->get_refs_list_called = 1;\n>  \thelper = get_helper(transport);\n> @@ -1236,9 +1236,13 @@ static struct ref *get_refs_list_using_list(struct transport *transport,\n>  \t\t\t\t\tdie(_(\"unsupported object format '%s'\"),\n>  \t\t\t\t\t    value);\n>  \t\t\t\ttransport->hash_algo = &hash_algos[algo];\n> +\t\t\t\treceived_object_format = true;\n>  \t\t\t}\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\telse if (data->object_format && !received_object_format) {\n> +\t\t\tdie(_(\"missing :object-format\"));\n> +\t\t}\n>  \n>  \t\teov = strchr(buf.buf, ' ');\n>  \t\tif (!eov)\n> \n> Am I missing something that makes a bad implementation?\n\nNo, that seems right to me (modulo that we do not use C99 \"bool\" in our\ncode base).\n\n> Hmm.  I thought gitremote-helpers.txt said the key value pairs\n> would precede everything else from a list command.\n> gitremote-helpers.txt does not mention that.  That looks like\n> a Documentation oversight.\n> \n> However remote-curl.c in output_refs prints :object-format before\n> anything else, and transport-helper.c will malfunction if :object-format\n> is sent after any of the refs.  As transport->hash_algop is used by\n> get_oid_hex_algop is used to parse the oids of the refs.\n\nYeah, I think it is a natural consequence of \"object-format\", since it\nis necessary for parsing the result. And since there aren't any other\nkeywords yet, we can surmise that nobody is doing the wrong thing yet.\nSo now is a good time to clarify the documentation.\n\nI'm also not sure if we ever say explicitly in the documentation that\nthe keywords start with a colon. But maybe I am just missing it.\n\n> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> index ed8da428c98b..b6ca29a245f3 100644\n> --- a/Documentation/gitremote-helpers.txt\n> +++ b/Documentation/gitremote-helpers.txt\n> @@ -268,6 +268,8 @@ Support for this command is mandatory.\n>  \tref. A space-separated list of attributes follows the name;\n>  \tunrecognized attributes are ignored. The list ends with a\n>  \tblank line.\n> +\n> +\tKeywords should precede everything else in the list.\n>  +\n>  See REF LIST ATTRIBUTES for a list of currently defined attributes.\n>  See REF LIST KEYWORDS for a list of currently defined keywords.\n> \n> I do agree that the sanity check can be added to your series, so if you\n> would prefer I can do that.\n\nYeah, do you want to send some patches that can go on top of mine?\n\n-Peff\n"}]}