[PATCH 00/13] remote helper improvements

[PATCH 00/13] remote helper improvements

[Sverre Rabbelier]

I had a week and then some stray days here and there to do some more
work on git-remote-hg, the result of which is this series. It takes
the 'import' and 'export' commands out of their 'toy' stage, and gets
them ready for real usage. Although 'git-remote-testgit' is still the
only thing using them, 'git-remote-hg' is nearing completion, I hope
to send out an RFC for it Real Soon Now (TM).

Sverre Rabbelier (13):
      fast-import: add the 'done' command
      fast-export: support done feature

These two are very important to the rest of the series, most of the
clean up relies on the 'done' command to make 'import/export' part of
the remote helper protocol not suck.

      transport-helper: check status code of finish_command
      remote-curl: accept empty line as terminator

If nothing else is applied, these two should be taken out together
and applied separately.

      transport-helper: factor out push_update_refs_status
      transport-helper: update ref status after push with export

This is not very fleshed out yet, (the second patch in particular),
but without this 'git push' to a remote that uses the 'export'
capability will always say 'everything up-to-date'.

      transport-helper: use the new done feature to properly do imports
      transport-helper: export should disconnect too

These two make the 'import' and 'export' command re-entrant. That is,
now the remote helper infrastructure could issue other commands after
issuing an 'import' or 'export' command.

      transport-helper: change import semantics

This is another cleanup to the protocol, without this it is more or
less impossible to import multiple refs.

      transport-helper: Use capname for gitdir capability too

This is a candidate for for maint, the current implementation is just
plain wrong.

      transport-helper: implement marks location as capability

Another protocol cleanup.

      git-remote-testgit: only push for non-local repositories
      git-remote-testgit: fix error handling

Both of these are maint candidates, they are bugfixes.

 Documentation/git-fast-export.txt  |    4 ++
 Documentation/git-fast-import.txt  |   17 ++++++-
 builtin/fast-export.c              |    9 +++
 fast-import.c                      |    5 ++
 git-remote-testgit.py              |   50 +++++++++++++------
 git_remote_helpers/git/importer.py |    5 +-
 remote-curl.c                      |    3 +
 transport-helper.c                 |   97 +++++++++++++++++++----------------
 8 files changed, 127 insertions(+), 63 deletions(-)

[PATCH 01/13] fast-import: add the 'done' command

[Sverre Rabbelier]

Currently the only way to end an import stream is to close it, which
is not desirable when the stream that's being used is shared. For
example, the remote helper infrastructure uses a pipe between it and
the helper process, part of the protocol is to send a fast-import
stream accross. Without a way to end the stream the remote helper
infrastructure is forced to limit itself to have a command that uses
a fast-import stream as it's last command.

Add a trivial 'done' command that causes fast-import to stop reading
from the stream and exit.
---

  Very straightforward. It is handled in parse_feature() instead of
  in parse_one_feature() because I didn't want to allow '--done' as a
  commandline argument. Allowing it would be silly, it surves no
  other purpose than to indicate up front that the stream will
  contain a 'done' command at the end.

  I'm fine too with dropping the feature and just adding the new
  command, whichever is preferred.

 Documentation/git-fast-import.txt |   17 ++++++++++++++++-
 fast-import.c                     |    5 +++++
 2 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 77a0a24..114f919 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -293,6 +293,10 @@ and control the current import process.  More detailed discussion
 	creating a new commit and updating the branch to point at
 	the newly created commit.
 
+`done`::
+	Treated as if EOF was read. This command is optional and is
+	not needed to perform an import.
+
 `tag`::
 	Creates an annotated tag object from an existing commit or
 	branch.  Lightweight tags are not supported by this command,
@@ -885,17 +889,20 @@ The <feature> part of the command may be any string matching
 ^[a-zA-Z][a-zA-Z-]*$ and should be understood by fast-import.
 
 Feature work identical as their option counterparts with the
-exception of the import-marks feature, see below.
+exception of the done and import-marks features, see below.
 
 The following features are currently supported:
 
 * date-format
+* done
 * import-marks
 * export-marks
 * relative-marks
 * no-relative-marks
 * force
 
+If the done feature is specified, the done command must be supported.
+
 The import-marks behaves differently from when it is specified as
 commandline option in that only one "feature import-marks" is allowed
 per stream. Also, any --import-marks= specified on the commandline
@@ -928,6 +935,14 @@ not be passed as option:
 * export-marks
 * force
 
+`done`
+~~~~~~
+
+Treated as if EOF was read. This can be used to stop fast-import
+from reading from the stream without closing the file handle. Such
+may be desired if the file handle is used for other purposes other
+than fast-import as well, and closing it is not desired.
+
 Crash Reports
 -------------
 If fast-import is supplied invalid input it will terminate with a
diff --git a/fast-import.c b/fast-import.c
index ddad289..1c3fa7d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2817,6 +2817,9 @@ static void parse_feature(void)
 	if (parse_one_feature(feature, 1))
 		return;
 
+	if (!prefixcmp(feature, "done"))
+		return;
+
 	die("This version of fast-import does not support feature %s.", feature);
 }
 
@@ -2935,6 +2938,8 @@ int main(int argc, const char **argv)
 			parse_new_blob();
 		else if (!prefixcmp(command_buf.buf, "commit "))
 			parse_new_commit();
+		else if (!prefixcmp(command_buf.buf, "done"))
+			break;
 		else if (!prefixcmp(command_buf.buf, "tag "))
 			parse_new_tag();
 		else if (!prefixcmp(command_buf.buf, "reset "))
-- 
1.7.2.1.240.g6a95c3

[PATCH 02/13] fast-export: support done feature

[Sverre Rabbelier]

If fast-export is being used to generate a fast-import stream that
will be used afterwards it is desirable to indicate the end of the
stream with the new 'done' command.

Add a flag that causes fast-export to end with 'done'.
---

  Also very trivial, obviously if the corresponding feature is
  removed the flag should be named differently.

 Documentation/git-fast-export.txt |    4 ++++
 builtin/fast-export.c             |    9 +++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 98ec6b5..912562e 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -82,6 +82,10 @@ marks the same across runs.
 	allow that.  So fake a tagger to be able to fast-import the
 	output.
 
+--use-done-feature::
+	Start the stream with a 'feature done' stanza, and terminate
+	it with a 'done' command.
+
 --no-data::
 	Skip output of blob objects and instead refer to blobs via
 	their original SHA-1 hash.  This is useful when rewriting the
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 9fe25ff..0c39c2e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -26,6 +26,7 @@ static int progress;
 static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
 static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;
 static int fake_missing_tagger;
+static int use_done_feature;
 static int no_data;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -584,6 +585,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			     "Import marks from this file"),
 		OPT_BOOLEAN(0, "fake-missing-tagger", &fake_missing_tagger,
 			     "Fake a tagger when tags lack one"),
+		OPT_BOOLEAN(0, "use-done-feature", &use_done_feature,
+			     "Use the done feature to terminate the stream"),
 		{ OPTION_NEGBIT, 0, "data", &no_data, NULL,
 			"Skip output of blob data",
 			PARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },
@@ -605,6 +608,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
+	if (use_done_feature)
+		printf("feature done\n");
+
 	if (import_filename)
 		import_marks(import_filename);
 
@@ -629,5 +635,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (export_filename)
 		export_marks(export_filename);
 
+	if (use_done_feature)
+		printf("done\n");
+
 	return 0;
 }
-- 
1.7.2.1.240.g6a95c3

[PATCH 03/13] transport-helper: factor out push_update_refs_status

[Sverre Rabbelier]

The update ref status part of push is useful for the export command
as well, factor it out into it's own function.
---

  I didn't move the new function up above push_refs_with_push so that
  it is obvious to the reviewer that the change is trivial.

 transport-helper.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 191fbf7..9f2ad00 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -554,6 +554,9 @@ static int fetch(struct transport *transport,
 	return -1;
 }
 
