{"thread":{"id":"42879","subject":"Looking for help to understand external filter driver code","startedAt":"2016-07-19T16:40:55Z","lastAt":"2016-07-20T13:49:24Z","messageCount":10,"participants":["Lars Schneider","Junio C Hamano","Torsten Bögershausen","Jakub Narębski","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291729","messageId":"67D9AC88-550E-4549-9AFD-2401B70B363B@gmail.com","threadId":"42879","inReplyTo":null,"subject":"Looking for help to understand external filter driver code","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-19T16:40:48Z","receivedAt":"2016-07-19T16:40:55Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\na long time ago in aa4ed4 Junio introduced the external filter driver definition. Since that time we fork the Git process and then we fork again to run the external filter. This is probably a super stupid question with an obvious answer... but can anyone help me to understand the code and explain why we fork twice? Wouldn't it be sufficient to just fork once for the external filter since we cannot process anything in parallel anyways?\n\nIn 546bb5 Hannes refactored Junio's code to use the async infrastructure that is used today.\n\nThank you,\nLars"},{"id":"291737","messageId":"xmqqbn1th5qn.fsf@gitster.mtv.corp.google.com","threadId":"42879","inReplyTo":"67D9AC88-550E-4549-9AFD-2401B70B363B@gmail.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T17:56:00Z","receivedAt":"2016-07-19T17:56:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> a long time ago in aa4ed4 Junio introduced the external filter\n> driver definition. Since that time we fork the Git process and\n> then we fork again to run the external filter. This is probably a\n> super stupid question with an obvious answer... but can anyone\n> help me to understand the code and explain why we fork twice?\n\nIn \"git show aa4ed4\" you find this picture:\n\n       /*\n        * Create a pipeline to have the command filter the buffer's\n        * contents.\n        *\n        * (child --> cmd) --> us\n        */\n\nwhere \"child\" is a fork of the original Git process; it still has\nthe <src, size> buffered data inherited from the original process,\nso it can write_in_full() into the pipe going to the cmd process.\n\"us\" is the original process that has the reading end of the pipe\nthat connects us to the \"(child --> cmd)\" processes, so that it can\nread the filtered result from them.\n\nThe key benefit of this arrangement is the above can be done without\nhaving to do poll() to flip between reading and writing that is\nneeded to avoid deadlocking, which kept the code simpler.  A later\nconversion of the write side into async does not fundamentally\nchange anything from the original arrangement.\n\n\n\n\n\n\n"},{"id":"291749","messageId":"xmqqlh0xfoho.fsf@gitster.mtv.corp.google.com","threadId":"42879","inReplyTo":"xmqqbn1th5qn.fsf@gitster.mtv.corp.google.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T18:53:55Z","receivedAt":"2016-07-19T18:54:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The key benefit of this arrangement is the above can be done without\n> having to do poll() to flip between reading and writing that is\n> needed to avoid deadlocking, which kept the code simpler.  A later\n> conversion of the write side into async does not fundamentally\n> change anything from the original arrangement.\n\nTranslation: I was too lazy to worry about doing poll()/select()\nwhen I did it originally.  As long as you can do so correctly, be my\nguest to reduce one process by having the main process do both\nreading and writing.\n\n;-)\n"},{"id":"291767","messageId":"7B44C694-7CB2-411D-9CC6-7334CCBAD20B@gmail.com","threadId":"42879","inReplyTo":"xmqqlh0xfoho.fsf@gitster.mtv.corp.google.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-19T20:44:53Z","receivedAt":"2016-07-19T20:44:48Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 19 Jul 2016, at 20:53, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> The key benefit of this arrangement is the above can be done without\n>> having to do poll() to flip between reading and writing that is\n>> needed to avoid deadlocking, which kept the code simpler.  A later\n>> conversion of the write side into async does not fundamentally\n>> change anything from the original arrangement.\n> \n> Translation: I was too lazy to worry about doing poll()/select()\n> when I did it originally.  As long as you can do so correctly, be my\n> guest to reduce one process by having the main process do both\n> reading and writing.\n> \n> ;-)\n\nThanks a lot for this explanation. My goal is to add an option to\nthe clean/smudge filter config that instructs Git to keep the\nexternal filter process running. Whenever Git encounters a file\nto be filtered then it would continue to talk to the filter process\nover a simple pipe protocol:\n\nGit writes --> 4 byte filename length\nGit writes --> filename string\nGit writes --> 4 byte content length\nGit writes --> content string\nGit reads <-- 4 byte filtered content length\nGit reads <-- filtered content\n\nI still need to read more about poll()/select() but it looks to me\nas if these functions are only required if Git doesn't know what to\nexpect. With the sketched protocol above that wouldn't be the case.\nTherefore, I wonder if I would need to use poll()/select()?\n\nThanks,\nLars\n\n"},{"id":"291770","messageId":"xmqqr3ape2ju.fsf@gitster.mtv.corp.google.com","threadId":"42879","inReplyTo":"7B44C694-7CB2-411D-9CC6-7334CCBAD20B@gmail.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T21:33:09Z","receivedAt":"2016-07-19T21:33:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> Git writes --> 4 byte filename length\n> Git writes --> filename string\n\nWhy limit to 32GB?  Perhaps NUL termination is more appropriate\nhere?\n\n> Git writes --> 4 byte content length\n> Git writes --> content string\n> Git reads <-- 4 byte filtered content length\n> Git reads <-- filtered content\n\nDo you really need to force the sender to know the length in\nadvance?  Together with the sequential nature of the above exchange,\ni.e. the filter is forbidden from producing even a single byte of\nits output before reading everything Git feeds it, you are making it\nimpossible to use filters that perform streaming conversion.\n\nOf course, with the \"sequential\" thing, you do not have to worry\nabout deadlocking hence no need for poll/select, but I am not sure\nthat is a good thing.\n\n\n"},{"id":"291771","messageId":"BDD74384-AA02-4B60-A246-504B4C6DDF02@gmail.com","threadId":"42879","inReplyTo":"xmqqr3ape2ju.fsf@gitster.mtv.corp.google.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-19T22:01:40Z","receivedAt":"2016-07-19T22:01:53Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 19 Jul 2016, at 23:33, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Lars Schneider <larsxschneider@gmail.com> writes:\n> \n>> Git writes --> 4 byte filename length\n>> Git writes --> filename string\n> \n> Why limit to 32GB?  Perhaps NUL termination is more appropriate\n> here?\nOK, I will use NUL termination for the filename. \nYou're also right about the limit - I will use 8 byte to encode the \ncontent length.\n\n\n>> Git writes --> 4 byte content length\n>> Git writes --> content string\n>> Git reads <-- 4 byte filtered content length\n>> Git reads <-- filtered content\n> \n> Do you really need to force the sender to know the length in\n> advance?  Together with the sequential nature of the above exchange,\n> i.e. the filter is forbidden from producing even a single byte of\n> its output before reading everything Git feeds it, you are making it\n> impossible to use filters that perform streaming conversion.\nThat is correct. However, for my particular use case streaming\nconversion wouldn't be useful anyways:\nhttps://github.com/github/git-lfs/pull/1382\n\n\n> Of course, with the \"sequential\" thing, you do not have to worry\n> about deadlocking hence no need for poll/select, but I am not sure\n> that is a good thing.\nThanks for the confirmation. I consider to exchange a \"filter protocol \nversion\" right after the filter process has started. That way someone \ncould add a more evolved \"filter driver protocol\" later on that supports \nstreaming and the external filter could pick whatever protocol is most\nappropriate (and supported). Could that be an acceptable compromise\nto get a serious review of the \"sequential\" thing?\n\nThanks,\nLars\n\n\n"},{"id":"291784","messageId":"cdb8005a-2ed9-04da-fdbf-600accfac38f@web.de","threadId":"42879","inReplyTo":"BDD74384-AA02-4B60-A246-504B4C6DDF02@gmail.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-07-20T02:33:49Z","receivedAt":"2016-07-20T02:34:59Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 07/20/2016 12:01 AM, Lars Schneider wrote:\n>\n> On 19 Jul 2016, at 23:33, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Lars Schneider <larsxschneider@gmail.com> writes:\n>>\n>>> Git writes --> 4 byte filename length\n>>> Git writes --> filename string\n>>\n>> Why limit to 32GB?  Perhaps NUL termination is more appropriate\n>> here?\n> OK, I will use NUL termination for the filename.\n> You're also right about the limit - I will use 8 byte to encode the\n> content length.\nIs there any reason to encode the file length in binary format?\nWith all the discussions about big endianess, little endianess, 4GiB or \n32 GiB.\nHow about simply writing the length as ASCII ?\n\nUnless we don't want to have a \"spare\" field for future extensions,\nit could be good to add an option field, which may be empty.\nOn top of that, do we want a field separator different from the line\nseparator ?\n\nHow about this:\n<options><TAB><length><TAB><filename><NUL>\n\n<options> may be \"var=value;var2=value2\" or simply \"\"\n"},{"id":"291794","messageId":"578F3D5E.2070201@gmail.com","threadId":"42879","inReplyTo":"xmqqr3ape2ju.fsf@gitster.mtv.corp.google.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-07-20T08:59:10Z","receivedAt":"2016-07-20T08:59:24Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2016-07-19 o 23:33, Junio C Hamano pisze:\n> Lars Schneider <larsxschneider@gmail.com> writes:\n> \n>> > Git writes --> 4 byte filename length\n>> > Git writes --> filename string\n>\n> Why limit to 32GB?  Perhaps NUL termination is more appropriate\n> here?\n\nErrr, I think limiting _filename_ to 32GB is a reasonable\nassumption, for forever...\n \n>> > Git writes --> 4 byte content length\n>> > Git writes --> content string\n\n...while limiting file _content_ to 32GB might not be\nfuture-proof enough.\n\n;-)\n-- \nJakub Narębski\n"},{"id":"291795","messageId":"6CD48865-8D77-4582-AB41-FCD27D41017C@gmail.com","threadId":"42879","inReplyTo":"578F3D5E.2070201@gmail.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-07-20T09:43:51Z","receivedAt":"2016-07-20T09:43:57Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 20 Jul 2016, at 10:59, Jakub Narębski <jnareb@gmail.com> wrote:\n> \n> W dniu 2016-07-19 o 23:33, Junio C Hamano pisze:\n>> Lars Schneider <larsxschneider@gmail.com> writes:\n>> \n>>>> Git writes --> 4 byte filename length\n>>>> Git writes --> filename string\n>> \n>> Why limit to 32GB?  Perhaps NUL termination is more appropriate\n>> here?\n> \n> Errr, I think limiting _filename_ to 32GB is a reasonable\n> assumption, for forever...\n\nWell, Java packages can get reaaaally long :D\n\n> \n>>>> Git writes --> 4 byte content length\n>>>> Git writes --> content string\n> \n> ...while limiting file _content_ to 32GB might not be\n> future-proof enough.\n> \n> ;-)\n\nI think this is what Junio meant to say. At least I \ninterpreted it that way.\n\n\n- Lars"},{"id":"291809","messageId":"20160720134916.GB19359@sigill.intra.peff.net","threadId":"42879","inReplyTo":"xmqqr3ape2ju.fsf@gitster.mtv.corp.google.com","subject":"Re: Looking for help to understand external filter driver code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-20T13:49:16Z","receivedAt":"2016-07-20T13:49:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 19, 2016 at 02:33:09PM -0700, Junio C Hamano wrote:\n\n> > Git writes --> 4 byte content length\n> > Git writes --> content string\n> > Git reads <-- 4 byte filtered content length\n> > Git reads <-- filtered content\n> \n> Do you really need to force the sender to know the length in\n> advance?  Together with the sequential nature of the above exchange,\n> i.e. the filter is forbidden from producing even a single byte of\n> its output before reading everything Git feeds it, you are making it\n> impossible to use filters that perform streaming conversion.\n\nAnother option: use pkt-lines with a flush packet to indicate\nend-of-input. That allows arbitrary sized data, with streaming, and\nreuses existing concepts from git. There is proportional overhead, but\nit's only 4 bytes per 64k, which is a tiny percent.\n\nIt does make some implementations easier if they know the size ahead of\ntime, though, so if we are _sure_ that nobody will want streaming later,\nit may not be a good tradeoff. If we do print a size ahead of time, the\n\"normal\" thing in git would be to do so in base-10 ascii followed by a\nnewline (e.g., as found in \"cat-file --batch\", or fast-import's \"data\"\ncommand).\n\n-Peff\n"}]}