{"thread":{"id":"40081","subject":"Thoughts on refactoring the transport (+helper) code","startedAt":"2015-08-13T15:42:50Z","lastAt":"2015-08-13T16:50:33Z","messageCount":3,"participants":["Dave Borowitz","Ilari Liusvaara"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"267985","messageId":"CAD0k6qR5AgtaDX3HuE1NVHnxsrAnYFnV1TYHWJsCJoD22ABb1g@mail.gmail.com","threadId":"40081","inReplyTo":null,"subject":"Thoughts on refactoring the transport (+helper) code","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T15:42:50Z","receivedAt":"2015-08-13T15:42:50Z","isPatch":false,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"I spent a day trying to understand what the heck is going on in the\ntransport code, with the intent of adding an option like\n--sign-if-possible to git push. This has come up twice on the mailing\nlist in the past couple weeks, and I also think it's important for\n$DAY_JOB.\n\nI'm giving up in despair, but I decided to leave some comments that\nwill hopefully help a future, more enterprising refactorer. Please\ndon't read this (entirely) as a rant; I understand that the transport\ncode is old and hairy and grew organically to get to this point. I\nreally do just hope this makes the next guy's job easier in\nunderstanding the code. (But then again, this hope may be in vain:\nit's not like I searched the mailing list myself before diving in :)\n\nOne of the biggest issues IMO is that the idea of \"options\" for\ntransports is grossly overloaded:\n\n-struct git_transport_options contains mostly boolean options that are\nread in fetch.c to enable certain options.\n\n-In push.c, we for the most part ignore the smart_options field in\ntransport, and instead rely on the flags bitfield. However, some (not\nall) flags in this bitfield have seemingly-equivalent fields in\ngit_transport_options. In some cases (particularly push_cert), the\nfield in git_transport_options is ignored.\n\n-Similarly, one might think the executable arg to the connect callback\nmight bear some relation to the uploadpack/receivepack field in\nsmart_options. It does not.\n\n-Some (but by no means all) options are set via transport_set_option\nby passing in one of the TRANS_OPT_* string constants. This a)\nswitches on these values and populates the appropriate smart_options\nfield, and b) calls the set_option callback.\n\n-The end result is that the TRANS_OPT_* constants are a mixture of\nthings that can be set on the command line and things that are\ndocumented in the remote helper protocol. But there are also options\nused in the remote helper protocol that are hard-coded and have no\nconstant, or can't be set on the command line, etc.\n\n-The helper protocol's set_helper_option synchronously sends the\noption to the helper process and reads the response. Naturally, the\nreturn code from transport_set_option is almost always ignored.\n\nIn my ideal world:\n-smart_options would never be NULL, and would instead be called\n\"options\" with a \"smart\" bit which is unset for dumb protocols.\n-Command line option processing code in {fetch,clone,push}.c would set\nfields in options (formerly known as smart_options) rather than\npassing around string constants.\n-TRANS_OPT_* string constants would only be used for remote helper\nprotocol option names, and no more hard-coding these names.\n-The flags arg to the push* callbacks would go away, and callbacks\nwould respect options instead.\n-The helper code would not send options immediately, but instead send\njust the relevant options immediately before a particular command\nrequires them. Hopefully we could then eliminate the set_option\ncallback entirely. (Two things I ran into that complicated this: 1)\nbackfill_tags mutates just a couple of options before reusing the\ntransport, and 2) the handling of push_cas_option is very\nspecial-cased.)\n\nThere are other confusing things but I think that would make option\nhandling in particular less of a head-scratcher.\n"},{"id":"267995","messageId":"20150813164556.GA18951@LK-Perkele-VII","threadId":"40081","inReplyTo":"CAD0k6qR5AgtaDX3HuE1NVHnxsrAnYFnV1TYHWJsCJoD22ABb1g@mail.gmail.com","subject":"Re: Thoughts on refactoring the transport (+helper) code","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2015-08-13T16:45:56Z","receivedAt":"2015-08-13T16:45:56Z","isPatch":false,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Aug 13, 2015 at 11:42:50AM -0400, Dave Borowitz wrote:\n> \n> In my ideal world:\n> -smart_options would never be NULL, and would instead be called\n> \"options\" with a \"smart\" bit which is unset for dumb protocols.\n> -Command line option processing code in {fetch,clone,push}.c would set\n> fields in options (formerly known as smart_options) rather than\n> passing around string constants.\n> -TRANS_OPT_* string constants would only be used for remote helper\n> protocol option names, and no more hard-coding these names.\n> -The flags arg to the push* callbacks would go away, and callbacks\n> would respect options instead.\n> -The helper code would not send options immediately, but instead send\n> just the relevant options immediately before a particular command\n> requires them. Hopefully we could then eliminate the set_option\n> callback entirely. (Two things I ran into that complicated this: 1)\n> backfill_tags mutates just a couple of options before reusing the\n> transport, and 2) the handling of push_cas_option is very\n> special-cased.)\n\nAFAIK, here are what one can encounter with remote helpers:\n\nSome remote helpers are always smart, some are always dumb, and some\nmay be smart or dumb, depending on the URL.\n\nI don't know how useful the last one is (smart or dumb depending on\nURL) is. I have never seen such remote helper (HTTP doesn't count,\nfrom Git PoV it is always dumb).\n\nAll smart helpers take common options associated with git smart\ntransport (e.g. thin, follow tags, push certs, etc..).\n\nDumb transports may take some of these kind of of options (esp.\nsmart HTTP), but it is highly dependent on the helper.\n\nThen transports can have connection-level options (e.g. HTTPS\ncert options). These can be present regardless of wheither\ntransport is smart or dumb.\n\nThe receive-pack / send-pack paths fall into connection-level\noptions, even if those are presently in smart transport common\noptions. Since those things make sense for some smart transports\n(e.g. ssh://), but not others (e.g. git://).\n\n\n-Ilari\n"},{"id":"267997","messageId":"CAD0k6qRitPBaizsY7vy9FuLnwqF3nWQrmkSifvdB3z+oYqAivg@mail.gmail.com","threadId":"40081","inReplyTo":"20150813164556.GA18951@LK-Perkele-VII","subject":"Re: Thoughts on refactoring the transport (+helper) code","fromName":"Dave Borowitz","fromEmail":"dborowitz@google.com","sentAt":"2015-08-13T16:50:33Z","receivedAt":"2015-08-13T16:50:33Z","isPatch":false,"sender":{"key":"dborowitz@google.com","avatar":"https://avatars.githubusercontent.com/u/194927?v=4"},"body":"On Thu, Aug 13, 2015 at 12:45 PM, Ilari Liusvaara\n<ilari.liusvaara@elisanet.fi> wrote:\n> On Thu, Aug 13, 2015 at 11:42:50AM -0400, Dave Borowitz wrote:\n>>\n>> In my ideal world:\n>> -smart_options would never be NULL, and would instead be called\n>> \"options\" with a \"smart\" bit which is unset for dumb protocols.\n>> -Command line option processing code in {fetch,clone,push}.c would set\n>> fields in options (formerly known as smart_options) rather than\n>> passing around string constants.\n>> -TRANS_OPT_* string constants would only be used for remote helper\n>> protocol option names, and no more hard-coding these names.\n>> -The flags arg to the push* callbacks would go away, and callbacks\n>> would respect options instead.\n>> -The helper code would not send options immediately, but instead send\n>> just the relevant options immediately before a particular command\n>> requires them. Hopefully we could then eliminate the set_option\n>> callback entirely. (Two things I ran into that complicated this: 1)\n>> backfill_tags mutates just a couple of options before reusing the\n>> transport, and 2) the handling of push_cas_option is very\n>> special-cased.)\n>\n> AFAIK, here are what one can encounter with remote helpers:\n>\n> Some remote helpers are always smart, some are always dumb, and some\n> may be smart or dumb, depending on the URL.\n>\n> I don't know how useful the last one is (smart or dumb depending on\n> URL) is. I have never seen such remote helper (HTTP doesn't count,\n> from Git PoV it is always dumb).\n>\n> All smart helpers take common options associated with git smart\n> transport (e.g. thin, follow tags, push certs, etc..).\n>\n> Dumb transports may take some of these kind of of options (esp.\n> smart HTTP), but it is highly dependent on the helper.\n>\n> Then transports can have connection-level options (e.g. HTTPS\n> cert options). These can be present regardless of wheither\n> transport is smart or dumb.\n>\n> The receive-pack / send-pack paths fall into connection-level\n> options, even if those are presently in smart transport common\n> options. Since those things make sense for some smart transports\n> (e.g. ssh://), but not others (e.g. git://).\n\nYeah, thanks for summarizing some of the differences between options.\nThe really confusing thing when looking at the code, though, is that\nthe various different ways of specifying options in the code don't\nactually align with those distinctions. Maybe they once did, but they\ncertainly don't today.\n\nI wouldn't be opposed to splitting up git_transport_options into\ndifferent structs for connection options, smart fetch options, smart\npush options, etc., rather than putting everything in one kitchen-sink\nstruct.\n\n>\n> -Ilari\n"}]}