+static void push_update_refs_status(struct helper_data *data,
+				    struct ref *remote_refs);
+
 static int push_refs_with_push(struct transport *transport,
 		struct ref *remote_refs, int flags)
 {
@@ -609,8 +612,17 @@ static int push_refs_with_push(struct transport *transport,
 
 	strbuf_addch(&buf, '\n');
 	sendline(data, &buf);
+	strbuf_release(&buf);
+
+	push_update_refs_status(data, remote_refs);
+	return 0;
+}
 
-	ref = remote_refs;
+static void push_update_refs_status(struct helper_data *data,
+				    struct ref *remote_refs)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct ref *ref = remote_refs;
 	while (1) {
 		char *refname, *msg;
 		int status;
@@ -679,7 +691,7 @@ static int push_refs_with_push(struct transport *transport,
 		ref->remote_status = msg;
 	}
 	strbuf_release(&buf);
-	return 0;
+	return;
 }
 
 static int push_refs_with_export(struct transport *transport,
-- 
1.7.2.1.240.g6a95c3

[PATCH 04/13] transport-helper: check status code of finish_command

[Sverre Rabbelier]

Previously the status code of all helpers were ignored, allowing
errors that occur to go unnoticed if the error text output by the
helper is not noticed (or was not present at all).
---

  I'm surprised nobody fixed this sooner.

 transport-helper.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 9f2ad00..4a2826d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -204,6 +204,7 @@ static int disconnect_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
 	struct strbuf buf = STRBUF_INIT;
+	int res = 0;
 
 	if (data->helper) {
 		if (debug)
@@ -215,13 +216,13 @@ static int disconnect_helper(struct transport *transport)
 		close(data->helper->in);
 		close(data->helper->out);
 		fclose(data->out);
-		finish_command(data->helper);
+		res = finish_command(data->helper);
 		free((char *)data->helper->argv[0]);
 		free(data->helper->argv);
 		free(data->helper);
 		data->helper = NULL;
 	}
-	return 0;
+	return res;
 }
 
 static const char *unsupported_options[] = {
@@ -299,12 +300,13 @@ static void standard_options(struct transport *t)
 
 static int release_helper(struct transport *transport)
 {
+	int res = 0;
 	struct helper_data *data = transport->data;
 	free_refspec(data->refspec_nr, data->refspecs);
 	data->refspecs = NULL;
-	disconnect_helper(transport);
+	res = disconnect_helper(transport);
 	free(transport->data);
-	return 0;
+	return res;
 }
 
 static int fetch_with_fetch(struct transport *transport,
@@ -410,8 +412,11 @@ static int fetch_with_import(struct transport *transport,
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
-	disconnect_helper(transport);
-	finish_command(&fastimport);
+	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;
 
@@ -751,8 +756,10 @@ static int push_refs_with_export(struct transport *transport,
 		die("Couldn't run fast-export");
 
 	data->no_disconnect_req = 1;
-	finish_command(&exporter);
-	disconnect_helper(transport);
+	if(finish_command(&exporter))
+		die("Error while running fast-export");
+	if(disconnect_helper(transport))
+		die("Error while disconnecting helper");
 	return 0;
 }
 
-- 
1.7.2.1.240.g6a95c3

[PATCH 05/13] transport-helper: use the new done feature to properly do imports

[Sverre Rabbelier]

Previously, the helper code would disconnect the helper before
starting fast-import. This was needed because there was no way to signal
that the helper was done other than to close stdout (which it would
do after importing iff the helper noticed it had been disconnected).

Instead, request that the fast-export uses the 'done' command to
signal when it is done exporting, so that we can disconnect the
helper at a time of our choosing.
---

  I really like what this does for the sanity of the import
  implementation, it makes it much more like a regular (re-entrant)
  command, rather than the "sorry, you're done now" way it is now.

 git-remote-testgit.py |    2 ++
 transport-helper.c    |    8 ++------
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..612cb5a 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -124,6 +124,8 @@ def do_import(repo, args):
     repo = update_local_repo(repo)
     repo.exporter.export_repo(repo.gitdir)
 
+    print "done"
+
 
 def do_export(repo, args):
     """Imports a fast-import stream from git to testgit.
diff --git a/transport-helper.c b/transport-helper.c
index 4a2826d..5647595 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -375,8 +375,9 @@ static int get_exporter(struct transport *transport,
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
 	fastexport->out = dup(helper->in);
-	fastexport->argv = xcalloc(4 + revlist_args->nr, sizeof(*fastexport->argv));
+	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
+	fastexport->argv[argc++] = "--use-done-feature";
 	if (export_marks)
 		fastexport->argv[argc++] = export_marks;
 	if (import_marks)
@@ -412,11 +413,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;
 
@@ -758,8 +756,6 @@ static int push_refs_with_export(struct transport *transport,
 	data->no_disconnect_req = 1;
 	if(finish_command(&exporter))
 		die("Error while running fast-export");
-	if(disconnect_helper(transport))
-		die("Error while disconnecting helper");
 	return 0;
 }
 
-- 
1.7.2.1.240.g6a95c3

[RFC PATCH 06/13] transport-helper: update ref status after push with export

[Sverre Rabbelier]

---

  Obviously the testgit helper shouldn't just print 'ok' for master,
  but it demonstrates the idea.

 git-remote-testgit.py |    3 +++
 transport-helper.c    |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 612cb5a..342a05d 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -151,6 +151,9 @@ def do_export(repo, args):
     repo.importer.do_import(repo.gitdir)
     repo.non_local.push(repo.gitdir)
 
+    print "ok refs/heads/master"
+    print
+
 
 def do_gitdir(repo, args):
     """Stores the location of the gitdir.
diff --git a/transport-helper.c b/transport-helper.c
index 5647595..ecaea25 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -756,6 +756,7 @@ static int push_refs_with_export(struct transport *transport,
 	data->no_disconnect_req = 1;
 	if(finish_command(&exporter))
 		die("Error while running fast-export");
+	push_update_refs_status(data, remote_refs);
 	return 0;
 }
 
-- 
1.7.2.1.240.g6a95c3

[PATCH 07/13] transport-helper: change import semantics

[Sverre Rabbelier]

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 list statement. That is, 'import\n' is followed by a list
of refs that should be exported, followed by '\n'.
---

  This changes the protcol a bit, but I don't think we have many
  users of the 'import' command yet, and if we do I would assume
  they're paying attention to development in the remote helper space.

 git-remote-testgit.py |   12 ++++++++++--
 transport-helper.c    |    7 ++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 342a05d..50341ce 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -115,12 +115,20 @@ def do_import(repo, args):
     """Exports a fast-import stream from testgit for git to import.
     """
 
-    if len(args) != 1:
-        die("Import needs exactly one ref")
+    if args:
+        die("Import expects its ref seperately")
 
     if not repo.gitdir:
         die("Need gitdir to import")
 
+    refs = []
+
+    while True:
+        line = sys.stdin.readline()
+        if line == '\n':
+            break
+        refs.append(line.strip())
+
     repo = update_local_repo(repo)
     repo.exporter.export_repo(repo.gitdir)
 
diff --git a/transport-helper.c b/transport-helper.c
index ecaea25..13ebb3b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -404,15 +404,20 @@ static int fetch_with_import(struct transport *transport,
 	if (get_importer(transport, &fastimport))
 		die("Couldn't run fast-import");
 
+	write_constant(data->helper->in, "import\n");
+
 	for (i = 0; i < nr_heads; i++) {
 		posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
 
-		strbuf_addf(&buf, "import %s\n", posn->name);
+		strbuf_addf(&buf, "%s\n", posn->name);
 		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.2.1.240.g6a95c3

[PATCH 08/13] transport-helper: export should disconnect too

[Sverre Rabbelier]

Now that the remote helper protocol uses the new done command in its
fast-import streams, export no longer needs to be the last command in
the stream.
---

  The fact that we had this before shows how messed up the protocol
  was earlier. Basically, any 'import' or 'export' command meant
  "you're done talking to the helper now".

 transport-helper.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 13ebb3b..1294d10 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -758,7 +758,6 @@ static int push_refs_with_export(struct transport *transport,
 			 export_marks, import_marks, &revlist_args))
 		die("Couldn't run fast-export");
 
-	data->no_disconnect_req = 1;
 	if(finish_command(&exporter))
 		die("Error while running fast-export");
 	push_update_refs_status(data, remote_refs);
-- 
1.7.2.1.240.g6a95c3

[PATCH 09/13] transport-helper: Use capname for gitdir capability too

[Sverre Rabbelier]

Also properly use capname in the refspec capability.

Previously the gitdir and refspec capabilities could not be listed as
required or their parsing would break.

CC: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
CC: Daniel Barkalow <barkalow@iabervon.org>
---

  The first hunk was real silly and I should have caught it while
  reviewing the patch that introduced the required capabilities.

  I suspect the reason the second hunk wasn't caught is because the
  series that added 'gitdir' as capability, and the one that added
  required capabilities were done in parallel.

 transport-helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 1294d10..82bdad3 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -171,10 +171,10 @@ static struct child_process *get_helper(struct transport *transport)
 			ALLOC_GROW(refspecs,
 				   refspec_nr + 1,
 				   refspec_alloc);
-			refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
+			refspecs[refspec_nr++] = strdup(capname + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
-		} else if (!strcmp(buf.buf, "gitdir")) {
+		} else if (!strcmp(capname, "gitdir")) {
 			struct strbuf gitdir = STRBUF_INIT;
 			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());
 			sendline(data, &gitdir);
-- 
1.7.2.1.240.g6a95c3

[PATCH 10/13] transport-helper: implement marks location as capability

[Sverre Rabbelier]

While this requires the helper to flush stdout after listing 'gitdir'
as capability, and read a command (the 'gitdir' response from the
remote helper infrastructure) right after that, this is more elegant
and does not require an ad-hoc exchange of values.

CC: Daniel Barkalow <barkalow@iabervon.org>
---

  Daniel made some fuss about the ad-hoc exchange when I first sent
  the 'export command' series for review, and it's been nagging me.

  As you can see in the remote-testgit implementation, it's a bit
  icky on the helper side (you have to flush sdout and read another
  command in the middle of responding to 'capabilities'), but I think
  it's better than the alternative.

 git-remote-testgit.py |   29 ++++++++++++++++-------------
 transport-helper.c    |   47 ++++++++++++++++++-----------------------------
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 50341ce..e2b213d 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -71,8 +71,24 @@ def do_capabilities(repo, args):
     print "import"
     print "export"
     print "gitdir"
+
+    sys.stdout.flush()
+    if not read_one_line(repo):
+        die("Expected gitdir, got empty line")
+
     print "refspec refs/heads/*:%s*" % repo.prefix
 
+    dirname = repo.get_base_path(repo.gitdir)
+
+    if not os.path.exists(dirname):
+        os.makedirs(dirname)
+
+    path = os.path.join(dirname, 'testgit.marks')
+
+    print "*export-marks %s" % path
+    if os.path.exists(path):
+        print "*import-marks %s" % path
+
     print # end capabilities
 
 
@@ -142,19 +158,6 @@ def do_export(repo, args):
     if not repo.gitdir:
         die("Need gitdir to export")
 
-    dirname = repo.get_base_path(repo.gitdir)
-
-    if not os.path.exists(dirname):
-        os.makedirs(dirname)
-
-    path = os.path.join(dirname, 'testgit.marks')
-    print path
-    if os.path.exists(path):
-        print path
-    else:
-        print ""
-    sys.stdout.flush()
-
     update_local_repo(repo)
     repo.importer.do_import(repo.gitdir)
     repo.non_local.push(repo.gitdir)
diff --git a/transport-helper.c b/transport-helper.c
index 82bdad3..0edc1d5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -23,6 +23,8 @@ struct helper_data
 		push : 1,
 		connect : 1,
 		no_disconnect_req : 1;
+	char *export_marks;
+	char *import_marks;
 	/* These go from remote name (as in "list") to private name */
 	struct refspec *refspecs;
 	int refspec_nr;
@@ -179,6 +181,16 @@ static struct child_process *get_helper(struct transport *transport)
 			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());
 			sendline(data, &gitdir);
 			strbuf_release(&gitdir);
+		} else if (!prefixcmp(capname, "export-marks ")) {
+			struct strbuf arg = STRBUF_INIT;
+			strbuf_addstr(&arg, "--export-marks=");
+			strbuf_addstr(&arg, capname + strlen("export-marks "));
+			data->export_marks = strbuf_detach(&arg, NULL);
+		} else if (!prefixcmp(capname, "import-marks")) {
+			struct strbuf arg = STRBUF_INIT;
+			strbuf_addstr(&arg, "--import-marks=");
+			strbuf_addstr(&arg, capname + strlen("import-marks "));
+			data->import_marks = strbuf_detach(&arg, NULL);
 		} else if (mandatory) {
 			die("Unknown mandatory capability %s. This remote "
 			    "helper probably needs newer version of Git.\n",
@@ -364,10 +376,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 
 static int get_exporter(struct transport *transport,
 			struct child_process *fastexport,
-			const char *export_marks,
-			const char *import_marks,
 			struct string_list *revlist_args)
 {
+	struct helper_data *data = transport->data;
 	struct child_process *helper = get_helper(transport);
 	int argc = 0, i;
 	memset(fastexport, 0, sizeof(*fastexport));
@@ -378,10 +389,10 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
 	fastexport->argv[argc++] = "--use-done-feature";
-	if (export_marks)
-		fastexport->argv[argc++] = export_marks;
-	if (import_marks)
-		fastexport->argv[argc++] = import_marks;
+	if (data->export_marks)
+		fastexport->argv[argc++] = data->export_marks;
+	if (data->import_marks)
+		fastexport->argv[argc++] = data->import_marks;
 
 	for (i = 0; i < revlist_args->nr; i++)
 		fastexport->argv[argc++] = revlist_args->items[i].string;
@@ -708,7 +719,6 @@ static int push_refs_with_export(struct transport *transport,
 	struct ref *ref;
 	struct child_process *helper, exporter;
 	struct helper_data *data = transport->data;
-	char *export_marks = NULL, *import_marks = NULL;
 	struct string_list revlist_args = { NULL, 0, 0 };
 	struct strbuf buf = STRBUF_INIT;
 
@@ -716,26 +726,6 @@ static int push_refs_with_export(struct transport *transport,
 
 	write_constant(helper->in, "export\n");
 
-	recvline(data, &buf);
-	if (debug)
-		fprintf(stderr, "Debug: Got export_marks '%s'\n", buf.buf);
-	if (buf.len) {
-		struct strbuf arg = STRBUF_INIT;
-		strbuf_addstr(&arg, "--export-marks=");
-		strbuf_addbuf(&arg, &buf);
-		export_marks = strbuf_detach(&arg, NULL);
-	}
-
-	recvline(data, &buf);
-	if (debug)
-		fprintf(stderr, "Debug: Got import_marks '%s'\n", buf.buf);
-	if (buf.len) {
-		struct strbuf arg = STRBUF_INIT;
-		strbuf_addstr(&arg, "--import-marks=");
-		strbuf_addbuf(&arg, &buf);
-		import_marks = strbuf_detach(&arg, NULL);
-	}
-
 	strbuf_reset(&buf);
 
 	for (ref = remote_refs; ref; ref = ref->next) {
@@ -754,8 +744,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	}
 
-	if (get_exporter(transport, &exporter,
-			 export_marks, import_marks, &revlist_args))
+	if (get_exporter(transport, &exporter, &revlist_args))
 		die("Couldn't run fast-export");
 
 	if(finish_command(&exporter))
-- 
1.7.2.1.240.g6a95c3

[PATCH 11/13] remote-curl: accept empty line as terminator

[Sverre Rabbelier]

The remote helper infrastructure terminates with a '\n', which the
remote-curl helper would interpret as a command to do '', a command
it did not understand. Consequently it would 'return 1'.

This went unnoticed because the transport helper infrastructure did
not check the return value of the helper, nor did the helper print
anything before exiting.

CC: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
CC: Daniel Barkalow <barkalow@iabervon.org>
---

  I noticed this when my tests suddenly broke. Bisecting pointed at
  the 'more rigorous return value checking' patch, after which some
  poking around in the remote-curl helper pointed this out as the
  problem.

  I'm not very sure about the error message, if anyone feels it
  should go (it indicates a bug in the remote helper infrastructure,
  not a user error) it can be left out as far as I'm concerned.

 remote-curl.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 04d4813..27fcd69 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -813,6 +813,8 @@ int main(int argc, const char **argv)
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF)
 			break;
+		if (buf.len == 0)
+			break;
 		if (!prefixcmp(buf.buf, "fetch ")) {
 			if (nongit)
 				die("Fetch attempted without a local repo");
@@ -851,6 +853,7 @@ int main(int argc, const char **argv)
 			printf("\n");
 			fflush(stdout);
 		} else {
+			fprintf(stderr, "Unknown command '%s'\n", buf.buf);
 			return 1;
 		}
 		strbuf_reset(&buf);
-- 
1.7.2.1.240.g6a95c3

[PATCH 12/13] git-remote-testgit: only push for non-local repositories

[Sverre Rabbelier]

Trying to push for local repositories will fail since there is no
local checkout in .git/info/... to push from.

This went unnoticed because the transport helper infrastructure did
not check the return value of the helper.
---

  I guess it also shows how many people look at the verbose output of
  the helper test suite ;-).

 git-remote-testgit.py |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index e2b213d..b428b1c 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -160,7 +160,9 @@ def do_export(repo, args):
 
     update_local_repo(repo)
     repo.importer.do_import(repo.gitdir)
-    repo.non_local.push(repo.gitdir)
+
+    if not repo.local:
+        repo.non_local.push(repo.gitdir)
 
     print "ok refs/heads/master"
     print
-- 
1.7.2.1.240.g6a95c3

[PATCH 13/13] git-remote-testgit: fix error handling

[Sverre Rabbelier]

If fast-export did not complete successfully the error handling code
itself would error out.
---

  *brown paper bag*

 git_remote_helpers/git/importer.py |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
index 70a7127..d938611 100644
--- a/git_remote_helpers/git/importer.py
+++ b/git_remote_helpers/git/importer.py
@@ -36,5 +36,6 @@ class GitImporter(object):
             args.append("--import-marks=" + path)
 
         child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        ret = child.wait()
+        if ret != 0:
+            raise subprocess.CalledProcessError(ret, args)
-- 
1.7.2.1.240.g6a95c3

Re: [PATCH 01/13] fast-import: add the 'done' command

[Daniel Barkalow]

On Sat, 28 Aug 2010, Sverre Rabbelier wrote:

> Currently the only way to end an import stream is to close it, which
> is not desirable when the stream that's being used is shared. For
> example, the remote helper infrastructure uses a pipe between it and
> the helper process, part of the protocol is to send a fast-import
> stream accross. Without a way to end the stream the remote helper
> infrastructure is forced to limit itself to have a command that uses
> a fast-import stream as it's last command.
> 
> Add a trivial 'done' command that causes fast-import to stop reading
> from the stream and exit.

Yeah, this is definitely worthwhile.

> ---
> 
>   Very straightforward. It is handled in parse_feature() instead of
>   in parse_one_feature() because I didn't want to allow '--done' as a
>   commandline argument. Allowing it would be silly, it surves no
>   other purpose than to indicate up front that the stream will
>   contain a 'done' command at the end.
> 
>   I'm fine too with dropping the feature and just adding the new
>   command, whichever is preferred.

I think the point of the feature would be to get the error response up 
front, where it might be easier to determine what to do about importers 
not supporting it. As such, I think the command line option actually makes 
at least as much sense, but it's probably not necessary anyway.

I believe there's a gfi mailing list, which ought to hear about this bit. 
Not that there are likely to be conflicts, but, when I was thinking about 
adding this command (for the same reason you're adding it), I'd called it 
"quit", so it's worth letting people know a de facto standard, so gfi 
implementations don't vary.

The code looks obviously good to me.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 02/13] fast-export: support done feature

[Daniel Barkalow]

On Sat, 28 Aug 2010, Sverre Rabbelier wrote:

> If fast-export is being used to generate a fast-import stream that
> will be used afterwards it is desirable to indicate the end of the
> stream with the new 'done' command.
> 
> Add a flag that causes fast-export to end with 'done'.

I was assuming that whatever passed the output from fast-export to 
fast-import would add the "done" itself when its fast-export child 
exitted. Obviously, if there's going to be anything after the gfi stream, 
something's going to have to write the next thing, and whatever that is 
can write the "done". Of course, the caller can't add the feature, so if 
the feature is necessary (and I don't remember all the possible 
interactions to say), this would be necessary.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 07/13] transport-helper: change import semantics

[Daniel Barkalow]

On Sat, 28 Aug 2010, Sverre Rabbelier 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.

I think your reasons for this change could be worked around, but the 
protocol is cleaner with your change, which is justification enough, given 
that it shouldn't be too big a deal to change. This also lets the helper 
consider all of the refs it is expected to update before producing the 
stream, which may simplify the stream (particularly if the history has 
merges involving branches that may or may not be imported are aren't 
listed first).

> Change the semantics of the import statement such that it matches
> that of the list statement. That is, 'import\n' is followed by a list
> of refs that should be exported, followed by '\n'.
> ---
> 
>   This changes the protcol a bit, but I don't think we have many
>   users of the 'import' command yet, and if we do I would assume
>   they're paying attention to development in the remote helper space.

I don't think "import" has gotten to the point where people could really 
use it in helpers not packaged with git, anyway, so I agree.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 08/13] transport-helper: export should disconnect too

[Daniel Barkalow]

On Sat, 28 Aug 2010, Sverre Rabbelier wrote:

> Now that the remote helper protocol uses the new done command in its
> fast-import streams, export no longer needs to be the last command in
> the stream.
> ---
> 
>   The fact that we had this before shows how messed up the protocol
>   was earlier. Basically, any 'import' or 'export' command meant
>   "you're done talking to the helper now".

Yup; this is a big improvement, and I'dhave done it this way in the first 
place, had I realized how easy it would be to get fast-import to have a 
"done" command. Your subject is backwards, I think, though; export won't 
require a disconnect.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 10/13] transport-helper: implement marks location as capability

[Daniel Barkalow]

On Sat, 28 Aug 2010, Sverre Rabbelier wrote:

> While this requires the helper to flush stdout after listing 'gitdir'
> as capability, and read a command (the 'gitdir' response from the
> remote helper infrastructure) right after that, this is more elegant
> and does not require an ad-hoc exchange of values.
> 
> CC: Daniel Barkalow <barkalow@iabervon.org>
> ---
> 
>   Daniel made some fuss about the ad-hoc exchange when I first sent
>   the 'export command' series for review, and it's been nagging me.
> 
>   As you can see in the remote-testgit implementation, it's a bit
>   icky on the helper side (you have to flush sdout and read another
>   command in the middle of responding to 'capabilities'), but I think
>   it's better than the alternative.

I think I was annoyed by it being ad-hoc, rather than having the exchange 
of values. I think if you need to get more information to the helper, you 
should have a generic mechanism for that, rather than anything that cares 
about the particular information involved.

I'm a bit unclear on what change you're making here; it looks like the 
helper side is reading another line, but that transport-helper isn't 
writing anything new, and you don't have any changes to the documentation 
here. Did this change get mixed into a different patch or something?

>  git-remote-testgit.py |   29 ++++++++++++++++-------------
>  transport-helper.c    |   47 ++++++++++++++++++-----------------------------
>  2 files changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 50341ce..e2b213d 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -71,8 +71,24 @@ def do_capabilities(repo, args):
>      print "import"
>      print "export"
>      print "gitdir"
> +
> +    sys.stdout.flush()
> +    if not read_one_line(repo):
> +        die("Expected gitdir, got empty line")
> +
>      print "refspec refs/heads/*:%s*" % repo.prefix
>  
> +    dirname = repo.get_base_path(repo.gitdir)
> +
> +    if not os.path.exists(dirname):
> +        os.makedirs(dirname)
> +
> +    path = os.path.join(dirname, 'testgit.marks')
> +
> +    print "*export-marks %s" % path
> +    if os.path.exists(path):
> +        print "*import-marks %s" % path
> +
>      print # end capabilities
>  
>  
> @@ -142,19 +158,6 @@ def do_export(repo, args):
>      if not repo.gitdir:
>          die("Need gitdir to export")
>  
> -    dirname = repo.get_base_path(repo.gitdir)
> -
> -    if not os.path.exists(dirname):
> -        os.makedirs(dirname)
> -
> -    path = os.path.join(dirname, 'testgit.marks')
> -    print path
> -    if os.path.exists(path):
> -        print path
> -    else:
> -        print ""
> -    sys.stdout.flush()
> -
>      update_local_repo(repo)
>      repo.importer.do_import(repo.gitdir)
>      repo.non_local.push(repo.gitdir)
> diff --git a/transport-helper.c b/transport-helper.c
> index 82bdad3..0edc1d5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -23,6 +23,8 @@ struct helper_data
>  		push : 1,
>  		connect : 1,
>  		no_disconnect_req : 1;
> +	char *export_marks;
> +	char *import_marks;
>  	/* These go from remote name (as in "list") to private name */
>  	struct refspec *refspecs;
>  	int refspec_nr;
> @@ -179,6 +181,16 @@ static struct child_process *get_helper(struct transport *transport)
>  			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());
>  			sendline(data, &gitdir);
>  			strbuf_release(&gitdir);
> +		} else if (!prefixcmp(capname, "export-marks ")) {
> +			struct strbuf arg = STRBUF_INIT;
> +			strbuf_addstr(&arg, "--export-marks=");
> +			strbuf_addstr(&arg, capname + strlen("export-marks "));
> +			data->export_marks = strbuf_detach(&arg, NULL);
> +		} else if (!prefixcmp(capname, "import-marks")) {
> +			struct strbuf arg = STRBUF_INIT;
> +			strbuf_addstr(&arg, "--import-marks=");
> +			strbuf_addstr(&arg, capname + strlen("import-marks "));
> +			data->import_marks = strbuf_detach(&arg, NULL);
>  		} else if (mandatory) {
>  			die("Unknown mandatory capability %s. This remote "
>  			    "helper probably needs newer version of Git.\n",
> @@ -364,10 +376,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti
>  
>  static int get_exporter(struct transport *transport,
>  			struct child_process *fastexport,
> -			const char *export_marks,
> -			const char *import_marks,
>  			struct string_list *revlist_args)
>  {
> +	struct helper_data *data = transport->data;
>  	struct child_process *helper = get_helper(transport);
>  	int argc = 0, i;
>  	memset(fastexport, 0, sizeof(*fastexport));
> @@ -378,10 +389,10 @@ static int get_exporter(struct transport *transport,
>  	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
>  	fastexport->argv[argc++] = "fast-export";
>  	fastexport->argv[argc++] = "--use-done-feature";
> -	if (export_marks)
> -		fastexport->argv[argc++] = export_marks;
> -	if (import_marks)
> -		fastexport->argv[argc++] = import_marks;
> +	if (data->export_marks)
> +		fastexport->argv[argc++] = data->export_marks;
> +	if (data->import_marks)
> +		fastexport->argv[argc++] = data->import_marks;
>  
>  	for (i = 0; i < revlist_args->nr; i++)
>  		fastexport->argv[argc++] = revlist_args->items[i].string;
> @@ -708,7 +719,6 @@ static int push_refs_with_export(struct transport *transport,
>  	struct ref *ref;
>  	struct child_process *helper, exporter;
>  	struct helper_data *data = transport->data;
> -	char *export_marks = NULL, *import_marks = NULL;
>  	struct string_list revlist_args = { NULL, 0, 0 };
>  	struct strbuf buf = STRBUF_INIT;
>  
> @@ -716,26 +726,6 @@ static int push_refs_with_export(struct transport *transport,
>  
>  	write_constant(helper->in, "export\n");
>  
> -	recvline(data, &buf);
> -	if (debug)
> -		fprintf(stderr, "Debug: Got export_marks '%s'\n", buf.buf);
> -	if (buf.len) {
> -		struct strbuf arg = STRBUF_INIT;
> -		strbuf_addstr(&arg, "--export-marks=");
> -		strbuf_addbuf(&arg, &buf);
> -		export_marks = strbuf_detach(&arg, NULL);
> -	}
> -
> -	recvline(data, &buf);
> -	if (debug)
> -		fprintf(stderr, "Debug: Got import_marks '%s'\n", buf.buf);
> -	if (buf.len) {
> -		struct strbuf arg = STRBUF_INIT;
> -		strbuf_addstr(&arg, "--import-marks=");
> -		strbuf_addbuf(&arg, &buf);
> -		import_marks = strbuf_detach(&arg, NULL);
> -	}
> -
>  	strbuf_reset(&buf);
>  
>  	for (ref = remote_refs; ref; ref = ref->next) {
> @@ -754,8 +744,7 @@ static int push_refs_with_export(struct transport *transport,
>  
>  	}
>  
> -	if (get_exporter(transport, &exporter,
> -			 export_marks, import_marks, &revlist_args))
> +	if (get_exporter(transport, &exporter, &revlist_args))
>  		die("Couldn't run fast-export");
>  
>  	if(finish_command(&exporter))
> -- 
> 1.7.2.1.240.g6a95c3
> 
> 

Re: [PATCH 10/13] transport-helper: implement marks location as capability

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 14:52, Daniel Barkalow <barkalow@iabervon.org> wrote:
> I think I was annoyed by it being ad-hoc, rather than having the exchange
> of values. I think if you need to get more information to the helper, you
> should have a generic mechanism for that, rather than anything that cares
> about the particular information involved.

Is the capability mechanism such as I used it now a good enough proxy for that?

> I'm a bit unclear on what change you're making here; it looks like the
> helper side is reading another line, but that transport-helper isn't
> writing anything new, and you don't have any changes to the documentation
> here. Did this change get mixed into a different patch or something?

Not at all. It's reading another command: the reply to the 'gitdir'
capability, being the gitdir command.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 01/13] fast-import: add the 'done' command

[Sverre Rabbelier]

[+vcs-fast-import-devs, not culled for their benefit]

On Sun, Aug 29, 2010 at 13:59, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Sat, 28 Aug 2010, Sverre Rabbelier wrote:
>> Currently the only way to end an import stream is to close it, which
>> is not desirable when the stream that's being used is shared. For
>> example, the remote helper infrastructure uses a pipe between it and
>> the helper process, part of the protocol is to send a fast-import
>> stream accross. Without a way to end the stream the remote helper
>> infrastructure is forced to limit itself to have a command that uses
>> a fast-import stream as it's last command.
>>
>> Add a trivial 'done' command that causes fast-import to stop reading
>> from the stream and exit.
>
> Yeah, this is definitely worthwhile.
>
>> ---
>>
>>   Very straightforward. It is handled in parse_feature() instead of
>>   in parse_one_feature() because I didn't want to allow '--done' as a
>>   commandline argument. Allowing it would be silly, it surves no
>>   other purpose than to indicate up front that the stream will
>>   contain a 'done' command at the end.
>>
>>   I'm fine too with dropping the feature and just adding the new
>>   command, whichever is preferred.
>
> I think the point of the feature would be to get the error response up
> front, where it might be easier to determine what to do about importers
> not supporting it. As such, I think the command line option actually makes
> at least as much sense, but it's probably not necessary anyway.
>
> I believe there's a gfi mailing list, which ought to hear about this bit.

I've added them.

> Not that there are likely to be conflicts, but, when I was thinking about
> adding this command (for the same reason you're adding it), I'd called it
> "quit", so it's worth letting people know a de facto standard, so gfi
> implementations don't vary.

Agreed, I've added it to the fastimport python library without much trouble

> The code looks obviously good to me.

Thanks.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 02/13] fast-export: support done feature

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 14:15, Daniel Barkalow <barkalow@iabervon.org> wrote:
> I was assuming that whatever passed the output from fast-export to
> fast-import would add the "done" itself when its fast-export child
> exitted.

I tried this, but it results in some unelegant code where you have to
make sure to flush before starting the exporter, etc. I thought in
general it was better to make one program responsible for the entire
stream.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 07/13] transport-helper: change import semantics

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 14:29, Daniel Barkalow <barkalow@iabervon.org> wrote:
> I think your reasons for this change could be worked around, but the
> protocol is cleaner with your change, which is justification enough, given
> that it shouldn't be too big a deal to change. This also lets the helper
> consider all of the refs it is expected to update before producing the
> stream, which may simplify the stream (particularly if the history has
> merges involving branches that may or may not be imported are aren't
> listed first).

Aye, that was also part of my motivation to do it this way (as opposed
to e.g. running fast-import multiple times).

> I don't think "import" has gotten to the point where people could really
> use it in helpers not packaged with git, anyway, so I agree.

Great :)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 08/13] transport-helper: export should disconnect too

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 14:32, Daniel Barkalow <barkalow@iabervon.org> wrote:
> Yup; this is a big improvement, and I'dhave done it this way in the first
> place, had I realized how easy it would be to get fast-import to have a
> "done" command. Your subject is backwards, I think, though; export won't
> require a disconnect.

Depends on how you look at it, the line this patch removes tells the
remote helper infrastructure not to issue a newline when disconnecting
(which was needed because the helper was already disconnected by that
time). On the other side though, you are right in that now the export
command no longer requires the helper to disconnect as part of the
export command.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 01/13] fast-import: add the 'done' command

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Add a trivial 'done' command that causes fast-import to stop reading
> from the stream and exit.

I like it.  

It is tempting to make the 'done' command mandatory when the "done"
feature is used, to prevent confusion from streams that are cut off
early.  What do frontends currently do to handle that?

Re: [PATCH 01/13] fast-import: add the 'done' command

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 16:24, Jonathan Nieder <jrnieder@gmail.com> wrote:
> It is tempting to make the 'done' command mandatory when the "done"
> feature is used, to prevent confusion from streams that are cut off
> early.  What do frontends currently do to handle that?

If the stream ends with an EOF at the end of a command, they would act
as if that was the end of the stream. If it ends mid-stream (e.g.,
while parsing a 'commit'), they would error out.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 03/13] transport-helper: factor out push_update_refs_status

[Jonathan Nieder]

Sverre Rabbelier wrote:

> +++ b/transport-helper.c
> @@ -554,6 +554,9 @@ static int fetch(struct transport *transport,
>  	return -1;
>  }
>  
> +static void push_update_refs_status(struct helper_data *data,
> +				       struct ref *remote_refs);
> +
>  static int push_refs_with_push(struct transport *transport,
>  		struct ref *remote_refs, int flags)
>  {
> @@ -609,8 +612,17 @@ static int push_refs_with_push(struct transport *transport,
[...]
> +static void push_update_refs_status(struct helper_data *data,
> +				    struct ref *remote_refs)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	struct ref *ref = remote_refs;
>  	while (1) {
>  		char *refname, *msg;
>  		int status;
> @@ -679,7 +691,7 @@ static int push_refs_with_push(struct transport *transport,
>  		ref->remote_status = msg;
>  	}

Hmm, I am not too happy with the long loop without explicit condition.
Maybe it would make sense to split out the loop body as its own function?
Something like

	struct ref *ref = remote_refs;
	for (;;) {
		recvline(data, &buf);
		if (!buf.len)
			break;

		push_update_ref_status(&buf, &ref, remote_refs);
	}

>  	strbuf_release(&buf);
> -	return 0;
> +	return;

Not necessary, I think.

>  }

Regardless, for what it's worth,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for a pleasant read.

Re: [PATCH 03/13] transport-helper: factor out push_update_refs_status

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 16:36, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hmm, I am not too happy with the long loop without explicit condition.
> Maybe it would make sense to split out the loop body as its own function?
> Something like
>
>        struct ref *ref = remote_refs;
>        for (;;) {
>                recvline(data, &buf);
>                if (!buf.len)
>                        break;
>
>                push_update_ref_status(&buf, &ref, remote_refs);
>        }

Ok, will fix.

>>       strbuf_release(&buf);
>> -     return 0;
>> +     return;
>
> Not necessary, I think.

Removed the return.

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks for a pleasant read.

Thanks for reading :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 04/13] transport-helper: check status code of finish_command

[Jonathan Nieder]

Sverre Rabbelier wrote:

> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -410,8 +412,11 @@ static int fetch_with_import(struct transport *transport,
>  		sendline(data, &buf);
>  		strbuf_reset(&buf);
>  	}
> -	disconnect_helper(transport);
> -	finish_command(&fastimport);
> +	if(disconnect_helper(transport))
> +		die("Error while disconnecting helper");
> +	if (finish_command(&fastimport))
> +		die("Error while running fast-import");

Nit: missing space after "if".

> +
>  	free(fastimport.argv);
>  	fastimport.argv = NULL;
>  
> @@ -751,8 +756,10 @@ static int push_refs_with_export(struct transport *transport,
>  		die("Couldn't run fast-export");
>  
>  	data->no_disconnect_req = 1;
> -	finish_command(&exporter);
> -	disconnect_helper(transport);
> +	if(finish_command(&exporter))
> +		die("Error while running fast-export");
> +	if(disconnect_helper(transport))

Likewise.

Thanks for this.  A test would be nice if someone has time to write
one.

Re: [PATCH 05/13] transport-helper: use the new done feature to properly do imports

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Previously, the helper code would disconnect the helper before
> starting fast-import. This was needed because there was no way to signal
> that the helper was done other than to close stdout (which it would
> do after importing iff the helper noticed it had been disconnected).
[...]
>   I really like what this does for the sanity of the import

Yeah, agreed.

> Instead, request that the fast-export uses the 'done' command
[...]
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -124,6 +124,8 @@ def do_import(repo, args):
>      repo = update_local_repo(repo)
>      repo.exporter.export_repo(repo.gitdir)
>  
> +    print "done"

I am probably not reading carefully enough, but I do not see what
this has to do with fast-export.  Is the patch actually about
something like this?

	Use the 'done' command where possible for remote
	helpers.

	In other words, use fast-export --use-done-feature to
	add a 'done' command at the end of streams passed to
	remote helpers' "import" commands, and teach the
	remote helpers implementing "export" to use the 'done'
	command in turn when producing their streams.

Re: [RFC PATCH 06/13] transport-helper: update ref status after push with export

[Jonathan Nieder]

Sverre Rabbelier wrote:

>   Obviously the testgit helper shouldn't just print 'ok' for master,
>   but it demonstrates the idea.

For those who (like me) wondered what it should do:

	When the push is complete, outputs one or more ok <dst> or
	error <dst> <why>?  lines to indicate success or failure of
	each pushed ref. The status report output is terminated by a
	blank line. The option field <why> may be quoted in a C style
	string if it contains an LF.

So I guess testgit should be getting this information from the
result of non_local.push().

Re: [PATCH 01/13] fast-import: add the 'done' command

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Sun, Aug 29, 2010 at 16:24, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> It is tempting to make the 'done' command mandatory when the "done"
>> feature is used, to prevent confusion from streams that are cut off
>> early.  What do frontends currently do to handle that?
>
> If the stream ends with an EOF at the end of a command, they would act
> as if that was the end of the stream. If it ends mid-stream (e.g.,
> while parsing a 'commit'), they would error out.

Okay, if the frontend is in control usually there would be some
nonzero exit code or signal; and if transport-helper is in control, I
think it would notice after your series.  I was just worried about
invocations like

 foo-fast-export | git fast-import

where an error might go undiagnosed (and any error message drowned out
by the summary fast-import writes at the end).

Will think more.

Re: [PATCH 02/13] fast-export: support done feature

[Tay Ray Chuan]

Hi,

On Sun, Aug 29, 2010 at 11:45 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> If fast-export is being used to generate a fast-import stream that
> will be used afterwards it is desirable to indicate the end of the
> stream with the new 'done' command.
>
> Add a flag that causes fast-export to end with 'done'.

For a user, what are the advantages of running it with the
--use-done-feature? Perhaps this should just be made a
non-configurable default (ie. always use it) to save the user from
some thinking.

-- 
Cheers,
Ray Chuan

Re: [PATCH 05/13] transport-helper: use the new done feature to properly do imports

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 17:02, Jonathan Nieder <jrnieder@gmail.com> wrote:
>        In other words, use fast-export --use-done-feature to
>        add a 'done' command at the end of streams passed to
>        remote helpers' "import" commands, and teach the
>        remote helpers implementing "export" to use the 'done'
>        command in turn when producing their streams.

Yes, that's a more accurate description, thanks.

-- 
Cheers,

Sverre Rabbelier

Re: [RFC PATCH 06/13] transport-helper: update ref status after push with export

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 17:25, Jonathan Nieder <jrnieder@gmail.com> wrote:
> So I guess testgit should be getting this information from the
> result of non_local.push().

Yes, or for example if a ref is a non-fast-forward, it should probably
detect that before even exporting it :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 01/13] fast-import: add the 'done' command

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 17:32, Jonathan Nieder <jrnieder@gmail.com> wrote:
> where an error might go undiagnosed (and any error message drowned out
> by the summary fast-import writes at the end).
>
> Will think more.

As far as I'm concerned that should be the responsibility of the
importer. If there is an error it should make sure not to drown the
error message with it's summary. I think it does a pretty good job at
that already though, doesn't it? It even saves a log file to try and
help you diagnose what went wrong.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 02/13] fast-export: support done feature

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 18:42, Tay Ray Chuan <rctay89@gmail.com> wrote:
> For a user, what are the advantages of running it with the
> --use-done-feature? Perhaps this should just be made a
> non-configurable default (ie. always use it) to save the user from
> some thinking.

No, that won't do, since not all importers will support this feature.
We might want to make it the default in the future (users can always
specify --no-use-done-feature), but it should definitely not be made
the default now.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 09/13] transport-helper: Use capname for gitdir capability too

[Jonathan Nieder]

Sverre Rabbelier wrote:

>   The first hunk was real silly and I should have caught it while
>   reviewing the patch that introduced the required capabilities.
> 
>   I suspect the reason the second hunk wasn't caught is because the
>   series that added 'gitdir' as capability, and the one that added
>   required capabilities were done in parallel.

Obviously good, and it looks to me like you caught all problems
of this kind.

Re: [PATCH 10/13] transport-helper: implement marks location as capability

[Jonathan Nieder]

Sverre Rabbelier wrote:

> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -71,8 +71,24 @@ def do_capabilities(repo, args):
>      print "import"
>      print "export"
>      print "gitdir"
> +
> +    sys.stdout.flush()
> +    if not read_one_line(repo):
> +        die("Expected gitdir, got empty line")

This seems fragile to me: shouldn't the remote helper check somehow
that the line it read was actually a gitdir line?

No other complaint on my part.  Requiring a flush seems entirely
appropriate to me, and if someone comes up with something nicer than
the "capabilities" sequence for requesting information, it would not
be the end of the world to have two ways to discover the .git dir.

Re: [PATCH 10/13] transport-helper: implement marks location as capability

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 20:31, Jonathan Nieder <jrnieder@gmail.com> wrote:
> This seems fragile to me: shouldn't the remote helper check somehow
> that the line it read was actually a gitdir line?

You're probably right, the simplest way would be to check if repo.gitdir is set.

> No other complaint on my part.  Requiring a flush seems entirely
> appropriate to me, and if someone comes up with something nicer than
> the "capabilities" sequence for requesting information, it would not
> be the end of the world to have two ways to discover the .git dir.

Agreed.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 11/13] remote-curl: accept empty line as terminator

