Hi, I'll comment on "[PATCH v3 16/23] transport-helper: use the new done feature where possible" ( http://thread.gmane.org/gmane.comp.version-control.git/177255/focus=177274) first. There is a hunk for transport-helper.c @@ -417,11 +418,8 @@ static int fetch_with_import(struct transport *transport, sendline(data, &buf); strbuf_reset(&buf); } - if (disconnect_helper(transport)) - die("Error while disconnecting helper"); if (finish_command(&fastimport)) die("Error while running fast-import"); - free(fastimport.argv); fastimport.argv = NULL; This is related to the done feature but a helper can use the done feature without this hunk. And it does change the import semantics, so I'd suggest moving the hunk here, to "[PATCH v3 18/23] transport-helper: change import semantics". On Sat, Jul 16, 2011 at 7:03 PM, Sverre Rabbelier <srabbelier@xxxxxxxxx> wrote: > Currently the helper must somehow guess how many import statements to > read before it starts outputting its fast-export stream. This is > because the remote helper infrastructure runs fast-import only once, > so the helper is forced to output one stream for all import commands > it will receive. The only reason this worked in the past was because > only one ref was imported at a time. > > Change the semantics of the import statement such that it matches > that of the push statement. That is, the import statement is followed > by a series of import statements that are terminated by a '\n'. I'll add more comments to the "import" job termination. Before these two patches: The helper input was closed which caused it to exit just after writing the import stream. (first, helper can terminate on eof; second it can terminate on '\n' just before eof; third, it may know that the import always was the last command and terminate before reading '\n'). The transport-helper.o didn't held a copy of helper's stdout, so once the helper exits this end of import-stream pipe becomes closed. And then the importer saw EOF on stdin and could terminate normally. After these patches: transport-helper waits for importer to terminate, so the helper must either a) crash or exit during or just after the import and reading no more than '\n' (blocking read beyond this character may hang us up) or b) use the 'done' feature to make importer terminate normally So the old helper won't hang us up if it terminates (or at least closes stdout) on just after the import or if it can read '\n' (but not blocking read further) and terminate. But if it just ignores empty lines and waits for new commands or eof as an exit indicator, it all hangs. Sadly, old documentation didn't mention many of these quirks, but on the other hand the old semantic is a bit broken and not documented so it's all ok. We should definitely add a new description for import command to Documentation/git-remote-helpers.txt Something roughly like this addition: A batch sequence of one or more import commands is terminated with a blank line. Single fast-import stream should be produced for the whole batch. If the helper is able to proceed with more commands after the import (in earlier versions import used to be the last command for a helper) it must use the "done" feature to indicate the end of this batch's import stream for the importer, otherwise the importer will wait forever and the caller will wait for the importer to finish. Aside from documenting and squashing I think this patch is the best way to proceed with improving the import command. > > Signed-off-by: Sverre Rabbelier <srabbelier@xxxxxxxxx> > --- > > As Jonathan suggested we now follow push' example, rather than > 'list'. It makes the remote-testgit code a bit longer, but it means > less changes to remote-helper.c. > > git-remote-testgit.py | 16 +++++++++++++++- > t/t5800-remote-helpers.sh | 2 +- > transport-helper.c | 3 +++ > 3 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/git-remote-testgit.py b/git-remote-testgit.py > index 0b5928d..1ed7a56 100644 > --- a/git-remote-testgit.py > +++ b/git-remote-testgit.py > @@ -120,8 +120,22 @@ def do_import(repo, args): > if not repo.gitdir: > die("Need gitdir to import") > > + ref = args[0] > + refs = [ref] > + > + while True: > + line = sys.stdin.readline() > + if line == '\n': > + break > + if not line.startswith('import '): > + die("Expected import line.") > + > + # strip of leading 'import ' > + ref = line[7:].strip() > + refs.append(ref) > + > repo = update_local_repo(repo) > - repo.exporter.export_repo(repo.gitdir, args) > + repo.exporter.export_repo(repo.gitdir, refs) > > print "done" > > diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh > index 12f471c..1c62001 100755 > --- a/t/t5800-remote-helpers.sh > +++ b/t/t5800-remote-helpers.sh > @@ -98,7 +98,7 @@ test_expect_success 'fetch new branch' ' > compare_refs public HEAD localclone FETCH_HEAD > ' > > -test_expect_failure 'fetch multiple branches' ' > +test_expect_success 'fetch multiple branches' ' > (cd localclone && > git fetch > ) && > diff --git a/transport-helper.c b/transport-helper.c > index a8f69b0..0c00be9 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -418,6 +418,9 @@ static int fetch_with_import(struct transport *transport, > sendline(data, &buf); > strbuf_reset(&buf); > } > + > + write_constant(data->helper->in, "\n"); > + > if (finish_command(&fastimport)) > die("Error while running fast-import"); > free(fastimport.argv); > -- > 1.7.5.1.292.g728120 > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html