{"thread":{"id":"24898","subject":"[PATCH 05/13] transport-helper: use the new done feature to properly do imports","startedAt":"2010-08-29T03:45:27Z","lastAt":"2011-02-13T09:42:12Z","messageCount":52,"participants":["Sverre Rabbelier","Daniel Barkalow","Jonathan Nieder","Tay Ray Chuan"],"isPatch":true,"patchVersion":1,"patchTotal":13},"messages":[{"id":"149189","messageId":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":null,"subject":"[PATCH 00/13] remote helper improvements","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:27Z","receivedAt":"2010-08-29T03:45:27Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"I had a week and then some stray days here and there to do some more\nwork on git-remote-hg, the result of which is this series. It takes\nthe 'import' and 'export' commands out of their 'toy' stage, and gets\nthem ready for real usage. Although 'git-remote-testgit' is still the\nonly thing using them, 'git-remote-hg' is nearing completion, I hope\nto send out an RFC for it Real Soon Now (TM).\n\nSverre Rabbelier (13):\n fast-import: add the 'done' command\n fast-export: support done feature\n\nThese two are very important to the rest of the series, most of the\nclean up relies on the 'done' command to make 'import/export' part of\nthe remote helper protocol not suck.\n\n transport-helper: check status code of finish_command\n remote-curl: accept empty line as terminator\n\nIf nothing else is applied, these two should be taken out together\nand applied separately.\n\n transport-helper: factor out push_update_refs_status\n transport-helper: update ref status after push with export\n\nThis is not very fleshed out yet, (the second patch in particular),\nbut without this 'git push' to a remote that uses the 'export'\ncapability will always say 'everything up-to-date'.\n\n transport-helper: use the new done feature to properly do imports\n transport-helper: export should disconnect too\n\nThese two make the 'import' and 'export' command re-entrant. That is,\nnow the remote helper infrastructure could issue other commands after\nissuing an 'import' or 'export' command.\n\n transport-helper: change import semantics\n\nThis is another cleanup to the protocol, without this it is more or\nless impossible to import multiple refs.\n\n transport-helper: Use capname for gitdir capability too\n\nThis is a candidate for for maint, the current implementation is just\nplain wrong.\n\n transport-helper: implement marks location as capability\n\nAnother protocol cleanup.\n\n git-remote-testgit: only push for non-local repositories\n git-remote-testgit: fix error handling\n\nBoth of these are maint candidates, they are bugfixes.\n\n Documentation/git-fast-export.txt | 4 ++\n Documentation/git-fast-import.txt | 17 ++++++-\n builtin/fast-export.c | 9 +++\n fast-import.c | 5 ++\n git-remote-testgit.py | 50 +++++++++++++------\n git_remote_helpers/git/importer.py | 5 +-\n remote-curl.c | 3 +\n transport-helper.c | 97 +++++++++++++++++++----------------\n 8 files changed, 127 insertions(+), 63 deletions(-)\n"},{"id":"149190","messageId":"1283053540-27042-2-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 01/13] fast-import: add the 'done' command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:28Z","receivedAt":"2010-08-29T03:45:28Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Currently the only way to end an import stream is to close it, which\nis not desirable when the stream that's being used is shared. For\nexample, the remote helper infrastructure uses a pipe between it and\nthe helper process, part of the protocol is to send a fast-import\nstream accross. Without a way to end the stream the remote helper\ninfrastructure is forced to limit itself to have a command that uses\na fast-import stream as it's last command.\n\nAdd a trivial 'done' command that causes fast-import to stop reading\nfrom the stream and exit.\n---\n\n Very straightforward. It is handled in parse_feature() instead of\n in parse_one_feature() because I didn't want to allow '--done' as a\n commandline argument. Allowing it would be silly, it surves no\n other purpose than to indicate up front that the stream will\n contain a 'done' command at the end.\n\n I'm fine too with dropping the feature and just adding the new\n command, whichever is preferred.\n\n Documentation/git-fast-import.txt | 17 ++++++++++++++++-\n fast-import.c | 5 +++++\n 2 files changed, 21 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 77a0a24..114f919 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -293,6 +293,10 @@ and control the current import process. More detailed discussion\n \tcreating a new commit and updating the branch to point at\n \tthe newly created commit.\n \n+`done`::\n+\tTreated as if EOF was read. This command is optional and is\n+\tnot needed to perform an import.\n+\n `tag`::\n \tCreates an annotated tag object from an existing commit or\n \tbranch. Lightweight tags are not supported by this command,\n@@ -885,17 +889,20 @@ The part of the command may be any string matching\n ^[a-zA-Z][a-zA-Z-]*$ and should be understood by fast-import.\n \n Feature work identical as their option counterparts with the\n-exception of the import-marks feature, see below.\n+exception of the done and import-marks features, see below.\n \n The following features are currently supported:\n \n * date-format\n+* done\n * import-marks\n * export-marks\n * relative-marks\n * no-relative-marks\n * force\n \n+If the done feature is specified, the done command must be supported.\n+\n The import-marks behaves differently from when it is specified as\n commandline option in that only one \"feature import-marks\" is allowed\n per stream. Also, any --import-marks= specified on the commandline\n@@ -928,6 +935,14 @@ not be passed as option:\n * export-marks\n * force\n \n+`done`\n+~~~~~~\n+\n+Treated as if EOF was read. This can be used to stop fast-import\n+from reading from the stream without closing the file handle. Such\n+may be desired if the file handle is used for other purposes other\n+than fast-import as well, and closing it is not desired.\n+\n Crash Reports\n -------------\n If fast-import is supplied invalid input it will terminate with a\ndiff --git a/fast-import.c b/fast-import.c\nindex ddad289..1c3fa7d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2817,6 +2817,9 @@ static void parse_feature(void)\n \tif (parse_one_feature(feature, 1))\n \t\treturn;\n \n+\tif (!prefixcmp(feature, \"done\"))\n+\t\treturn;\n+\n \tdie(\"This version of fast-import does not support feature %s.\", feature);\n }\n \n@@ -2935,6 +2938,8 @@ int main(int argc, const char **argv)\n \t\t\tparse_new_blob();\n \t\telse if (!prefixcmp(command_buf.buf, \"commit \"))\n \t\t\tparse_new_commit();\n+\t\telse if (!prefixcmp(command_buf.buf, \"done\"))\n+\t\t\tbreak;\n \t\telse if (!prefixcmp(command_buf.buf, \"tag \"))\n \t\t\tparse_new_tag();\n \t\telse if (!prefixcmp(command_buf.buf, \"reset \"))\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149196","messageId":"1283053540-27042-3-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 02/13] fast-export: support done feature","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:29Z","receivedAt":"2010-08-29T03:45:29Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"If fast-export is being used to generate a fast-import stream that\nwill be used afterwards it is desirable to indicate the end of the\nstream with the new 'done' command.\n\nAdd a flag that causes fast-export to end with 'done'.\n---\n\n Also very trivial, obviously if the corresponding feature is\n removed the flag should be named differently.\n\n Documentation/git-fast-export.txt | 4 ++++\n builtin/fast-export.c | 9 +++++++++\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt\nindex 98ec6b5..912562e 100644\n--- a/Documentation/git-fast-export.txt\n+++ b/Documentation/git-fast-export.txt\n@@ -82,6 +82,10 @@ marks the same across runs.\n \tallow that. So fake a tagger to be able to fast-import the\n \toutput.\n \n+--use-done-feature::\n+\tStart the stream with a 'feature done' stanza, and terminate\n+\tit with a 'done' command.\n+\n --no-data::\n \tSkip output of blob objects and instead refer to blobs via\n \ttheir original SHA-1 hash. This is useful when rewriting the\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 9fe25ff..0c39c2e 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -26,6 +26,7 @@ static int progress;\n static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;\n static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;\n static int fake_missing_tagger;\n+static int use_done_feature;\n static int no_data;\n \n static int parse_opt_signed_tag_mode(const struct option *opt,\n@@ -584,6 +585,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \t\t\t \"Import marks from this file\"),\n \t\tOPT_BOOLEAN(0, \"fake-missing-tagger\", &fake_missing_tagger,\n \t\t\t \"Fake a tagger when tags lack one\"),\n+\t\tOPT_BOOLEAN(0, \"use-done-feature\", &use_done_feature,\n+\t\t\t \"Use the done feature to terminate the stream\"),\n \t\t{ OPTION_NEGBIT, 0, \"data\", &no_data, NULL,\n \t\t\t\"Skip output of blob data\",\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },\n@@ -605,6 +608,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (argc > 1)\n \t\tusage_with_options (fast_export_usage, options);\n \n+\tif (use_done_feature)\n+\t\tprintf(\"feature done\\n\");\n+\n \tif (import_filename)\n \t\timport_marks(import_filename);\n \n@@ -629,5 +635,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \tif (export_filename)\n \t\texport_marks(export_filename);\n \n+\tif (use_done_feature)\n+\t\tprintf(\"done\\n\");\n+\n \treturn 0;\n }\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149193","messageId":"1283053540-27042-4-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 03/13] transport-helper: factor out push_update_refs_status","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:30Z","receivedAt":"2010-08-29T03:45:30Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"The update ref status part of push is useful for the export command\nas well, factor it out into it's own function.\n---\n\n I didn't move the new function up above push_refs_with_push so that\n it is obvious to the reviewer that the change is trivial.\n\n transport-helper.c | 16 ++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 191fbf7..9f2ad00 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -554,6 +554,9 @@ static int fetch(struct transport *transport,\n \treturn -1;\n }\n \n+static void push_update_refs_status(struct helper_data *data,\n+\t\t\t\t struct ref *remote_refs);\n+\n static int push_refs_with_push(struct transport *transport,\n \t\tstruct ref *remote_refs, int flags)\n {\n@@ -609,8 +612,17 @@ static int push_refs_with_push(struct transport *transport,\n \n \tstrbuf_addch(&buf, '\\n');\n \tsendline(data, &buf);\n+\tstrbuf_release(&buf);\n+\n+\tpush_update_refs_status(data, remote_refs);\n+\treturn 0;\n+}\n \n-\tref = remote_refs;\n+static void push_update_refs_status(struct helper_data *data,\n+\t\t\t\t struct ref *remote_refs)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct ref *ref = remote_refs;\n \twhile (1) {\n \t\tchar *refname, *msg;\n \t\tint status;\n@@ -679,7 +691,7 @@ static int push_refs_with_push(struct transport *transport,\n \t\tref->remote_status = msg;\n \t}\n \tstrbuf_release(&buf);\n-\treturn 0;\n+\treturn;\n }\n \n static int push_refs_with_export(struct transport *transport,\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149191","messageId":"1283053540-27042-5-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 04/13] transport-helper: check status code of finish_command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:31Z","receivedAt":"2010-08-29T03:45:31Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Previously the status code of all helpers were ignored, allowing\nerrors that occur to go unnoticed if the error text output by the\nhelper is not noticed (or was not present at all).\n---\n\n I'm surprised nobody fixed this sooner.\n\n transport-helper.c | 23 +++++++++++++++--------\n 1 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 9f2ad00..4a2826d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -204,6 +204,7 @@ static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tint res = 0;\n \n \tif (data->helper) {\n \t\tif (debug)\n@@ -215,13 +216,13 @@ static int disconnect_helper(struct transport *transport)\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n-\t\tfinish_command(data->helper);\n+\t\tres = finish_command(data->helper);\n \t\tfree((char *)data->helper->argv[0]);\n \t\tfree(data->helper->argv);\n \t\tfree(data->helper);\n \t\tdata->helper = NULL;\n \t}\n-\treturn 0;\n+\treturn res;\n }\n \n static const char *unsupported_options[] = {\n@@ -299,12 +300,13 @@ static void standard_options(struct transport *t)\n \n static int release_helper(struct transport *transport)\n {\n+\tint res = 0;\n \tstruct helper_data *data = transport->data;\n \tfree_refspec(data->refspec_nr, data->refspecs);\n \tdata->refspecs = NULL;\n-\tdisconnect_helper(transport);\n+\tres = disconnect_helper(transport);\n \tfree(transport->data);\n-\treturn 0;\n+\treturn res;\n }\n \n static int fetch_with_fetch(struct transport *transport,\n@@ -410,8 +412,11 @@ static int fetch_with_import(struct transport *transport,\n \t\tsendline(data, &buf);\n \t\tstrbuf_reset(&buf);\n \t}\n-\tdisconnect_helper(transport);\n-\tfinish_command(&fastimport);\n+\tif(disconnect_helper(transport))\n+\t\tdie(\"Error while disconnecting helper\");\n+\tif (finish_command(&fastimport))\n+\t\tdie(\"Error while running fast-import\");\n+\n \tfree(fastimport.argv);\n \tfastimport.argv = NULL;\n \n@@ -751,8 +756,10 @@ static int push_refs_with_export(struct transport *transport,\n \t\tdie(\"Couldn't run fast-export\");\n \n \tdata->no_disconnect_req = 1;\n-\tfinish_command(&exporter);\n-\tdisconnect_helper(transport);\n+\tif(finish_command(&exporter))\n+\t\tdie(\"Error while running fast-export\");\n+\tif(disconnect_helper(transport))\n+\t\tdie(\"Error while disconnecting helper\");\n \treturn 0;\n }\n \n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149188","messageId":"1283053540-27042-6-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 05/13] transport-helper: use the new done feature to properly do imports","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:32Z","receivedAt":"2010-08-29T03:45:32Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Previously, the helper code would disconnect the helper before\nstarting fast-import. This was needed because there was no way to signal\nthat the helper was done other than to close stdout (which it would\ndo after importing iff the helper noticed it had been disconnected).\n\nInstead, request that the fast-export uses the 'done' command to\nsignal when it is done exporting, so that we can disconnect the\nhelper at a time of our choosing.\n---\n\n I really like what this does for the sanity of the import\n implementation, it makes it much more like a regular (re-entrant)\n command, rather than the \"sorry, you're done now\" way it is now.\n\n git-remote-testgit.py | 2 ++\n transport-helper.c | 8 ++------\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex df9d512..612cb5a 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -124,6 +124,8 @@ def do_import(repo, args):\n repo = update_local_repo(repo)\n repo.exporter.export_repo(repo.gitdir)\n \n+ print \"done\"\n+\n \n def do_export(repo, args):\n \"\"\"Imports a fast-import stream from git to testgit.\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4a2826d..5647595 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -375,8 +375,9 @@ static int get_exporter(struct transport *transport,\n \t/* we need to duplicate helper->in because we want to use it after\n \t * fastexport is done with it. */\n \tfastexport->out = dup(helper->in);\n-\tfastexport->argv = xcalloc(4 + revlist_args->nr, sizeof(*fastexport->argv));\n+\tfastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));\n \tfastexport->argv[argc++] = \"fast-export\";\n+\tfastexport->argv[argc++] = \"--use-done-feature\";\n \tif (export_marks)\n \t\tfastexport->argv[argc++] = export_marks;\n \tif (import_marks)\n@@ -412,11 +413,8 @@ static int fetch_with_import(struct transport *transport,\n \t\tsendline(data, &buf);\n \t\tstrbuf_reset(&buf);\n \t}\n-\tif(disconnect_helper(transport))\n-\t\tdie(\"Error while disconnecting helper\");\n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n-\n \tfree(fastimport.argv);\n \tfastimport.argv = NULL;\n \n@@ -758,8 +756,6 @@ static int push_refs_with_export(struct transport *transport,\n \tdata->no_disconnect_req = 1;\n \tif(finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n-\tif(disconnect_helper(transport))\n-\t\tdie(\"Error while disconnecting helper\");\n \treturn 0;\n }\n \n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149197","messageId":"1283053540-27042-7-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[RFC PATCH 06/13] transport-helper: update ref status after push with export","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:33Z","receivedAt":"2010-08-29T03:45:33Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"---\n\n Obviously the testgit helper shouldn't just print 'ok' for master,\n but it demonstrates the idea.\n\n git-remote-testgit.py | 3 +++\n transport-helper.c | 1 +\n 2 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 612cb5a..342a05d 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -151,6 +151,9 @@ def do_export(repo, args):\n repo.importer.do_import(repo.gitdir)\n repo.non_local.push(repo.gitdir)\n \n+ print \"ok refs/heads/master\"\n+ print\n+\n \n def do_gitdir(repo, args):\n \"\"\"Stores the location of the gitdir.\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 5647595..ecaea25 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -756,6 +756,7 @@ static int push_refs_with_export(struct transport *transport,\n \tdata->no_disconnect_req = 1;\n \tif(finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n+\tpush_update_refs_status(data, remote_refs);\n \treturn 0;\n }\n \n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149194","messageId":"1283053540-27042-8-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 07/13] transport-helper: change import semantics","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:34Z","receivedAt":"2010-08-29T03:45:34Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Currently the helper must somehow guess how many import statements to\nread before it starts outputting its fast-export stream. This is\nbecause the remote helper infrastructure runs fast-import only once,\nso the helper is forced to output one stream for all import commands\nit will receive. The only reason this worked in the past was because\nonly one ref was imported at a time.\n\nChange the semantics of the import statement such that it matches\nthat of the list statement. That is, 'import\\n' is followed by a list\nof refs that should be exported, followed by '\\n'.\n---\n\n This changes the protcol a bit, but I don't think we have many\n users of the 'import' command yet, and if we do I would assume\n they're paying attention to development in the remote helper space.\n\n git-remote-testgit.py | 12 ++++++++++--\n transport-helper.c | 7 ++++++-\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 342a05d..50341ce 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -115,12 +115,20 @@ def do_import(repo, args):\n \"\"\"Exports a fast-import stream from testgit for git to import.\n \"\"\"\n \n- if len(args) != 1:\n- die(\"Import needs exactly one ref\")\n+ if args:\n+ die(\"Import expects its ref seperately\")\n \n if not repo.gitdir:\n die(\"Need gitdir to import\")\n \n+ refs = []\n+\n+ while True:\n+ line = sys.stdin.readline()\n+ if line == '\\n':\n+ break\n+ refs.append(line.strip())\n+\n repo = update_local_repo(repo)\n repo.exporter.export_repo(repo.gitdir)\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex ecaea25..13ebb3b 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -404,15 +404,20 @@ static int fetch_with_import(struct transport *transport,\n \tif (get_importer(transport, &fastimport))\n \t\tdie(\"Couldn't run fast-import\");\n \n+\twrite_constant(data->helper->in, \"import\\n\");\n+\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tposn = to_fetch[i];\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n \t\t\tcontinue;\n \n-\t\tstrbuf_addf(&buf, \"import %s\\n\", posn->name);\n+\t\tstrbuf_addf(&buf, \"%s\\n\", posn->name);\n \t\tsendline(data, &buf);\n \t\tstrbuf_reset(&buf);\n \t}\n+\n+\twrite_constant(data->helper->in, \"\\n\");\n+\n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n \tfree(fastimport.argv);\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149199","messageId":"1283053540-27042-9-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 08/13] transport-helper: export should disconnect too","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:35Z","receivedAt":"2010-08-29T03:45:35Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Now that the remote helper protocol uses the new done command in its\nfast-import streams, export no longer needs to be the last command in\nthe stream.\n---\n\n The fact that we had this before shows how messed up the protocol\n was earlier. Basically, any 'import' or 'export' command meant\n \"you're done talking to the helper now\".\n\n transport-helper.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 13ebb3b..1294d10 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -758,7 +758,6 @@ static int push_refs_with_export(struct transport *transport,\n \t\t\t export_marks, import_marks, &revlist_args))\n \t\tdie(\"Couldn't run fast-export\");\n \n-\tdata->no_disconnect_req = 1;\n \tif(finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n \tpush_update_refs_status(data, remote_refs);\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149192","messageId":"1283053540-27042-10-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 09/13] transport-helper: Use capname for gitdir capability too","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:36Z","receivedAt":"2010-08-29T03:45:36Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Also properly use capname in the refspec capability.\n\nPreviously the gitdir and refspec capabilities could not be listed as\nrequired or their parsing would break.\n\nCC: Ilari Liusvaara \nCC: Daniel Barkalow \n---\n\n The first hunk was real silly and I should have caught it while\n reviewing the patch that introduced the required capabilities.\n\n I suspect the reason the second hunk wasn't caught is because the\n series that added 'gitdir' as capability, and the one that added\n required capabilities were done in parallel.\n\n transport-helper.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 1294d10..82bdad3 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -171,10 +171,10 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tALLOC_GROW(refspecs,\n \t\t\t\t refspec_nr + 1,\n \t\t\t\t refspec_alloc);\n-\t\t\trefspecs[refspec_nr++] = strdup(buf.buf + strlen(\"refspec \"));\n+\t\t\trefspecs[refspec_nr++] = strdup(capname + strlen(\"refspec \"));\n \t\t} else if (!strcmp(capname, \"connect\")) {\n \t\t\tdata->connect = 1;\n-\t\t} else if (!strcmp(buf.buf, \"gitdir\")) {\n+\t\t} else if (!strcmp(capname, \"gitdir\")) {\n \t\t\tstruct strbuf gitdir = STRBUF_INIT;\n \t\t\tstrbuf_addf(&gitdir, \"gitdir %s\\n\", get_git_dir());\n \t\t\tsendline(data, &gitdir);\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149201","messageId":"1283053540-27042-11-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 10/13] transport-helper: implement marks location as capability","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:37Z","receivedAt":"2010-08-29T03:45:37Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"While this requires the helper to flush stdout after listing 'gitdir'\nas capability, and read a command (the 'gitdir' response from the\nremote helper infrastructure) right after that, this is more elegant\nand does not require an ad-hoc exchange of values.\n\nCC: Daniel Barkalow \n---\n\n Daniel made some fuss about the ad-hoc exchange when I first sent\n the 'export command' series for review, and it's been nagging me.\n\n As you can see in the remote-testgit implementation, it's a bit\n icky on the helper side (you have to flush sdout and read another\n command in the middle of responding to 'capabilities'), but I think\n it's better than the alternative.\n\n git-remote-testgit.py | 29 ++++++++++++++++-------------\n transport-helper.c | 47 ++++++++++++++++++-----------------------------\n 2 files changed, 34 insertions(+), 42 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 50341ce..e2b213d 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -71,8 +71,24 @@ def do_capabilities(repo, args):\n print \"import\"\n print \"export\"\n print \"gitdir\"\n+\n+ sys.stdout.flush()\n+ if not read_one_line(repo):\n+ die(\"Expected gitdir, got empty line\")\n+\n print \"refspec refs/heads/*:%s*\" % repo.prefix\n \n+ dirname = repo.get_base_path(repo.gitdir)\n+\n+ if not os.path.exists(dirname):\n+ os.makedirs(dirname)\n+\n+ path = os.path.join(dirname, 'testgit.marks')\n+\n+ print \"*export-marks %s\" % path\n+ if os.path.exists(path):\n+ print \"*import-marks %s\" % path\n+\n print # end capabilities\n \n \n@@ -142,19 +158,6 @@ def do_export(repo, args):\n if not repo.gitdir:\n die(\"Need gitdir to export\")\n \n- dirname = repo.get_base_path(repo.gitdir)\n-\n- if not os.path.exists(dirname):\n- os.makedirs(dirname)\n-\n- path = os.path.join(dirname, 'testgit.marks')\n- print path\n- if os.path.exists(path):\n- print path\n- else:\n- print \"\"\n- sys.stdout.flush()\n-\n update_local_repo(repo)\n repo.importer.do_import(repo.gitdir)\n repo.non_local.push(repo.gitdir)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 82bdad3..0edc1d5 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -23,6 +23,8 @@ struct helper_data\n \t\tpush : 1,\n \t\tconnect : 1,\n \t\tno_disconnect_req : 1;\n+\tchar *export_marks;\n+\tchar *import_marks;\n \t/* These go from remote name (as in \"list\") to private name */\n \tstruct refspec *refspecs;\n \tint refspec_nr;\n@@ -179,6 +181,16 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tstrbuf_addf(&gitdir, \"gitdir %s\\n\", get_git_dir());\n \t\t\tsendline(data, &gitdir);\n \t\t\tstrbuf_release(&gitdir);\n+\t\t} else if (!prefixcmp(capname, \"export-marks \")) {\n+\t\t\tstruct strbuf arg = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n+\t\t\tstrbuf_addstr(&arg, capname + strlen(\"export-marks \"));\n+\t\t\tdata->export_marks = strbuf_detach(&arg, NULL);\n+\t\t} else if (!prefixcmp(capname, \"import-marks\")) {\n+\t\t\tstruct strbuf arg = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n+\t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n+\t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n \t\t} else if (mandatory) {\n \t\t\tdie(\"Unknown mandatory capability %s. This remote \"\n \t\t\t \"helper probably needs newer version of Git.\\n\",\n@@ -364,10 +376,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti\n \n static int get_exporter(struct transport *transport,\n \t\t\tstruct child_process *fastexport,\n-\t\t\tconst char *export_marks,\n-\t\t\tconst char *import_marks,\n \t\t\tstruct string_list *revlist_args)\n {\n+\tstruct helper_data *data = transport->data;\n \tstruct child_process *helper = get_helper(transport);\n \tint argc = 0, i;\n \tmemset(fastexport, 0, sizeof(*fastexport));\n@@ -378,10 +389,10 @@ static int get_exporter(struct transport *transport,\n \tfastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));\n \tfastexport->argv[argc++] = \"fast-export\";\n \tfastexport->argv[argc++] = \"--use-done-feature\";\n-\tif (export_marks)\n-\t\tfastexport->argv[argc++] = export_marks;\n-\tif (import_marks)\n-\t\tfastexport->argv[argc++] = import_marks;\n+\tif (data->export_marks)\n+\t\tfastexport->argv[argc++] = data->export_marks;\n+\tif (data->import_marks)\n+\t\tfastexport->argv[argc++] = data->import_marks;\n \n \tfor (i = 0; i < revlist_args->nr; i++)\n \t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n@@ -708,7 +719,6 @@ static int push_refs_with_export(struct transport *transport,\n \tstruct ref *ref;\n \tstruct child_process *helper, exporter;\n \tstruct helper_data *data = transport->data;\n-\tchar *export_marks = NULL, *import_marks = NULL;\n \tstruct string_list revlist_args = { NULL, 0, 0 };\n \tstruct strbuf buf = STRBUF_INIT;\n \n@@ -716,26 +726,6 @@ static int push_refs_with_export(struct transport *transport,\n \n \twrite_constant(helper->in, \"export\\n\");\n \n-\trecvline(data, &buf);\n-\tif (debug)\n-\t\tfprintf(stderr, \"Debug: Got export_marks '%s'\\n\", buf.buf);\n-\tif (buf.len) {\n-\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n-\t\tstrbuf_addbuf(&arg, &buf);\n-\t\texport_marks = strbuf_detach(&arg, NULL);\n-\t}\n-\n-\trecvline(data, &buf);\n-\tif (debug)\n-\t\tfprintf(stderr, \"Debug: Got import_marks '%s'\\n\", buf.buf);\n-\tif (buf.len) {\n-\t\tstruct strbuf arg = STRBUF_INIT;\n-\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n-\t\tstrbuf_addbuf(&arg, &buf);\n-\t\timport_marks = strbuf_detach(&arg, NULL);\n-\t}\n-\n \tstrbuf_reset(&buf);\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n@@ -754,8 +744,7 @@ static int push_refs_with_export(struct transport *transport,\n \n \t}\n \n-\tif (get_exporter(transport, &exporter,\n-\t\t\t export_marks, import_marks, &revlist_args))\n+\tif (get_exporter(transport, &exporter, &revlist_args))\n \t\tdie(\"Couldn't run fast-export\");\n \n \tif(finish_command(&exporter))\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149200","messageId":"1283053540-27042-12-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 11/13] remote-curl: accept empty line as terminator","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:38Z","receivedAt":"2010-08-29T03:45:38Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"The remote helper infrastructure terminates with a '\\n', which the\nremote-curl helper would interpret as a command to do '', a command\nit did not understand. Consequently it would 'return 1'.\n\nThis went unnoticed because the transport helper infrastructure did\nnot check the return value of the helper, nor did the helper print\nanything before exiting.\n\nCC: Ilari Liusvaara \nCC: Daniel Barkalow \n---\n\n I noticed this when my tests suddenly broke. Bisecting pointed at\n the 'more rigorous return value checking' patch, after which some\n poking around in the remote-curl helper pointed this out as the\n problem.\n\n I'm not very sure about the error message, if anyone feels it\n should go (it indicates a bug in the remote helper infrastructure,\n not a user error) it can be left out as far as I'm concerned.\n\n remote-curl.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 04d4813..27fcd69 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -813,6 +813,8 @@ int main(int argc, const char **argv)\n \tdo {\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n \t\t\tbreak;\n+\t\tif (buf.len == 0)\n+\t\t\tbreak;\n \t\tif (!prefixcmp(buf.buf, \"fetch \")) {\n \t\t\tif (nongit)\n \t\t\t\tdie(\"Fetch attempted without a local repo\");\n@@ -851,6 +853,7 @@ int main(int argc, const char **argv)\n \t\t\tprintf(\"\\n\");\n \t\t\tfflush(stdout);\n \t\t} else {\n+\t\t\tfprintf(stderr, \"Unknown command '%s'\\n\", buf.buf);\n \t\t\treturn 1;\n \t\t}\n \t\tstrbuf_reset(&buf);\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149198","messageId":"1283053540-27042-13-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 12/13] git-remote-testgit: only push for non-local repositories","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:39Z","receivedAt":"2010-08-29T03:45:39Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Trying to push for local repositories will fail since there is no\nlocal checkout in .git/info/... to push from.\n\nThis went unnoticed because the transport helper infrastructure did\nnot check the return value of the helper.\n---\n\n I guess it also shows how many people look at the verbose output of\n the helper test suite ;-).\n\n git-remote-testgit.py | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex e2b213d..b428b1c 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -160,7 +160,9 @@ def do_export(repo, args):\n \n update_local_repo(repo)\n repo.importer.do_import(repo.gitdir)\n- repo.non_local.push(repo.gitdir)\n+\n+ if not repo.local:\n+ repo.non_local.push(repo.gitdir)\n \n print \"ok refs/heads/master\"\n print\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149195","messageId":"1283053540-27042-14-git-send-email-srabbelier@gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"[PATCH 13/13] git-remote-testgit: fix error handling","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T03:45:40Z","receivedAt":"2010-08-29T03:45:40Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"If fast-export did not complete successfully the error handling code\nitself would error out.\n---\n\n *brown paper bag*\n\n git_remote_helpers/git/importer.py | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py\nindex 70a7127..d938611 100644\n--- a/git_remote_helpers/git/importer.py\n+++ b/git_remote_helpers/git/importer.py\n@@ -36,5 +36,6 @@ class GitImporter(object):\n args.append(\"--import-marks=\" + path)\n \n child = subprocess.Popen(args)\n- if child.wait() != 0:\n- raise CalledProcessError\n+ ret = child.wait()\n+ if ret != 0:\n+ raise subprocess.CalledProcessError(ret, args)\n-- \n1.7.2.1.240.g6a95c3\n"},{"id":"149221","messageId":"alpine.LNX.2.00.1008291443030.14365@iabervon.org","threadId":"24898","inReplyTo":"1283053540-27042-2-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-08-29T18:59:41Z","receivedAt":"2010-08-29T18:59:41Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n\n> Currently the only way to end an import stream is to close it, which\n> is not desirable when the stream that's being used is shared. For\n> example, the remote helper infrastructure uses a pipe between it and\n> the helper process, part of the protocol is to send a fast-import\n> stream accross. Without a way to end the stream the remote helper\n> infrastructure is forced to limit itself to have a command that uses\n> a fast-import stream as it's last command.\n> \n> Add a trivial 'done' command that causes fast-import to stop reading\n> from the stream and exit.\n\nYeah, this is definitely worthwhile.\n\n> ---\n> \n> Very straightforward. It is handled in parse_feature() instead of\n> in parse_one_feature() because I didn't want to allow '--done' as a\n> commandline argument. Allowing it would be silly, it surves no\n> other purpose than to indicate up front that the stream will\n> contain a 'done' command at the end.\n> \n> I'm fine too with dropping the feature and just adding the new\n> command, whichever is preferred.\n\nI think the point of the feature would be to get the error response up \nfront, where it might be easier to determine what to do about importers \nnot supporting it. As such, I think the command line option actually makes \nat least as much sense, but it's probably not necessary anyway.\n\nI believe there's a gfi mailing list, which ought to hear about this bit. \nNot that there are likely to be conflicts, but, when I was thinking about \nadding this command (for the same reason you're adding it), I'd called it \n\"quit\", so it's worth letting people know a de facto standard, so gfi \nimplementations don't vary.\n\nThe code looks obviously good to me.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"149222","messageId":"alpine.LNX.2.00.1008291500070.14365@iabervon.org","threadId":"24898","inReplyTo":"1283053540-27042-3-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 02/13] fast-export: support done feature","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-08-29T19:15:40Z","receivedAt":"2010-08-29T19:15:40Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n\n> If fast-export is being used to generate a fast-import stream that\n> will be used afterwards it is desirable to indicate the end of the\n> stream with the new 'done' command.\n> \n> Add a flag that causes fast-export to end with 'done'.\n\nI was assuming that whatever passed the output from fast-export to \nfast-import would add the \"done\" itself when its fast-export child \nexitted. Obviously, if there's going to be anything after the gfi stream, \nsomething's going to have to write the next thing, and whatever that is \ncan write the \"done\". Of course, the caller can't add the feature, so if \nthe feature is necessary (and I don't remember all the possible \ninteractions to say), this would be necessary.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"149224","messageId":"alpine.LNX.2.00.1008291521350.14365@iabervon.org","threadId":"24898","inReplyTo":"1283053540-27042-8-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 07/13] transport-helper: change import semantics","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-08-29T19:29:18Z","receivedAt":"2010-08-29T19:29:18Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n\n> Currently the helper must somehow guess how many import statements to\n> read before it starts outputting its fast-export stream. This is\n> because the remote helper infrastructure runs fast-import only once,\n> so the helper is forced to output one stream for all import commands\n> it will receive. The only reason this worked in the past was because\n> only one ref was imported at a time.\n\nI think your reasons for this change could be worked around, but the \nprotocol is cleaner with your change, which is justification enough, given \nthat it shouldn't be too big a deal to change. This also lets the helper \nconsider all of the refs it is expected to update before producing the \nstream, which may simplify the stream (particularly if the history has \nmerges involving branches that may or may not be imported are aren't \nlisted first).\n\n> Change the semantics of the import statement such that it matches\n> that of the list statement. That is, 'import\\n' is followed by a list\n> of refs that should be exported, followed by '\\n'.\n> ---\n> \n> This changes the protcol a bit, but I don't think we have many\n> users of the 'import' command yet, and if we do I would assume\n> they're paying attention to development in the remote helper space.\n\nI don't think \"import\" has gotten to the point where people could really \nuse it in helpers not packaged with git, anyway, so I agree.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"149225","messageId":"alpine.LNX.2.00.1008291529510.14365@iabervon.org","threadId":"24898","inReplyTo":"1283053540-27042-9-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 08/13] transport-helper: export should disconnect too","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-08-29T19:32:07Z","receivedAt":"2010-08-29T19:32:07Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n\n> Now that the remote helper protocol uses the new done command in its\n> fast-import streams, export no longer needs to be the last command in\n> the stream.\n> ---\n> \n> The fact that we had this before shows how messed up the protocol\n> was earlier. Basically, any 'import' or 'export' command meant\n> \"you're done talking to the helper now\".\n\nYup; this is a big improvement, and I'dhave done it this way in the first \nplace, had I realized how easy it would be to get fast-import to have a \n\"done\" command. Your subject is backwards, I think, though; export won't \nrequire a disconnect.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"149228","messageId":"alpine.LNX.2.00.1008291536030.14365@iabervon.org","threadId":"24898","inReplyTo":"1283053540-27042-11-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 10/13] transport-helper: implement marks location as capability","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-08-29T19:52:48Z","receivedAt":"2010-08-29T19:52:48Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n\n> While this requires the helper to flush stdout after listing 'gitdir'\n> as capability, and read a command (the 'gitdir' response from the\n> remote helper infrastructure) right after that, this is more elegant\n> and does not require an ad-hoc exchange of values.\n> \n> CC: Daniel Barkalow \n> ---\n> \n> Daniel made some fuss about the ad-hoc exchange when I first sent\n> the 'export command' series for review, and it's been nagging me.\n> \n> As you can see in the remote-testgit implementation, it's a bit\n> icky on the helper side (you have to flush sdout and read another\n> command in the middle of responding to 'capabilities'), but I think\n> it's better than the alternative.\n\nI think I was annoyed by it being ad-hoc, rather than having the exchange \nof values. I think if you need to get more information to the helper, you \nshould have a generic mechanism for that, rather than anything that cares \nabout the particular information involved.\n\nI'm a bit unclear on what change you're making here; it looks like the \nhelper side is reading another line, but that transport-helper isn't \nwriting anything new, and you don't have any changes to the documentation \nhere. Did this change get mixed into a different patch or something?\n\n> git-remote-testgit.py | 29 ++++++++++++++++-------------\n> transport-helper.c | 47 ++++++++++++++++++-----------------------------\n> 2 files changed, 34 insertions(+), 42 deletions(-)\n> \n> diff --git a/git-remote-testgit.py b/git-remote-testgit.py\n> index 50341ce..e2b213d 100644\n> --- a/git-remote-testgit.py\n> +++ b/git-remote-testgit.py\n> @@ -71,8 +71,24 @@ def do_capabilities(repo, args):\n> print \"import\"\n> print \"export\"\n> print \"gitdir\"\n> +\n> + sys.stdout.flush()\n> + if not read_one_line(repo):\n> + die(\"Expected gitdir, got empty line\")\n> +\n> print \"refspec refs/heads/*:%s*\" % repo.prefix\n> \n> + dirname = repo.get_base_path(repo.gitdir)\n> +\n> + if not os.path.exists(dirname):\n> + os.makedirs(dirname)\n> +\n> + path = os.path.join(dirname, 'testgit.marks')\n> +\n> + print \"*export-marks %s\" % path\n> + if os.path.exists(path):\n> + print \"*import-marks %s\" % path\n> +\n> print # end capabilities\n> \n> \n> @@ -142,19 +158,6 @@ def do_export(repo, args):\n> if not repo.gitdir:\n> die(\"Need gitdir to export\")\n> \n> - dirname = repo.get_base_path(repo.gitdir)\n> -\n> - if not os.path.exists(dirname):\n> - os.makedirs(dirname)\n> -\n> - path = os.path.join(dirname, 'testgit.marks')\n> - print path\n> - if os.path.exists(path):\n> - print path\n> - else:\n> - print \"\"\n> - sys.stdout.flush()\n> -\n> update_local_repo(repo)\n> repo.importer.do_import(repo.gitdir)\n> repo.non_local.push(repo.gitdir)\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 82bdad3..0edc1d5 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -23,6 +23,8 @@ struct helper_data\n> \t\tpush : 1,\n> \t\tconnect : 1,\n> \t\tno_disconnect_req : 1;\n> +\tchar *export_marks;\n> +\tchar *import_marks;\n> \t/* These go from remote name (as in \"list\") to private name */\n> \tstruct refspec *refspecs;\n> \tint refspec_nr;\n> @@ -179,6 +181,16 @@ static struct child_process *get_helper(struct transport *transport)\n> \t\t\tstrbuf_addf(&gitdir, \"gitdir %s\\n\", get_git_dir());\n> \t\t\tsendline(data, &gitdir);\n> \t\t\tstrbuf_release(&gitdir);\n> +\t\t} else if (!prefixcmp(capname, \"export-marks \")) {\n> +\t\t\tstruct strbuf arg = STRBUF_INIT;\n> +\t\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n> +\t\t\tstrbuf_addstr(&arg, capname + strlen(\"export-marks \"));\n> +\t\t\tdata->export_marks = strbuf_detach(&arg, NULL);\n> +\t\t} else if (!prefixcmp(capname, \"import-marks\")) {\n> +\t\t\tstruct strbuf arg = STRBUF_INIT;\n> +\t\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n> +\t\t\tstrbuf_addstr(&arg, capname + strlen(\"import-marks \"));\n> +\t\t\tdata->import_marks = strbuf_detach(&arg, NULL);\n> \t\t} else if (mandatory) {\n> \t\t\tdie(\"Unknown mandatory capability %s. This remote \"\n> \t\t\t \"helper probably needs newer version of Git.\\n\",\n> @@ -364,10 +376,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti\n> \n> static int get_exporter(struct transport *transport,\n> \t\t\tstruct child_process *fastexport,\n> -\t\t\tconst char *export_marks,\n> -\t\t\tconst char *import_marks,\n> \t\t\tstruct string_list *revlist_args)\n> {\n> +\tstruct helper_data *data = transport->data;\n> \tstruct child_process *helper = get_helper(transport);\n> \tint argc = 0, i;\n> \tmemset(fastexport, 0, sizeof(*fastexport));\n> @@ -378,10 +389,10 @@ static int get_exporter(struct transport *transport,\n> \tfastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));\n> \tfastexport->argv[argc++] = \"fast-export\";\n> \tfastexport->argv[argc++] = \"--use-done-feature\";\n> -\tif (export_marks)\n> -\t\tfastexport->argv[argc++] = export_marks;\n> -\tif (import_marks)\n> -\t\tfastexport->argv[argc++] = import_marks;\n> +\tif (data->export_marks)\n> +\t\tfastexport->argv[argc++] = data->export_marks;\n> +\tif (data->import_marks)\n> +\t\tfastexport->argv[argc++] = data->import_marks;\n> \n> \tfor (i = 0; i < revlist_args->nr; i++)\n> \t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n> @@ -708,7 +719,6 @@ static int push_refs_with_export(struct transport *transport,\n> \tstruct ref *ref;\n> \tstruct child_process *helper, exporter;\n> \tstruct helper_data *data = transport->data;\n> -\tchar *export_marks = NULL, *import_marks = NULL;\n> \tstruct string_list revlist_args = { NULL, 0, 0 };\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> @@ -716,26 +726,6 @@ static int push_refs_with_export(struct transport *transport,\n> \n> \twrite_constant(helper->in, \"export\\n\");\n> \n> -\trecvline(data, &buf);\n> -\tif (debug)\n> -\t\tfprintf(stderr, \"Debug: Got export_marks '%s'\\n\", buf.buf);\n> -\tif (buf.len) {\n> -\t\tstruct strbuf arg = STRBUF_INIT;\n> -\t\tstrbuf_addstr(&arg, \"--export-marks=\");\n> -\t\tstrbuf_addbuf(&arg, &buf);\n> -\t\texport_marks = strbuf_detach(&arg, NULL);\n> -\t}\n> -\n> -\trecvline(data, &buf);\n> -\tif (debug)\n> -\t\tfprintf(stderr, \"Debug: Got import_marks '%s'\\n\", buf.buf);\n> -\tif (buf.len) {\n> -\t\tstruct strbuf arg = STRBUF_INIT;\n> -\t\tstrbuf_addstr(&arg, \"--import-marks=\");\n> -\t\tstrbuf_addbuf(&arg, &buf);\n> -\t\timport_marks = strbuf_detach(&arg, NULL);\n> -\t}\n> -\n> \tstrbuf_reset(&buf);\n> \n> \tfor (ref = remote_refs; ref; ref = ref->next) {\n> @@ -754,8 +744,7 @@ static int push_refs_with_export(struct transport *transport,\n> \n> \t}\n> \n> -\tif (get_exporter(transport, &exporter,\n> -\t\t\t export_marks, import_marks, &revlist_args))\n> +\tif (get_exporter(transport, &exporter, &revlist_args))\n> \t\tdie(\"Couldn't run fast-export\");\n> \n> \tif(finish_command(&exporter))\n> -- \n> 1.7.2.1.240.g6a95c3\n> \n> \n"},{"id":"149229","messageId":"AANLkTinOqoLbmMyzUrKZTgWh67RAYHap-4-pubuF3WOy@mail.gmail.com","threadId":"24898","inReplyTo":"alpine.LNX.2.00.1008291536030.14365@iabervon.org","subject":"Re: [PATCH 10/13] transport-helper: implement marks location as capability","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T20:17:54Z","receivedAt":"2010-08-29T20:17:54Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 14:52, Daniel Barkalow wrote:\n> I think I was annoyed by it being ad-hoc, rather than having the exchange\n> of values. I think if you need to get more information to the helper, you\n> should have a generic mechanism for that, rather than anything that cares\n> about the particular information involved.\n\nIs the capability mechanism such as I used it now a good enough proxy for that?\n\n> I'm a bit unclear on what change you're making here; it looks like the\n> helper side is reading another line, but that transport-helper isn't\n> writing anything new, and you don't have any changes to the documentation\n> here. Did this change get mixed into a different patch or something?\n\nNot at all. It's reading another command: the reply to the 'gitdir'\ncapability, being the gitdir command.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149230","messageId":"AANLkTik7Lw7G=rAmrQ6cx4_s5_JCGmVwmV-vx7bN4kVG@mail.gmail.com","threadId":"24898","inReplyTo":"alpine.LNX.2.00.1008291443030.14365@iabervon.org","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T20:23:49Z","receivedAt":"2010-08-29T20:23:49Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"[+vcs-fast-import-devs, not culled for their benefit]\n\nOn Sun, Aug 29, 2010 at 13:59, Daniel Barkalow wrote:\n> On Sat, 28 Aug 2010, Sverre Rabbelier wrote:\n>> Currently the only way to end an import stream is to close it, which\n>> is not desirable when the stream that's being used is shared. For\n>> example, the remote helper infrastructure uses a pipe between it and\n>> the helper process, part of the protocol is to send a fast-import\n>> stream accross. Without a way to end the stream the remote helper\n>> infrastructure is forced to limit itself to have a command that uses\n>> a fast-import stream as it's last command.\n>>\n>> Add a trivial 'done' command that causes fast-import to stop reading\n>> from the stream and exit.\n>\n> Yeah, this is definitely worthwhile.\n>\n>> ---\n>>\n>>   Very straightforward. It is handled in parse_feature() instead of\n>>   in parse_one_feature() because I didn't want to allow '--done' as a\n>>   commandline argument. Allowing it would be silly, it surves no\n>>   other purpose than to indicate up front that the stream will\n>>   contain a 'done' command at the end.\n>>\n>>   I'm fine too with dropping the feature and just adding the new\n>>   command, whichever is preferred.\n>\n> I think the point of the feature would be to get the error response up\n> front, where it might be easier to determine what to do about importers\n> not supporting it. As such, I think the command line option actually makes\n> at least as much sense, but it's probably not necessary anyway.\n>\n> I believe there's a gfi mailing list, which ought to hear about this bit.\n\nI've added them.\n\n> Not that there are likely to be conflicts, but, when I was thinking about\n> adding this command (for the same reason you're adding it), I'd called it\n> \"quit\", so it's worth letting people know a de facto standard, so gfi\n> implementations don't vary.\n\nAgreed, I've added it to the fastimport python library without much trouble\n\n> The code looks obviously good to me.\n\nThanks.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149231","messageId":"AANLkTikeWG=PpR4knWAYuKfO7g4Edm5+a2_rR4VwSTeU@mail.gmail.com","threadId":"24898","inReplyTo":"alpine.LNX.2.00.1008291500070.14365@iabervon.org","subject":"Re: [PATCH 02/13] fast-export: support done feature","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T20:25:05Z","receivedAt":"2010-08-29T20:25:05Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 14:15, Daniel Barkalow wrote:\n> I was assuming that whatever passed the output from fast-export to\n> fast-import would add the \"done\" itself when its fast-export child\n> exitted.\n\nI tried this, but it results in some unelegant code where you have to\nmake sure to flush before starting the exporter, etc. I thought in\ngeneral it was better to make one program responsible for the entire\nstream.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149232","messageId":"AANLkTi=bLe_3u+-u=+L71JiKQrQwmVz6=x=h7mws8zaC@mail.gmail.com","threadId":"24898","inReplyTo":"alpine.LNX.2.00.1008291521350.14365@iabervon.org","subject":"Re: [PATCH 07/13] transport-helper: change import semantics","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T20:26:11Z","receivedAt":"2010-08-29T20:26:11Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 14:29, Daniel Barkalow wrote:\n> I think your reasons for this change could be worked around, but the\n> protocol is cleaner with your change, which is justification enough, given\n> that it shouldn't be too big a deal to change. This also lets the helper\n> consider all of the refs it is expected to update before producing the\n> stream, which may simplify the stream (particularly if the history has\n> merges involving branches that may or may not be imported are aren't\n> listed first).\n\nAye, that was also part of my motivation to do it this way (as opposed\nto e.g. running fast-import multiple times).\n\n> I don't think \"import\" has gotten to the point where people could really\n> use it in helpers not packaged with git, anyway, so I agree.\n\nGreat :)\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149233","messageId":"AANLkTi=EkmEeMUx=sSFjnvkVzhUyxtGv4-TL6SsKuBsA@mail.gmail.com","threadId":"24898","inReplyTo":"alpine.LNX.2.00.1008291529510.14365@iabervon.org","subject":"Re: [PATCH 08/13] transport-helper: export should disconnect too","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T20:28:13Z","receivedAt":"2010-08-29T20:28:13Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 14:32, Daniel Barkalow wrote:\n> Yup; this is a big improvement, and I'dhave done it this way in the first\n> place, had I realized how easy it would be to get fast-import to have a\n> \"done\" command. Your subject is backwards, I think, though; export won't\n> require a disconnect.\n\nDepends on how you look at it, the line this patch removes tells the\nremote helper infrastructure not to issue a newline when disconnecting\n(which was needed because the helper was already disconnected by that\ntime). On the other side though, you are right in that now the export\ncommand no longer requires the helper to disconnect as part of the\nexport command.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149238","messageId":"20100829212419.GC1890@burratino","threadId":"24898","inReplyTo":"1283053540-27042-2-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T21:24:20Z","receivedAt":"2010-08-29T21:24:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> Add a trivial 'done' command that causes fast-import to stop reading\n> from the stream and exit.\n\nI like it. \n\nIt is tempting to make the 'done' command mandatory when the \"done\"\nfeature is used, to prevent confusion from streams that are cut off\nearly. What do frontends currently do to handle that?\n"},{"id":"149239","messageId":"AANLkTik_kPy8p-OTy8E7fcLFMfKFHex2ppw4Oy7BesUX@mail.gmail.com","threadId":"24898","inReplyTo":"20100829212419.GC1890@burratino","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T21:28:14Z","receivedAt":"2010-08-29T21:28:14Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 16:24, Jonathan Nieder wrote:\n> It is tempting to make the 'done' command mandatory when the \"done\"\n> feature is used, to prevent confusion from streams that are cut off\n> early.  What do frontends currently do to handle that?\n\nIf the stream ends with an EOF at the end of a command, they would act\nas if that was the end of the stream. If it ends mid-stream (e.g.,\nwhile parsing a 'commit'), they would error out.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149240","messageId":"20100829213618.GD1890@burratino","threadId":"24898","inReplyTo":"1283053540-27042-4-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 03/13] transport-helper: factor out push_update_refs_status","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T21:36:18Z","receivedAt":"2010-08-29T21:36:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> +++ b/transport-helper.c\n> @@ -554,6 +554,9 @@ static int fetch(struct transport *transport,\n> \treturn -1;\n> }\n> \n> +static void push_update_refs_status(struct helper_data *data,\n> +\t\t\t\t struct ref *remote_refs);\n> +\n> static int push_refs_with_push(struct transport *transport,\n> \t\tstruct ref *remote_refs, int flags)\n> {\n> @@ -609,8 +612,17 @@ static int push_refs_with_push(struct transport *transport,\n[...]\n> +static void push_update_refs_status(struct helper_data *data,\n> +\t\t\t\t struct ref *remote_refs)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct ref *ref = remote_refs;\n> \twhile (1) {\n> \t\tchar *refname, *msg;\n> \t\tint status;\n> @@ -679,7 +691,7 @@ static int push_refs_with_push(struct transport *transport,\n> \t\tref->remote_status = msg;\n> \t}\n\nHmm, I am not too happy with the long loop without explicit condition.\nMaybe it would make sense to split out the loop body as its own function?\nSomething like\n\n\tstruct ref *ref = remote_refs;\n\tfor (;;) {\n\t\trecvline(data, &buf);\n\t\tif (!buf.len)\n\t\t\tbreak;\n\n\t\tpush_update_ref_status(&buf, &ref, remote_refs);\n\t}\n\n> \tstrbuf_release(&buf);\n> -\treturn 0;\n> +\treturn;\n\nNot necessary, I think.\n\n> }\n\nRegardless, for what it's worth,\n\nReviewed-by: Jonathan Nieder \n\nThanks for a pleasant read.\n"},{"id":"149241","messageId":"AANLkTikP_Mm4C2C_TC57Adi9egPMdv83htc1J8ZJ4mN-@mail.gmail.com","threadId":"24898","inReplyTo":"20100829213618.GD1890@burratino","subject":"Re: [PATCH 03/13] transport-helper: factor out push_update_refs_status","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-29T21:45:21Z","receivedAt":"2010-08-29T21:45:21Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 16:36, Jonathan Nieder wrote:\n> Hmm, I am not too happy with the long loop without explicit condition.\n> Maybe it would make sense to split out the loop body as its own function?\n> Something like\n>\n>        struct ref *ref = remote_refs;\n>        for (;;) {\n>                recvline(data, &buf);\n>                if (!buf.len)\n>                        break;\n>\n>                push_update_ref_status(&buf, &ref, remote_refs);\n>        }\n\nOk, will fix.\n\n>>       strbuf_release(&buf);\n>> -     return 0;\n>> +     return;\n>\n> Not necessary, I think.\n\nRemoved the return.\n\n> Reviewed-by: Jonathan Nieder \n>\n> Thanks for a pleasant read.\n\nThanks for reading :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149243","messageId":"20100829215223.GF1890@burratino","threadId":"24898","inReplyTo":"1283053540-27042-5-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 04/13] transport-helper: check status code of finish_command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T21:52:24Z","receivedAt":"2010-08-29T21:52:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -410,8 +412,11 @@ static int fetch_with_import(struct transport *transport,\n> \t\tsendline(data, &buf);\n> \t\tstrbuf_reset(&buf);\n> \t}\n> -\tdisconnect_helper(transport);\n> -\tfinish_command(&fastimport);\n> +\tif(disconnect_helper(transport))\n> +\t\tdie(\"Error while disconnecting helper\");\n> +\tif (finish_command(&fastimport))\n> +\t\tdie(\"Error while running fast-import\");\n\nNit: missing space after \"if\".\n\n> +\n> \tfree(fastimport.argv);\n> \tfastimport.argv = NULL;\n> \n> @@ -751,8 +756,10 @@ static int push_refs_with_export(struct transport *transport,\n> \t\tdie(\"Couldn't run fast-export\");\n> \n> \tdata->no_disconnect_req = 1;\n> -\tfinish_command(&exporter);\n> -\tdisconnect_helper(transport);\n> +\tif(finish_command(&exporter))\n> +\t\tdie(\"Error while running fast-export\");\n> +\tif(disconnect_helper(transport))\n\nLikewise.\n\nThanks for this. A test would be nice if someone has time to write\none.\n"},{"id":"149245","messageId":"20100829220239.GG1890@burratino","threadId":"24898","inReplyTo":"1283053540-27042-6-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 05/13] transport-helper: use the new done feature to properly do imports","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T22:02:39Z","receivedAt":"2010-08-29T22:02:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> Previously, the helper code would disconnect the helper before\n> starting fast-import. This was needed because there was no way to signal\n> that the helper was done other than to close stdout (which it would\n> do after importing iff the helper noticed it had been disconnected).\n[...]\n> I really like what this does for the sanity of the import\n\nYeah, agreed.\n\n> Instead, request that the fast-export uses the 'done' command\n[...]\n> --- a/git-remote-testgit.py\n> +++ b/git-remote-testgit.py\n> @@ -124,6 +124,8 @@ def do_import(repo, args):\n> repo = update_local_repo(repo)\n> repo.exporter.export_repo(repo.gitdir)\n> \n> + print \"done\"\n\nI am probably not reading carefully enough, but I do not see what\nthis has to do with fast-export. Is the patch actually about\nsomething like this?\n\n\tUse the 'done' command where possible for remote\n\thelpers.\n\n\tIn other words, use fast-export --use-done-feature to\n\tadd a 'done' command at the end of streams passed to\n\tremote helpers' \"import\" commands, and teach the\n\tremote helpers implementing \"export\" to use the 'done'\n\tcommand in turn when producing their streams.\n"},{"id":"149249","messageId":"20100829222554.GJ1890@burratino","threadId":"24898","inReplyTo":"1283053540-27042-7-git-send-email-srabbelier@gmail.com","subject":"Re: [RFC PATCH 06/13] transport-helper: update ref status after push with export","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T22:25:54Z","receivedAt":"2010-08-29T22:25:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> Obviously the testgit helper shouldn't just print 'ok' for master,\n> but it demonstrates the idea.\n\nFor those who (like me) wondered what it should do:\n\n\tWhen the push is complete, outputs one or more ok or\n\terror ? lines to indicate success or failure of\n\teach pushed ref. The status report output is terminated by a\n\tblank line. The option field may be quoted in a C style\n\tstring if it contains an LF.\n\nSo I guess testgit should be getting this information from the\nresult of non_local.push().\n"},{"id":"149250","messageId":"20100829223218.GL1890@burratino","threadId":"24898","inReplyTo":"AANLkTik_kPy8p-OTy8E7fcLFMfKFHex2ppw4Oy7BesUX@mail.gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-29T22:32:18Z","receivedAt":"2010-08-29T22:32:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sun, Aug 29, 2010 at 16:24, Jonathan Nieder wrote:\n\n>> It is tempting to make the 'done' command mandatory when the \"done\"\n>> feature is used, to prevent confusion from streams that are cut off\n>> early.  What do frontends currently do to handle that?\n>\n> If the stream ends with an EOF at the end of a command, they would act\n> as if that was the end of the stream. If it ends mid-stream (e.g.,\n> while parsing a 'commit'), they would error out.\n\nOkay, if the frontend is in control usually there would be some\nnonzero exit code or signal; and if transport-helper is in control, I\nthink it would notice after your series. I was just worried about\ninvocations like\n\n foo-fast-export | git fast-import\n\nwhere an error might go undiagnosed (and any error message drowned out\nby the summary fast-import writes at the end).\n\nWill think more.\n"},{"id":"149253","messageId":"AANLkTikx__RWGhxZUtdOKJy=X=0trfdnd50tcstHhRO3@mail.gmail.com","threadId":"24898","inReplyTo":"1283053540-27042-3-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 02/13] fast-export: support done feature","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-08-29T23:42:45Z","receivedAt":"2010-08-29T23:42:45Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sun, Aug 29, 2010 at 11:45 AM, Sverre Rabbelier wrote:\n> If fast-export is being used to generate a fast-import stream that\n> will be used afterwards it is desirable to indicate the end of the\n> stream with the new 'done' command.\n>\n> Add a flag that causes fast-export to end with 'done'.\n\nFor a user, what are the advantages of running it with the\n--use-done-feature? Perhaps this should just be made a\nnon-configurable default (ie. always use it) to save the user from\nsome thinking.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"149254","messageId":"AANLkTimoWjN+Q7XOkNMtoczpDQzecFgpnLYembQMmEdL@mail.gmail.com","threadId":"24898","inReplyTo":"20100829220239.GG1890@burratino","subject":"Re: [PATCH 05/13] transport-helper: use the new done feature to properly do imports","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T00:28:29Z","receivedAt":"2010-08-30T00:28:29Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 17:02, Jonathan Nieder wrote:\n>        In other words, use fast-export --use-done-feature to\n>        add a 'done' command at the end of streams passed to\n>        remote helpers' \"import\" commands, and teach the\n>        remote helpers implementing \"export\" to use the 'done'\n>        command in turn when producing their streams.\n\nYes, that's a more accurate description, thanks.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149255","messageId":"AANLkTimX4am2VZBRuYjOui9+-_ouGO1s1ck8V+-ajB3E@mail.gmail.com","threadId":"24898","inReplyTo":"20100829222554.GJ1890@burratino","subject":"Re: [RFC PATCH 06/13] transport-helper: update ref status after push with export","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T00:29:15Z","receivedAt":"2010-08-30T00:29:15Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 17:25, Jonathan Nieder wrote:\n> So I guess testgit should be getting this information from the\n> result of non_local.push().\n\nYes, or for example if a ref is a non-fast-forward, it should probably\ndetect that before even exporting it :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149256","messageId":"AANLkTik3H6hVgViAX5ur9Tq4tFQ9mJEPuTmAwcrLStvU@mail.gmail.com","threadId":"24898","inReplyTo":"20100829223218.GL1890@burratino","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T00:30:49Z","receivedAt":"2010-08-30T00:30:49Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 17:32, Jonathan Nieder wrote:\n> where an error might go undiagnosed (and any error message drowned out\n> by the summary fast-import writes at the end).\n>\n> Will think more.\n\nAs far as I'm concerned that should be the responsibility of the\nimporter. If there is an error it should make sure not to drown the\nerror message with it's summary. I think it does a pretty good job at\nthat already though, doesn't it? It even saves a log file to try and\nhelp you diagnose what went wrong.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149257","messageId":"AANLkTi=wdvZpPixsQ5B+mJUbnp40Myf4rQDS+pnLezW5@mail.gmail.com","threadId":"24898","inReplyTo":"AANLkTikx__RWGhxZUtdOKJy=X=0trfdnd50tcstHhRO3@mail.gmail.com","subject":"Re: [PATCH 02/13] fast-export: support done feature","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T00:32:01Z","receivedAt":"2010-08-30T00:32:01Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 18:42, Tay Ray Chuan wrote:\n> For a user, what are the advantages of running it with the\n> --use-done-feature? Perhaps this should just be made a\n> non-configurable default (ie. always use it) to save the user from\n> some thinking.\n\nNo, that won't do, since not all importers will support this feature.\nWe might want to make it the default in the future (users can always\nspecify --no-use-done-feature), but it should definitely not be made\nthe default now.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149258","messageId":"20100830010527.GA2305@burratino","threadId":"24898","inReplyTo":"1283053540-27042-10-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 09/13] transport-helper: Use capname for gitdir capability too","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T01:05:27Z","receivedAt":"2010-08-30T01:05:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> The first hunk was real silly and I should have caught it while\n> reviewing the patch that introduced the required capabilities.\n> \n> I suspect the reason the second hunk wasn't caught is because the\n> series that added 'gitdir' as capability, and the one that added\n> required capabilities were done in parallel.\n\nObviously good, and it looks to me like you caught all problems\nof this kind.\n"},{"id":"149259","messageId":"20100830013156.GD2305@burratino","threadId":"24898","inReplyTo":"1283053540-27042-11-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 10/13] transport-helper: implement marks location as capability","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T01:31:56Z","receivedAt":"2010-08-30T01:31:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> --- a/git-remote-testgit.py\n> +++ b/git-remote-testgit.py\n> @@ -71,8 +71,24 @@ def do_capabilities(repo, args):\n> print \"import\"\n> print \"export\"\n> print \"gitdir\"\n> +\n> + sys.stdout.flush()\n> + if not read_one_line(repo):\n> + die(\"Expected gitdir, got empty line\")\n\nThis seems fragile to me: shouldn't the remote helper check somehow\nthat the line it read was actually a gitdir line?\n\nNo other complaint on my part. Requiring a flush seems entirely\nappropriate to me, and if someone comes up with something nicer than\nthe \"capabilities\" sequence for requesting information, it would not\nbe the end of the world to have two ways to discover the .git dir.\n"},{"id":"149260","messageId":"AANLkTinyuCoC7P2kSS7epgfO3xjJ8mTEQ+P8qtsEmAct@mail.gmail.com","threadId":"24898","inReplyTo":"20100830013156.GD2305@burratino","subject":"Re: [PATCH 10/13] transport-helper: implement marks location as capability","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T01:35:51Z","receivedAt":"2010-08-30T01:35:51Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 20:31, Jonathan Nieder wrote:\n> This seems fragile to me: shouldn't the remote helper check somehow\n> that the line it read was actually a gitdir line?\n\nYou're probably right, the simplest way would be to check if repo.gitdir is set.\n\n> No other complaint on my part.  Requiring a flush seems entirely\n> appropriate to me, and if someone comes up with something nicer than\n> the \"capabilities\" sequence for requesting information, it would not\n> be the end of the world to have two ways to discover the .git dir.\n\nAgreed.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149261","messageId":"20100830013928.GE2305@burratino","threadId":"24898","inReplyTo":"1283053540-27042-12-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 11/13] remote-curl: accept empty line as terminator","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T01:39:28Z","receivedAt":"2010-08-30T01:39:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> I noticed this when my tests suddenly broke. Bisecting pointed at\n> the 'more rigorous return value checking' patch\n\nShouldn't this go before \"check status code of finish_command\" for\nbisectability, then?\n\n> I'm not very sure about the error message, if anyone feels it\n> should go (it indicates a bug in the remote helper infrastructure,\n> not a user error) it can be left out as far as I'm concerned.\n\nNo preference here.\n\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -813,6 +813,8 @@ int main(int argc, const char **argv)\n> \tdo {\n> \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n> \t\t\tbreak;\n> +\t\tif (buf.len == 0)\n> +\t\t\tbreak;\n\nThis is just a bug, I think. Other strbuf_getline() invocations in\nthat file all use the equivalent\n\n\tif (*buf->buf)\n\t\tbreak;\n\ntoo.\n \n> @@ -851,6 +853,7 @@ int main(int argc, const char **argv)\n> \t\t\tprintf(\"\\n\");\n> \t\t\tfflush(stdout);\n> \t\t} else {\n> +\t\t\tfprintf(stderr, \"Unknown command '%s'\\n\", buf.buf);\n> \t\t\treturn 1;\n> \t\t}\n\nReviewed-by: Jonathan Nieder \n"},{"id":"149263","messageId":"20100830014821.GF2305@burratino","threadId":"24898","inReplyTo":"1283053540-27042-13-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T01:48:21Z","receivedAt":"2010-08-30T01:48:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> Trying to push for local repositories will fail since there is no\n> local checkout in .git/info/... to push from.\n> \n> This went unnoticed because the transport helper infrastructure did\n> not check the return value of the helper.\n> ---\n> \n> I guess it also shows how many people look at the verbose output of\n> the helper test suite ;-).\n[...]\n> +++ b/git-remote-testgit.py\n> @@ -160,7 +160,9 @@ def do_export(repo, args):\n> \n> update_local_repo(repo)\n> repo.importer.do_import(repo.gitdir)\n> - repo.non_local.push(repo.gitdir)\n> +\n> + if not repo.local:\n> + repo.non_local.push(repo.gitdir)\n\n[warning: I have not read through remote-testgit carefully]\n\nCould you explain further? I see\n\n ERROR: could not find repo at .git/info/fast-import/4dc49bf026b65e6a1b28e2457d4d6393af8d382c/.git\n\nbut I do not know why there should have been a repo there, or why we would\nnot want to do the equivalent of\n\n git push . refs/testgit/origin/refs/heads/master:refs/heads/master\n"},{"id":"149264","messageId":"20100830015321.GG2305@burratino","threadId":"24898","inReplyTo":"1283053540-27042-1-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 00/13] remote helper improvements","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T01:53:21Z","receivedAt":"2010-08-30T01:53:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> I had a week and then some stray days here and there to do some more\n> work on git-remote-hg, the result of which is this series. It takes\n> the 'import' and 'export' commands out of their 'toy' stage, and gets\n> them ready for real usage.\n\nSign-off?\n\n> Although 'git-remote-testgit' is still the\n> only thing using them, 'git-remote-hg' is nearing completion, I hope\n> to send out an RFC for it Real Soon Now (TM).\n\nVery good to hear. :)\n"},{"id":"149265","messageId":"AANLkTimf1S_1Y=E+3bCv6CgoChrxY3gT32crwDGdhbeN@mail.gmail.com","threadId":"24898","inReplyTo":"20100830014821.GF2305@burratino","subject":"Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T01:59:45Z","receivedAt":"2010-08-30T01:59:45Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 20:48, Jonathan Nieder wrote:\n> [warning: I have not read through remote-testgit carefully]\n>\n> Could you explain further?  I see\n>\n>  ERROR: could not find repo at .git/info/fast-import/4dc49bf026b65e6a1b28e2457d4d6393af8d382c/.git\n\nThe repo in .git/info/... is only there iff the remote repo is not on\ndisk. If the remote _is_ on disk (i.e., repo.is_local),\n`repo.importer.do_import(repo.gitdir)` will have directly updated the\nremote. For remotes that are not on disk (i.e., not repo.is_local),\n`repo.importer.do_import(repo.gitdir)` will have instead updated a\non-disk clone of the remote, which is stored in .git/info/...\n\nSo, to answer your question:\n\n> but I do not know why there should have been a repo there\n\nThere should be a repo there only if the remote is not on disk.\n\n> or why we would\n> not want to do the equivalent of\n>\n>  git push . refs/testgit/origin/refs/heads/master:refs/heads/master\n\nThat isn't needed since the importer has already done that.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149266","messageId":"AANLkTinyPRHnkyH8j1QNw=b2VuJfG6iqvF6SBjHcFXaA@mail.gmail.com","threadId":"24898","inReplyTo":"20100830015321.GG2305@burratino","subject":"Re: [PATCH 00/13] remote helper improvements","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T02:01:10Z","receivedAt":"2010-08-30T02:01:10Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 20:53, Jonathan Nieder wrote:\n> Sign-off?\n\nThe next round probably, I wanted feedback first.\n\n> Very good to hear. :)\n\nAye, it'll be nice to have mercurial Just Work :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149267","messageId":"AANLkTik-kcXrTKJiN+euhYYgC4582oO_Nto6bk58pH1Z@mail.gmail.com","threadId":"24898","inReplyTo":"20100830013928.GE2305@burratino","subject":"Re: [PATCH 11/13] remote-curl: accept empty line as terminator","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T02:02:17Z","receivedAt":"2010-08-30T02:02:17Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 20:39, Jonathan Nieder wrote:\n> Shouldn't this go before \"check status code of finish_command\" for\n> bisectability, then?\n\nI guess so, the current code is already broken, but at least the tests pass now.\n\n> Reviewed-by: Jonathan Nieder \n\nThanks.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149268","messageId":"20100830020236.GH2305@burratino","threadId":"24898","inReplyTo":"AANLkTik3H6hVgViAX5ur9Tq4tFQ9mJEPuTmAwcrLStvU@mail.gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T02:02:36Z","receivedAt":"2010-08-30T02:02:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sun, Aug 29, 2010 at 17:32, Jonathan Nieder wrote:\n\n>> where an error might go undiagnosed (and any error message drowned out\n>> by the summary fast-import writes at the end).\n>>\n>> Will think more.\n>\n> As far as I'm concerned that should be the responsibility of the\n> importer. If there is an error it should make sure not to drown the\n> error message with it's summary. I think it does a pretty good job at\n> that already though, doesn't it? It even saves a log file to try and\n> help you diagnose what went wrong.\n\nI was thinking specifically of the case where one is unlucky enough\nfor the stream to end at a valid, early spot.\n\nThe way all importers seem to end up is to call \"git fast-import\" as a\nchild process (rather than advertising an interface like\n\n\tsvnrdump dump | svn-fe | git fast-import\n\n) so maybe this is not such a big deal.\n"},{"id":"149269","messageId":"AANLkTimNsVeGLB5=y8WyLqdkiQFwoBkdp_YrfuuT_5Ec@mail.gmail.com","threadId":"24898","inReplyTo":"20100830020236.GH2305@burratino","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T02:08:58Z","receivedAt":"2010-08-30T02:08:58Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 21:02, Jonathan Nieder wrote:\n> I was thinking specifically of the case where one is unlucky enough\n> for the stream to end at a valid, early spot.\n\nI think it makes sense to say that if you issue a 'feature done', we\nchange the code that checks for EOF to error instead of quit.\n\n> The way all importers seem to end up is to call \"git fast-import\" as a\n> child process (rather than advertising an interface like\n>\n>        svnrdump dump | svn-fe | git fast-import\n>\n> ) so maybe this is not such a big deal.\n\nDoes it matter much which way the importer is called? If it ends early\nat a valid point nobody will know regardless of how it is called, no?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"149270","messageId":"20100830020940.GI2305@burratino","threadId":"24898","inReplyTo":"AANLkTimf1S_1Y=E+3bCv6CgoChrxY3gT32crwDGdhbeN@mail.gmail.com","subject":"Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T02:09:40Z","receivedAt":"2010-08-30T02:09:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sun, Aug 29, 2010 at 20:48, Jonathan Nieder wrote:\n\n>> or why we would\n>> not want to do the equivalent of\n>>\n>>  git push . refs/testgit/origin/refs/heads/master:refs/heads/master\n>\n> That isn't needed since the importer has already done that.\n\nGot it. Thanks for the explanation.\n"},{"id":"149271","messageId":"20100830021205.GJ2305@burratino","threadId":"24898","inReplyTo":"AANLkTimNsVeGLB5=y8WyLqdkiQFwoBkdp_YrfuuT_5Ec@mail.gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-30T02:12:05Z","receivedAt":"2010-08-30T02:12:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"[out of order for convenience]\nSverre Rabbelier wrote:\n\n> Does it matter much which way the importer is called? If it ends early\n> at a valid point nobody will know regardless of how it is called, no?\n\nIf the importer calls fast-import itself, it can\n\n 1. close the pipe to fast-import\n 2. wait for fast-import to exit\n 3. print a relevant message\n 4. exit\n\n> I think it makes sense to say that if you issue a 'feature done', we\n> change the code that checks for EOF to error instead of quit.\n\nOk. :)\n"},{"id":"149281","messageId":"AANLkTi=Rn2wEs3Zrq9OHha9SMTo9EPD5HgBxy5mJHUeW@mail.gmail.com","threadId":"24898","inReplyTo":"1283137728899-5476616.post@n2.nabble.com","subject":"Re: [PATCH 00/13] remote helper improvements","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-08-30T05:54:24Z","receivedAt":"2010-08-30T05:54:24Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Aug 29, 2010 at 22:08, a721018 <281422091@qq.com> wrote:\n\n\n\n> --\n> View this message in context: http://git.661346.n2.nabble.com/PATCH-00-13-remote-helper-improvements-tp5474106p5476616.html\n> Sent from the git mailing list archive at Nabble.com.\n\nJunio, who maintains git@vger.kernel.org, is it Warthog (cc-ed)? Can\nwe please have this spam dealt with? I recall we're using some kind of\nword/regexp based block list, I reckon it would do well with some shoe\nrelated terms...\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"160986","messageId":"20110213094212.GA25435@elie","threadId":"24898","inReplyTo":"1283053540-27042-2-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH 01/13] fast-import: add the 'done' command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-02-13T09:42:12Z","receivedAt":"2011-02-13T09:42:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Sverre et al,\n\nSverre Rabbelier wrote:\n\n> Currently the only way to end an import stream is to close it, which\n> is not desirable when the stream that's being used is shared.\n\nHere's a variation on the same theme, with notes indicating\nwhat remains to be fixed. Maybe it can save someone some time.\n\n-- 8< --\nFrom: Sverre Rabbelier \nDate: Sat, 28 Aug 2010 22:45:28 -0500\nSubject: fast-import: introduce 'done' command\n\nAdd a 'done' command that causes fast-import to stop reading from the\nstream and exit.\n\nIf the new --done command line flag was passed on the command line\n(or a \"feature done\" declaration included at the start of the stream),\nmake the 'done' command mandatory. So \"git fast-import --done\"'s\ninput format will be prefix-free, making errors easier to detect when\nthey show up as early termination at some convenient time of the\nupstream of a pipe writing to fast-import.\n\nAnother possible application of the 'done' command would to be allow a\nfast-import stream that is only a small part of a larger encapsulating\nstream to be easily parsed, leaving the file offset after the \"done\\n\"\nso the other application can pick up from there. This patch does not\nteach fast-import to do that --- fast-import still uses buffered input\n(stdio).\n\nSigned-off-by: Jonathan Nieder \n---\n Documentation/git-fast-import.txt | 25 ++++++++++++++++++++++\n fast-import.c | 14 ++++++++++++\n t/t9300-fast-import.sh | 42 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 81 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex c3a2766..d0efdf8 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -101,6 +101,12 @@ OPTIONS\n \twhen the `cat-blob` command is encountered in the stream.\n \tThe default behaviour is to write to `stdout`.\n \n+--done::\n+\tRequire a `done` command at the end of the stream.\n+\tThis option might be useful for detecting errors that\n+\tcause the frontend to terminate before it has started to\n+\twrite a stream.\n+\n --export-pack-edges=::\n \tAfter creating a packfile, print a line of data to\n \t listing the filename of the packfile and the last\n@@ -329,6 +335,11 @@ and control the current import process. More detailed discussion\n \tstandard output. This command is optional and is not needed\n \tto perform an import.\n \n+`done`::\n+\tMarks the end of the stream. This command is optional\n+\tunless the `done` feature was requested using the\n+\t`--done` command line option or `feature done` command.\n+\n `cat-blob`::\n \tCauses fast-import to print a blob in 'cat-file --batch'\n \tformat to the file descriptor set with `--cat-blob-fd` or\n@@ -958,6 +969,11 @@ notes::\n \tVersions of fast-import not supporting notes will exit\n \twith a message indicating so.\n \n+done::\n+\tError out if the stream ends without a 'done' command.\n+\tWithout this feature, errors causing the frontend to end\n+\tabruptly at a convenient point in the stream can go\n+\tundetected.\n \n `option`\n ~~~~~~~~\n@@ -987,6 +1003,15 @@ not be passed as option:\n * cat-blob-fd\n * force\n \n+`done`\n+~~~~~~\n+If the `done` feature is not in use, treated as if EOF was read.\n+This can be used to tell fast-import to finish early.\n+\n+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 Crash Reports\n -------------\n If fast-import is supplied invalid input it will terminate with a\ndiff --git a/fast-import.c b/fast-import.c\nindex 3886a1b..cbcf61f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -365,6 +365,7 @@ static unsigned int cmd_save = 100;\n static uintmax_t next_mark;\n static struct strbuf new_data = STRBUF_INIT;\n static int seen_data_command;\n+static int require_explicit_termination;\n \n /* Signal handling */\n static volatile sig_atomic_t checkpoint_requested;\n@@ -2999,6 +3000,8 @@ static int parse_one_feature(const char *feature, int from_stream)\n \t\trelative_marks_paths = 1;\n \t} else if (!prefixcmp(feature, \"no-relative-marks\")) {\n \t\trelative_marks_paths = 0;\n+\t} else if (!strcmp(feature, \"done\")) {\n+\t\trequire_explicit_termination = 1;\n \t} else if (!prefixcmp(feature, \"force\")) {\n \t\tforce_update = 1;\n \t} else if (!strcmp(feature, \"notes\")) {\n@@ -3150,6 +3153,8 @@ int main(int argc, const char **argv)\n \t\t\tparse_reset_branch();\n \t\telse if (!strcmp(\"checkpoint\", command_buf.buf))\n \t\t\tparse_checkpoint();\n+\t\telse if (!strcmp(\"done\", command_buf.buf))\n+\t\t\tbreak;\n \t\telse if (!prefixcmp(command_buf.buf, \"progress \"))\n \t\t\tparse_progress();\n \t\telse if (!prefixcmp(command_buf.buf, \"feature \"))\n@@ -3169,6 +3174,15 @@ int main(int argc, const char **argv)\n \tif (!seen_data_command)\n \t\tparse_argv();\n \n+\t/*\n+\t * NEEDSWORK: we should report input errors before\n+\t * errno has a chance to be clobbered.\n+\t */\n+\tif (ferror(stdin))\n+\t\tdie(\"error reading input\");\n+\tif (require_explicit_termination && feof(stdin))\n+\t\tdie(\"stream ends early\");\n+\n \tend_packfile();\n \n \tdump_branches();\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 52ac0e5..a366ee2 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2121,6 +2121,48 @@ test_expect_success 'R: quiet option results in no stats being output' '\n test_cmp empty output\n '\n \n+test_expect_success 'R: feature done means terminating \"done\" is mandatory' '\n+\techo feature done | test_must_fail git fast-import &&\n+\ttest_must_fail git fast-import --done expect <<-\\EOF &&\n+\tOBJID\n+\t:000000 100644 OBJID OBJID A\thello.c\n+\t:000000 100644 OBJID OBJID A\thello2.c\n+\tEOF\n+\tgit fast-import <<-EOF &&\n+\tcommit refs/heads/done-ends\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >input <