[Jonathan Nieder]

Sverre Rabbelier wrote:

>   I noticed this when my tests suddenly broke. Bisecting pointed at
>   the 'more rigorous return value checking' patch

Shouldn't this go before "check status code of finish_command" for
bisectability, then?

>   I'm not very sure about the error message, if anyone feels it
>   should go (it indicates a bug in the remote helper infrastructure,
>   not a user error) it can be left out as far as I'm concerned.

No preference here.

> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -813,6 +813,8 @@ int main(int argc, const char **argv)
>  	do {
>  		if (strbuf_getline(&buf, stdin, '\n') == EOF)
>  			break;
> +		if (buf.len == 0)
> +			break;

This is just a bug, I think.  Other strbuf_getline() invocations in
that file all use the equivalent

	if (*buf->buf)
		break;

too.
 
> @@ -851,6 +853,7 @@ int main(int argc, const char **argv)
>  			printf("\n");
>  			fflush(stdout);
>  		} else {
> +			fprintf(stderr, "Unknown command '%s'\n", buf.buf);
>  			return 1;
>  		}

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories

[Jonathan Nieder]

Sverre Rabbelier wrote:
> Trying to push for local repositories will fail since there is no
> local checkout in .git/info/... to push from.
> 
> This went unnoticed because the transport helper infrastructure did
> not check the return value of the helper.
> ---
> 
>   I guess it also shows how many people look at the verbose output of
>   the helper test suite ;-).
[...]
> +++ b/git-remote-testgit.py
> @@ -160,7 +160,9 @@ def do_export(repo, args):
>  
>      update_local_repo(repo)
>      repo.importer.do_import(repo.gitdir)
> -    repo.non_local.push(repo.gitdir)
> +
> +    if not repo.local:
> +        repo.non_local.push(repo.gitdir)

