{"thread":{"id":"30209","subject":"[PATCH/RFC] remote-helpers: set up a pipe to read fast-import response","startedAt":"2012-04-11T14:32:49Z","lastAt":"2012-04-11T21:46:48Z","messageCount":7,"participants":["Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188965","messageId":"20120411143249.GA4140@burratino","threadId":"30209","inReplyTo":null,"subject":"[PATCH/RFC] remote-helpers: set up a pipe to read fast-import response","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-11T14:32:49Z","receivedAt":"2012-04-11T14:32:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Dmitry Ivankov \nDate: Tue, 21 Jun 2011 20:52:34 +0600\n\nRemote helpers may want to issue \"ls\" or \"cat-blob\" requests to\nfast-import. For example, the svn remote helper from the\nsvn-dump-fast-import project uses that capability.\n\nThe response should be fed back to remote helper; let it go to the\nremote helper's standard input (command stream).\n\nAfter listing the branches to import:\n\n\t'import' SP 'refs/heads/foo' LF\n\t'import' SP 'refs/heads/bar' LF\n\t'import' SP 'refs/heads/baz' LF\n\tLF\n\nthe transport-helper does not send any commands to the remote helper\nuntil the remote helper sends the \"done\" command to fast-import, so\nthis does not result in interleaved output from the transport-helper\nand fast-import.\n\nSigned-off-by: Dmitry Ivankov \nSigned-off-by: Jonathan Nieder \n---\nHi,\n\nSomething like this patch was sitting in my tree gathering moss but I\nquite like it so I thought I should send it out for feedback.\n\nI can't seem to find any copy of this patch in the list archives.\nMaybe I received it by private email. Dmitry, do you remember better?\n\nCan also be found at\n\n git://repo.or.cz/git/jrn.git topics/di/remote-helper-blob-access\n\nand will be part of the svn-fe-pu branch there in the next push.\n\nIt is possible (likely, even) that this doesn't work on Windows since\nfile descriptors are not inherited by default, but it won't cause harm\nuntil someone tries to use \"ls\" or \"cat-blob\" from a remote helper.\nIf someone wants to write tests for it, that would be very useful.\nOtherwise, we can deal with it once the remote helper hits the tree.\n\nChanges since v2:\n\n - new description\n - documented in remote-helpers manpage\n - don't waste so much space for fastimport->argv. (I restrained\n myself from using malloc in place of calloc since that change\n probably belongs in a separate patch.)\n\nThoughts? Bugs? Improvements?\n\nThanks,\nJonathan\n\n Documentation/git-remote-helpers.txt | 5 ++++-\n transport-helper.c | 6 +++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt\nindex 3a23477c..0cae14c7 100644\n--- a/Documentation/git-remote-helpers.txt\n+++ b/Documentation/git-remote-helpers.txt\n@@ -133,7 +133,10 @@ Supported if the helper has the \"push\" capability.\n \n 'import' ::\n \tProduces a fast-import stream which imports the current value\n-\tof the named ref. It may additionally import other refs as\n+\tof the named ref. Standard input for the helper is connected\n+\tto the fast-import backend's cat-blob stream so the helper\n+\tcan read responses from \"ls\" and \"cat-blob\" requests.\n+\tThe helper may additionally import other refs as\n \tneeded to construct the history efficiently. The script writes\n \tto a helper-specific private namespace. The value of the named\n \tref should be written to a location in this namespace derived\ndiff --git a/transport-helper.c b/transport-helper.c\nindex acfc88e3..4c547b9a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -349,12 +349,16 @@ static int fetch_with_fetch(struct transport *transport,\n \n static int get_importer(struct transport *transport, struct child_process *fastimport)\n {\n+\tchar buf[32];\n+\tstruct helper_data *data = transport->data;\n \tstruct child_process *helper = get_helper(transport);\n \tmemset(fastimport, 0, sizeof(*fastimport));\n \tfastimport->in = helper->out;\n-\tfastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n+\tfastimport->argv = xcalloc(4, sizeof(*fastimport->argv));\n \tfastimport->argv[0] = \"fast-import\";\n \tfastimport->argv[1] = \"--quiet\";\n+\tsnprintf(buf, 32, \"--cat-blob-fd=%d\", data->helper->in);\n+\tfastimport->argv[2] = buf;\n \n \tfastimport->git_cmd = 1;\n \treturn start_command(fastimport);\n-- \n1.7.10\n"},{"id":"188975","messageId":"7v1ununtb2.fsf@alter.siamese.dyndns.org","threadId":"30209","inReplyTo":"20120411143249.GA4140@burratino","subject":"Re: [PATCH/RFC] remote-helpers: set up a pipe to read fast-import response","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T16:10:09Z","receivedAt":"2012-04-11T16:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Something like this patch was sitting in my tree gathering moss but I\n> quite like it so I thought I should send it out for feedback.\n>\n> I can't seem to find any copy of this patch in the list archives.\n> Maybe I received it by private email. Dmitry, do you remember better?\n>\n> Can also be found at\n>\n> git://repo.or.cz/git/jrn.git topics/di/remote-helper-blob-access\n>\n> and will be part of the svn-fe-pu branch there in the next push.\n\nDoes this essentially connect the frontend and fast-import via\nbidirectional pipes? How is the flow control and deadlock avoidance\nsupposed to happen (I guess the side that wants to give a command to ask\nfor information needs to wait until it reads everything from the other\nside to drain the pipe before doing so), and isn't it something that need\nto be documented?\n"},{"id":"188998","messageId":"20120411171707.GD4248@burratino","threadId":"30209","inReplyTo":"7v1ununtb2.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC] fast-import doc: deadlock avoidance in bidirectional mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-11T17:17:07Z","receivedAt":"2012-04-11T17:17:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If fast-import's command pipe and the frontend's cat-blob/ls response\npipe are both filled, there can be a deadlock. Luckily all existing\nfrontends consume any pending cat-blob/ls responses completely before\nwriting the next command.\n\nDocument the requirements so future frontend authors and users can be\nspared from the problem, too. It is not always easy to catch that\nkind of bug by testing.\n\nReported-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n\n> Does this essentially connect the frontend and fast-import via\n> bidirectional pipes? How is the flow control and deadlock avoidance\n> supposed to happen\n\nfast-import never asks the frontend for information, which makes life a\nlittle simpler.\n\nTypically the interaction works just as you described:\n\n 1. Frontend sends \"ls\" or \"cat-blob\" request and flushes it, and then\n blocks waiting for the response.\n\n 2. Once fast-import catches up with pending commands, it sends its\n response to the \"cat-blob response\" pipe.\n\n (If the pipe does not have enough room, it fills the pipe and then\n blocks until some more room is available. If the reader has\n closed the pipe, it assumes the import was botched and just exits.)\n\n 3. Only once the frontend has received its response, it moves on and\n starts to send new commands.\n\nA frontend can be more clever than that and use slack to queue some\nextra commands, as long as the author understands:\n\n i. fast-import is not guaranteed to make any progress in consuming\n its input until the \"cat-blob response\" pipe has been drained.\n\n ii. the command pipe is not guaranteed to be able to hold extra\n commands when fast-import is not consuming its input. Probably\n 512 bytes (_POSIX_PIPE_BUF) will fit but I don't know how\n portable that is to other fast-import backends so let's say the\n limit is 1 byte.\n\nIn other words, in practice it's best not to rely on the extra space\nat all.\n\nHow about this?\n\n Documentation/git-fast-import.txt | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex ec6ef311..0ea649f4 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -942,6 +942,12 @@ This command can be used anywhere in the stream that comments are\n accepted. In particular, the `cat-blob` command can be used in the\n middle of a commit but not in the middle of a `data` command.\n \n+While in some cases the 'cat-blob' result will fit in the pipe buffer,\n+allowing fast-import to continue processing additional commands, this\n+is not guaranteed. Frontends must consume the cat-blob response\n+completely before performing any writes to fast-import that might\n+block.\n+\n `ls`\n ~~~~\n Prints information about the object at a path to a file descriptor\n@@ -975,7 +981,12 @@ Reading from a named tree::\n \n See `filemodify` above for a detailed description of ``.\n \n-Output uses the same format as `git ls-tree {litdd} `:\n+While in some cases the 'ls' response will fit in the pipe buffer,\n+allowing fast-import to continue processing additional commands, this\n+is not guaranteed. Frontends must consume the ls response completely\n+before performing any writes to fast-import that might block.\n+\n+The 'ls' response uses the same format as `git ls-tree {litdd} `:\n \n ====\n \t SP ('blob' | 'tree' | 'commit') SP HT LF\n-- \n1.7.10\n"},{"id":"189001","messageId":"7vlim2kwcv.fsf@alter.siamese.dyndns.org","threadId":"30209","inReplyTo":"20120411171707.GD4248@burratino","subject":"Re: [PATCH/RFC] fast-import doc: deadlock avoidance in bidirectional mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T17:32:32Z","receivedAt":"2012-04-11T17:32:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> How about this?\n\nI find all the explanation above this line (elided) understandable and\nsensible.\n\nThe expected use of --cat-blob-fd is to loop it back to the frontend, and\nthat is obvious to _us_, but there is nothing in the existing text (or the\ntext added by this patch) that suggests that fact to the first time\nreader. \"Frontends must consume\" may not \"click\" without that knowledge.\n\n> Documentation/git-fast-import.txt | 13 ++++++++++++-\n> 1 file changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\n> index ec6ef311..0ea649f4 100644\n> --- a/Documentation/git-fast-import.txt\n> +++ b/Documentation/git-fast-import.txt\n> @@ -942,6 +942,12 @@ This command can be used anywhere in the stream that comments are\n> accepted. In particular, the `cat-blob` command can be used in the\n> middle of a commit but not in the middle of a `data` command.\n> \n> +While in some cases the 'cat-blob' result will fit in the pipe buffer,\n> +allowing fast-import to continue processing additional commands, this\n> +is not guaranteed. Frontends must consume the cat-blob response\n> +completely before performing any writes to fast-import that might\n> +block.\n> +\n> `ls`\n> ~~~~\n> Prints information about the object at a path to a file descriptor\n> @@ -975,7 +981,12 @@ Reading from a named tree::\n> \n> See `filemodify` above for a detailed description of ``.\n> \n> -Output uses the same format as `git ls-tree {litdd} `:\n> +While in some cases the 'ls' response will fit in the pipe buffer,\n> +allowing fast-import to continue processing additional commands, this\n> +is not guaranteed. Frontends must consume the ls response completely\n> +before performing any writes to fast-import that might block.\n> +\n> +The 'ls' response uses the same format as `git ls-tree {litdd} `:\n> \n> ====\n> \t SP ('blob' | 'tree' | 'commit') SP HT LF\n"},{"id":"189043","messageId":"20120411212501.GH4248@burratino","threadId":"30209","inReplyTo":"7vlim2kwcv.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC v2] fast-import doc: cat-blob and ls responses need to be consumed quickly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-11T21:25:01Z","receivedAt":"2012-04-11T21:25:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If fast-import's command pipe and the frontend's cat-blob/ls response\npipe are both filled, there can be a deadlock. Luckily all existing\nfrontends consume any pending cat-blob/ls responses completely before\nwriting the next command.\n\nDocument the requirements so future frontend authors and users can be\nspared from the problem, too. It is not always easy to catch that\nkind of bug by testing.\n\nTo set the scene, add some words of explanation to help the novice\nunderstand that \"cat-blob\" and \"ls\" output are meant for consumption\nby the frontend.\n\nReported-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n\n> The expected use of --cat-blob-fd is to loop it back to the frontend, and\n> that is obvious to _us_, but there is nothing in the existing text (or the\n> text added by this patch) that suggests that fact to the first time\n> reader. \"Frontends must consume\" may not \"click\" without that knowledge.\n\nTrue. Here's a rough attempt (again against \"master\").\n\n Documentation/git-fast-import.txt | 44 +++++++++++++++++++++++++++++++++----\n 1 file changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex ec6ef311..7b220c28 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -98,9 +98,10 @@ OPTIONS\n \toptions.\n \n --cat-blob-fd=::\n-\tSpecify the file descriptor that will be written to\n-\twhen the `cat-blob` command is encountered in the stream.\n-\tThe default behaviour is to write to `stdout`.\n+\tWrite responses to `cat-blob` and `ls` queries to the\n+\tfile descriptor instead of `stdout`. Allows `progress`\n+\toutput intended for the end-user to be separated from other\n+\toutput.\n \n --done::\n \tRequire a `done` command at the end of the stream.\n@@ -942,6 +943,9 @@ This command can be used anywhere in the stream that comments are\n accepted. In particular, the `cat-blob` command can be used in the\n middle of a commit but not in the middle of a `data` command.\n \n+See ``Responses To Commands'' below for details about how to read\n+this output safely.\n+\n `ls`\n ~~~~\n Prints information about the object at a path to a file descriptor\n@@ -991,6 +995,9 @@ instead report\n \tmissing SP LF\n ====\n \n+See ``Responses To Commands'' below for details about how to read\n+this output safely.\n+\n `feature`\n ~~~~~~~~~\n Require that fast-import supports the specified feature, or abort if\n@@ -1079,6 +1086,35 @@ If the `--done` command line option or `feature done` command is\n in use, the `done` command is mandatory and marks the end of the\n stream.\n \n+Responses To Commands\n+---------------------\n+New objects written by fast-import are not available immediately.\n+Most fast-import commands have no visible effect until the next\n+checkpoint (or completion). The frontend can send commands to\n+fill fast-import's input pipe without worrying about how quickly\n+they will take effect, which improves performance by simplifying\n+scheduling.\n+\n+For some frontends, though, it is useful to be able to read back\n+data from the current repository as it is being updated (for\n+example when the source material describes objects in terms of\n+patches to be applied to previously imported objects). This can\n+be accomplished by connecting the frontend and fast-import via\n+bidirectional pipes:\n+\n+====\n+\tmkfifo fast-import-output\n+\tfrontend fast-import-output\n+====\n+\n+A frontend set up this way can use `progress`, `ls`, and `cat-blob`\n+commands to read information from the import in progress.\n+\n+To avoid deadlock, such frontends must completely consume any\n+pending output from `progress`, `ls`, and `cat-blob` before\n+performing writes to fast-import that might block.\n+\n Crash Reports\n -------------\n If fast-import is supplied invalid input it will terminate with a\n-- \n1.7.10\n"},{"id":"189044","messageId":"7vobqyhs43.fsf@alter.siamese.dyndns.org","threadId":"30209","inReplyTo":"20120411212501.GH4248@burratino","subject":"Re: [PATCH/RFC v2] fast-import doc: cat-blob and ls responses need to be consumed quickly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-11T21:32:28Z","receivedAt":"2012-04-11T21:32:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> True. Here's a rough attempt (again against \"master\").\n\nLooks good. Do you want me to queue it, or would it be of not much use\noutside the context of the other patch that actually does create the\nloop-back?\n"},{"id":"189047","messageId":"20120411214648.GI4248@burratino","threadId":"30209","inReplyTo":"7vobqyhs43.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC v2] fast-import doc: cat-blob and ls responses need to be consumed quickly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-11T21:46:48Z","receivedAt":"2012-04-11T21:46:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Looks good. Do you want me to queue it, or would it be of not much use\n> outside the context of the other patch that actually does create the\n> loop-back?\n\nThanks. It's useful for git versions v1.7.9.5~1^2~2 (\"fast-import:\nadd 'ls' command\") and newer. I've pushed a copy of the patch to\n\n git://repo.or.cz/git/jrn.git fast-import\n"}]}