[warning: I have not read through remote-testgit carefully]

Could you explain further?  I see

 ERROR: could not find repo at .git/info/fast-import/4dc49bf026b65e6a1b28e2457d4d6393af8d382c/.git

but I do not know why there should have been a repo there, or why we would
not want to do the equivalent of

 git push . refs/testgit/origin/refs/heads/master:refs/heads/master

Re: [PATCH 00/13] remote helper improvements

[Jonathan Nieder]

Sverre Rabbelier wrote:

> I had a week and then some stray days here and there to do some more
> work on git-remote-hg, the result of which is this series. It takes
> the 'import' and 'export' commands out of their 'toy' stage, and gets
> them ready for real usage.

Sign-off?

> Although 'git-remote-testgit' is still the
> only thing using them, 'git-remote-hg' is nearing completion, I hope
> to send out an RFC for it Real Soon Now (TM).

Very good to hear. :)

Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 20:48, Jonathan Nieder <jrnieder@gmail.com> wrote:
> [warning: I have not read through remote-testgit carefully]
>
> Could you explain further?  I see
>
>  ERROR: could not find repo at .git/info/fast-import/4dc49bf026b65e6a1b28e2457d4d6393af8d382c/.git

The repo in .git/info/... is only there iff the remote repo is not on
disk. If the remote _is_ on disk (i.e., repo.is_local),
`repo.importer.do_import(repo.gitdir)` will have directly updated the
remote. For remotes that are not on disk (i.e., not repo.is_local),
`repo.importer.do_import(repo.gitdir)` will have instead updated a
on-disk clone of the remote, which is stored in .git/info/...

So, to answer your question:

> but I do not know why there should have been a repo there

There should be a repo there only if the remote is not on disk.

> or why we would
> not want to do the equivalent of
>
>  git push . refs/testgit/origin/refs/heads/master:refs/heads/master

That isn't needed since the importer has already done that.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 00/13] remote helper improvements

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 20:53, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Sign-off?

The next round probably, I wanted feedback first.

> Very good to hear. :)

Aye, it'll be nice to have mercurial Just Work :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 11/13] remote-curl: accept empty line as terminator

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 20:39, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Shouldn't this go before "check status code of finish_command" for
> bisectability, then?

I guess so, the current code is already broken, but at least the tests pass now.

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 01/13] fast-import: add the 'done' command

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Sun, Aug 29, 2010 at 17:32, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> where an error might go undiagnosed (and any error message drowned out
>> by the summary fast-import writes at the end).
>>
>> Will think more.
>
> As far as I'm concerned that should be the responsibility of the
> importer. If there is an error it should make sure not to drown the
> error message with it's summary. I think it does a pretty good job at
> that already though, doesn't it? It even saves a log file to try and
> help you diagnose what went wrong.

I was thinking specifically of the case where one is unlucky enough
for the stream to end at a valid, early spot.

The way all importers seem to end up is to call "git fast-import" as a
child process (rather than advertising an interface like

	svnrdump dump <URI> | svn-fe | git fast-import

) so maybe this is not such a big deal.

Re: [PATCH 01/13] fast-import: add the 'done' command

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 21:02, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I was thinking specifically of the case where one is unlucky enough
> for the stream to end at a valid, early spot.

I think it makes sense to say that if you issue a 'feature done', we
change the code that checks for EOF to error instead of quit.

> The way all importers seem to end up is to call "git fast-import" as a
> child process (rather than advertising an interface like
>
>        svnrdump dump <URI> | svn-fe | git fast-import
>
> ) so maybe this is not such a big deal.

Does it matter much which way the importer is called? If it ends early
at a valid point nobody will know regardless of how it is called, no?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 12/13] git-remote-testgit: only push for non-local repositories

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Sun, Aug 29, 2010 at 20:48, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> or why we would
>> not want to do the equivalent of
>>
>>  git push . refs/testgit/origin/refs/heads/master:refs/heads/master
>
> That isn't needed since the importer has already done that.

Got it.  Thanks for the explanation.

Re: [PATCH 01/13] fast-import: add the 'done' command

[Jonathan Nieder]

[out of order for convenience]
Sverre Rabbelier wrote:

> Does it matter much which way the importer is called? If it ends early
> at a valid point nobody will know regardless of how it is called, no?

If the importer calls fast-import itself, it can

 1. close the pipe to fast-import
 2. wait for fast-import to exit
 3. print a relevant message
 4. exit

> I think it makes sense to say that if you issue a 'feature done', we
> change the code that checks for EOF to error instead of quit.

Ok. :)

Re: [PATCH 00/13] remote helper improvements

[Sverre Rabbelier]

Heya,

On Sun, Aug 29, 2010 at 22:08, a721018 <281422091@qq.com> wrote:

<snip spam>

> --
> View this message in context: http://git.661346.n2.nabble.com/PATCH-00-13-remote-helper-improvements-tp5474106p5476616.html
> Sent from the git mailing list archive at Nabble.com.

Junio, who maintains git@vger.kernel.org, is it Warthog (cc-ed)? Can
we please have this spam dealt with? I recall we're using some kind of
word/regexp based block list, I reckon it would do well with some shoe
related terms...

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 01/13] fast-import: add the 'done' command

[Jonathan Nieder]

Hi Sverre et al,

Sverre Rabbelier wrote:

> Currently the only way to end an import stream is to close it, which
> is not desirable when the stream that's being used is shared.

Here's a variation on the same theme, with notes indicating
what remains to be fixed.  Maybe it can save someone some time.

-- 8< --
From: Sverre Rabbelier <srabbelier@gmail.com>
Date: Sat, 28 Aug 2010 22:45:28 -0500
Subject: fast-import: introduce 'done' command

Add a 'done' command that causes fast-import to stop reading from the
stream and exit.

If the new --done command line flag was passed on the command line
(or a "feature done" declaration included at the start of the stream),
make the 'done' command mandatory.  So "git fast-import --done"'s
input format will be prefix-free, making errors easier to detect when
they show up as early termination at some convenient time of the
upstream of a pipe writing to fast-import.

Another possible application of the 'done' command would to be allow a
fast-import stream that is only a small part of a larger encapsulating
stream to be easily parsed, leaving the file offset after the "done\n"
so the other application can pick up from there.  This patch does not
teach fast-import to do that --- fast-import still uses buffered input
(stdio).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-fast-import.txt |   25 ++++++++++++++++++++++
 fast-import.c                     |   14 ++++++++++++
 t/t9300-fast-import.sh            |   42 +++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c3a2766..d0efdf8 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -101,6 +101,12 @@ OPTIONS
 	when the `cat-blob` command is encountered in the stream.
 	The default behaviour is to write to `stdout`.
 
+--done::
+	Require a `done` command at the end of the stream.
+	This option might be useful for detecting errors that
+	cause the frontend to terminate before it has started to
+	write a stream.
+
 --export-pack-edges=<file>::
 	After creating a packfile, print a line of data to
 	<file> listing the filename of the packfile and the last
@@ -329,6 +335,11 @@ and control the current import process.  More detailed discussion
 	standard output.  This command is optional and is not needed
 	to perform an import.
 
+`done`::
+	Marks the end of the stream. This command is optional
+	unless the `done` feature was requested using the
+	`--done` command line option or `feature done` command.
+
 `cat-blob`::
 	Causes fast-import to print a blob in 'cat-file --batch'
 	format to the file descriptor set with `--cat-blob-fd` or
@@ -958,6 +969,11 @@ notes::
 	Versions of fast-import not supporting notes will exit
 	with a message indicating so.
 
+done::
+	Error out if the stream ends without a 'done' command.
+	Without this feature, errors causing the frontend to end
+	abruptly at a convenient point in the stream can go
+	undetected.
 
 `option`
 ~~~~~~~~
@@ -987,6 +1003,15 @@ not be passed as option:
 * cat-blob-fd
 * force
 
+`done`
+~~~~~~
+If the `done` feature is not in use, treated as if EOF was read.
+This can be used to tell fast-import to finish early.
+
+If the `--done` command line option or `feature done` command is
+in use, the `done` command is mandatory and marks the end of the
+stream.
+
 Crash Reports
 -------------
 If fast-import is supplied invalid input it will terminate with a
diff --git a/fast-import.c b/fast-import.c
index 3886a1b..cbcf61f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -365,6 +365,7 @@ static unsigned int cmd_save = 100;
 static uintmax_t next_mark;
 static struct strbuf new_data = STRBUF_INIT;
 static int seen_data_command;
+static int require_explicit_termination;
 
 /* Signal handling */
 static volatile sig_atomic_t checkpoint_requested;
@@ -2999,6 +3000,8 @@ static int parse_one_feature(const char *feature, int from_stream)
 		relative_marks_paths = 1;
 	} else if (!prefixcmp(feature, "no-relative-marks")) {
 		relative_marks_paths = 0;
+	} else if (!strcmp(feature, "done")) {
+		require_explicit_termination = 1;
 	} else if (!prefixcmp(feature, "force")) {
 		force_update = 1;
 	} else if (!strcmp(feature, "notes")) {
@@ -3150,6 +3153,8 @@ int main(int argc, const char **argv)
 			parse_reset_branch();
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
+		else if (!strcmp("done", command_buf.buf))
+			break;
 		else if (!prefixcmp(command_buf.buf, "progress "))
 			parse_progress();
 		else if (!prefixcmp(command_buf.buf, "feature "))
@@ -3169,6 +3174,15 @@ int main(int argc, const char **argv)
 	if (!seen_data_command)
 		parse_argv();
 
+	/*
+	 * NEEDSWORK: we should report input errors before
+	 * errno has a chance to be clobbered.
+	 */
+	if (ferror(stdin))
+		die("error reading input");
+	if (require_explicit_termination && feof(stdin))
+		die("stream ends early");
+
 	end_packfile();
 
 	dump_branches();
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 52ac0e5..a366ee2 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2121,6 +2121,48 @@ test_expect_success 'R: quiet option results in no stats being output' '
     test_cmp empty output
 '
 
+test_expect_success 'R: feature done means terminating "done" is mandatory' '
+	echo feature done | test_must_fail git fast-import &&
+	test_must_fail git fast-import --done </dev/null
+'
+
+test_expect_success 'R: terminating "done" with trailing gibberish is ok' '
+	git fast-import <<-\EOF &&
+	feature done
+	done
+	trailing gibberish
+	EOF
+	git fast-import <<-\EOF
+	done
+	more trailing gibberish
+	EOF
+'
+
+test_expect_success 'R: terminating "done" within commit' '
+	cat >expect <<-\EOF &&
+	OBJID
+	:000000 100644 OBJID OBJID A	hello.c
+	:000000 100644 OBJID OBJID A	hello2.c
+	EOF
+	git fast-import <<-EOF &&
+	commit refs/heads/done-ends
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<EOT
+	Commit terminated by "done" command
+	EOT
+	M 100644 inline hello.c
+	data <<EOT
+	Hello, world.
+	EOT
+	C hello.c hello2.c
+	done
+	EOF
+	git rev-list done-ends |
+	git diff-tree -r --stdin --root --always |
+	sed -e "s/$_x40/OBJID/g" >actual &&
+	test_cmp expect actual
+'
+
 cat >input <<EOF
 option git non-existing-option
 EOF
-- 
1.7.4.1