[PATCH v2 v2 00/20] remote-helper improvements

[PATCH v2 v2 00/20] remote-helper improvements

[Sverre Rabbelier]

This is v2 of the series, mostly minor changes.

Jeff King (6):
  transport-helper: fix minor leak in push_refs_with_export
  t5800: factor out some ref tests
  t5800: document some non-functional parts of remote helpers
  teach remote-testgit to import non-HEAD refs
  transport-helper: don't feed bogus refs to export push
  git_remote_helpers: push all refs during a non-local export

Sverre Rabbelier (14):
  t5800: use skip_all instead of prereq
  remote-curl: accept empty line as terminator
  git-remote-testgit: only push for non-local repositories
  git-remote-testgit: fix error handling
  fast-import: introduce 'done' command
  fast-export: support done feature
  transport-helper: factor out push_update_refs_status
  transport-helper: check status code of finish_command
  transport-helper: use the new done feature where possible
  transport-helper: update ref status after push with export
  transport-helper: change import semantics
  transport-helper: export is no longer always the last command
  transport-helper: Use capname for gitdir capability too
  transport-helper: implement marks location as capability

 Documentation/git-fast-export.txt   |    4 +
 Documentation/git-fast-import.txt   |   25 ++++
 builtin/fast-export.c               |    9 ++
 fast-import.c                       |    8 ++
 git-remote-testgit.py               |   57 ++++++---
 git_remote_helpers/git/exporter.py  |   15 ++-
 git_remote_helpers/git/importer.py  |   32 +++++-
 git_remote_helpers/git/non_local.py |   20 +--
 git_remote_helpers/git/repo.py      |    7 +-
 git_remote_helpers/util.py          |   66 ++++++++++
 remote-curl.c                       |    3 +
 t/t5800-remote-helpers.sh           |   93 +++++++++++---
 t/t9300-fast-import.sh              |   42 ++++++
 transport-helper.c                  |  238 ++++++++++++++++++----------------
 14 files changed, 445 insertions(+), 174 deletions(-)

-- 
1.7.5.1.292.g728120

[PATCH v2 01/20] transport-helper: fix minor leak in push_refs_with_export

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>


Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

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

diff --git a/transport-helper.c b/transport-helper.c
index 660147f..b560b64 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -728,6 +728,7 @@ static int push_refs_with_export(struct transport *transport,
 			strbuf_addf(&buf, "^%s", private);
 			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
 		}
+		free(private);
 
 		string_list_append(&revlist_args, ref->name);
 
-- 
1.7.5.1.292.g728120

[PATCH v2 02/20] t5800: factor out some ref tests

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>

These are a little hard to read, and I'm about to add more
just like them. Plus the failure output is nicer if we use
test_cmp than a comparison with "test".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 t/t5800-remote-helpers.sh |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 1fb6380..3a37ad0 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -17,6 +17,12 @@ then
 	test_set_prereq PYTHON_24
 fi
 
+compare_refs() {
+	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
+	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success PYTHON_24 'setup repository' '
 	git init --bare server/.git &&
 	git clone server public &&
@@ -59,8 +65,7 @@ test_expect_success PYTHON_24 'pushing to local repo' '
 	echo content >>file &&
 	git commit -a -m three &&
 	git push) &&
-	HEAD=$(git --git-dir=localclone/.git rev-parse --verify HEAD) &&
-	test $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)
+	compare_refs localclone HEAD server HEAD
 '
 
 test_expect_success PYTHON_24 'synch with changes from localclone' '
@@ -73,8 +78,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	echo content >>file &&
 	git commit -a -m four &&
 	git push) &&
-	HEAD=$(git --git-dir=clone/.git rev-parse --verify HEAD) &&
-	test $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)
+	compare_refs clone HEAD server HEAD
 '
 
 test_done
-- 
1.7.5.1.292.g728120

[PATCH v2 03/20] t5800: use skip_all instead of prereq

[Sverre Rabbelier]

All tests require python 2.4 or higher.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  New in this series.

 t/t5800-remote-helpers.sh |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 3a37ad0..f6796e3 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -7,15 +7,19 @@ test_description='Test remote-helper import and export commands'
 
 . ./test-lib.sh
 
-if test_have_prereq PYTHON && "$PYTHON_PATH" -c '
+if ! test_have_prereq PYTHON ; then
+	skip_all='skipping git-remote-hg tests, python not available'
+	test_done
+fi
+
+"$PYTHON_PATH" -c '
 import sys
 if sys.hexversion < 0x02040000:
     sys.exit(1)
-'
-then
-    # Requires Python 2.4 or newer
-	test_set_prereq PYTHON_24
-fi
+' || {
+	skip_all='skipping git-remote-hg tests, python version < 2.4'
+	test_done
+}
 
 compare_refs() {
 	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
@@ -23,7 +27,7 @@ compare_refs() {
 	test_cmp expect actual
 }
 
-test_expect_success PYTHON_24 'setup repository' '
+test_expect_success 'setup repository' '
 	git init --bare server/.git &&
 	git clone server public &&
 	(cd public &&
@@ -33,34 +37,34 @@ test_expect_success PYTHON_24 'setup repository' '
 	 git push origin master)
 '
 
-test_expect_success PYTHON_24 'cloning from local repo' '
+test_expect_success 'cloning from local repo' '
 	git clone "testgit::${PWD}/server" localclone &&
 	test_cmp public/file localclone/file
 '
 
-test_expect_success PYTHON_24 'cloning from remote repo' '
+test_expect_success 'cloning from remote repo' '
 	git clone "testgit::file://${PWD}/server" clone &&
 	test_cmp public/file clone/file
 '
 
-test_expect_success PYTHON_24 'create new commit on remote' '
+test_expect_success 'create new commit on remote' '
 	(cd public &&
 	 echo content >>file &&
 	 git commit -a -m two &&
 	 git push)
 '
 
-test_expect_success PYTHON_24 'pulling from local repo' '
+test_expect_success 'pulling from local repo' '
 	(cd localclone && git pull) &&
 	test_cmp public/file localclone/file
 '
 
-test_expect_success PYTHON_24 'pulling from remote remote' '
+test_expect_success 'pulling from remote remote' '
 	(cd clone && git pull) &&
 	test_cmp public/file clone/file
 '
 
-test_expect_success PYTHON_24 'pushing to local repo' '
+test_expect_success 'pushing to local repo' '
 	(cd localclone &&
 	echo content >>file &&
 	git commit -a -m three &&
@@ -68,12 +72,12 @@ test_expect_success PYTHON_24 'pushing to local repo' '
 	compare_refs localclone HEAD server HEAD
 '
 
-test_expect_success PYTHON_24 'synch with changes from localclone' '
+test_expect_success 'synch with changes from localclone' '
 	(cd clone &&
 	 git pull)
 '
 
-test_expect_success PYTHON_24 'pushing remote local repo' '
+test_expect_success 'pushing remote local repo' '
 	(cd clone &&
 	echo content >>file &&
 	git commit -a -m four &&
-- 
1.7.5.1.292.g728120

[PATCH v2 04/20] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>

These are all things one might expect to work in a helper
that is capable of handling multiple branches (which our
testgit helper in theory should be able to do, as it is
backed by git). All of these bugs are specific to the
import/export codepaths, so they don't affect helpers like
git-remote-curl that use fetch/push commands.

The first and fourth tests are about fetching and pushing
new refs, and demonstrate bugs in the git_remote_helpers
library (so they would be most likely to impact helpers for
other VCSs which import/export git).

The second test is about importing multiple refs; it
demonstrates a bug in git-remote-testgit, which is mostly
for exercising the test code. Therefore it probably doesn't
affect anyone in practice.

The third test demonstrates a bug in git's side of the
helper code when the upstream has added refs that we do not
have locally. This could impact git users who use remote
helpers to access foreign VCSs.

All of those bugs have fixes later in this series.

The fifth test is the most complex, and does not have a fix
in this series. It tests pushing a ref via the export
mechanism to a new name on the remote side (i.e.,
"git push $remote old:new").

The problem is that we push all of the work of generating
the export stream onto fast-export, but we have no way of
communicating to fast-export that this name mapping is
happening. So we tell fast-export to generate a stream with
the commits for "old", but we can't tell it to label them
all as "new".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Culled the part of the commit message that is discussion of a
  possible implementation, but kept the parts describing the tests
  and why the last one is failing.

 t/t5800-remote-helpers.sh |   47 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index f6796e3..9db8ca8 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -85,4 +85,51 @@ test_expect_success 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
+test_expect_failure 'fetch new branch' '
+	(cd public &&
+	 git checkout -b new &&
+	 echo content >>file &&
+	 git commit -a -m five &&
+	 git push origin new
+	) &&
+	(cd localclone &&
+	 git fetch origin new
+	) &&
+	compare_refs public HEAD localclone FETCH_HEAD
+'
+
+test_expect_failure 'fetch multiple branches' '
+	(cd localclone &&
+	 git fetch
+	) &&
+	compare_refs server master localclone refs/remotes/origin/master &&
+	compare_refs server new localclone refs/remotes/origin/new
+'
+
+test_expect_failure 'push when remote has extra refs' '
+	(cd clone &&
+	 echo content >>file &&
+	 git commit -a -m six &&
+	 git push
+	) &&
+	compare_refs clone master server master
+'
+
+test_expect_failure 'push new branch by name' '
+	(cd clone &&
+	 git checkout -b new-name  &&
+	 echo content >>file &&
+	 git commit -a -m seven &&
+	 git push origin new-name
+	) &&
+	compare_refs clone HEAD server refs/heads/new-name
+'
+
+test_expect_failure 'push new branch with old:new refspec' '
+	(cd clone &&
+	 git push origin new-name:new-refspec
+	) &&
+	compare_refs clone HEAD server refs/heads/new-refspec
+'
+
 test_done
-- 
1.7.5.1.292.g728120

Re: [PATCH v2 04/20] t5800: document some non-functional parts of remote helpers

[Jonathan Nieder]

Sverre Rabbelier wrote:

>   Culled the part of the commit message that is discussion of a
>   possible implementation, but kept the parts describing the tests
>   and why the last one is failing.

Looks good.  What happened to the extra tests (e.g., push tag)
mentioned in the last round?

Re: [PATCH v2 04/20] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 00:02, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Looks good.  What happened to the extra tests (e.g., push tag)
> mentioned in the last round?

Do you think they should be added as test_expect_fail targets? I
didn't add them because they didn't work, but I suppose it might make
sense to at least document these missing features.

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 05/20] teach remote-testgit to import non-HEAD refs

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>

Upon receiving an "import" command, the testgit remote
helper would ignore the ref asked for by git and generate a
fast-export stream based on HEAD. Instead, we should
actually give git the ref it asked for.

This requires adding a new parameter to the export_repo
method in the remote-helpers python library, which may be
used by code outside of git.git. We use a default parameter
so that callers without the new parameter will get the same
behavior as before.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Fixed spurious appending of "HEAD" noticed by Junio.

 git-remote-testgit.py              |    2 +-
 git_remote_helpers/git/exporter.py |    9 +++++++--
 t/t5800-remote-helpers.sh          |    2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..e4a99a3 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -122,7 +122,7 @@ def do_import(repo, args):
         die("Need gitdir to import")
 
     repo = update_local_repo(repo)
-    repo.exporter.export_repo(repo.gitdir)
+    repo.exporter.export_repo(repo.gitdir, args)
 
 
 def do_export(repo, args):
diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
index f40f9d6..bc39163 100644
--- a/git_remote_helpers/git/exporter.py
+++ b/git_remote_helpers/git/exporter.py
@@ -15,7 +15,7 @@ class GitExporter(object):
 
         self.repo = repo
 
-    def export_repo(self, base):
+    def export_repo(self, base, refs=None):
         """Exports a fast-export stream for the given directory.
 
         Simply delegates to git fast-epxort and pipes it through sed
@@ -23,8 +23,13 @@ class GitExporter(object):
         default refs/heads. This is to demonstrate how the export
         data can be stored under it's own ref (using the refspec
         capability).
+
+        If None, refs defaults to ["HEAD"].
         """
 
+        if not refs:
+            refs = ["HEAD"]
+
         dirname = self.repo.get_base_path(base)
         path = os.path.abspath(os.path.join(dirname, 'testgit.marks'))
 
@@ -42,7 +47,7 @@ class GitExporter(object):
         if os.path.exists(path):
             args.append("--import-marks=" + path)
 
-        args.append("HEAD")
+        args.extend(refs)
 
         p1 = subprocess.Popen(args, stdout=subprocess.PIPE)
 
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 9db8ca8..ca115cc 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -85,7 +85,7 @@ test_expect_success 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
-test_expect_failure 'fetch new branch' '
+test_expect_success 'fetch new branch' '
 	(cd public &&
 	 git checkout -b new &&
 	 echo content >>file &&
-- 
1.7.5.1.292.g728120

[PATCH v2 06/20] transport-helper: don't feed bogus refs to export push

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>

When we want to push to a remote helper that has the
"export" capability, we collect all of the refs we want to
push and then feed them to fast-export.

However, the list of refs is actually a list of remote refs,
not local refs. The mapped local refs are included via the
peer_ref pointer. So when we add an argument to our
fast-export command line, we must be sure to use the local
peer_ref name (and if there is no local name, it is because
we are not actually sending that ref, or we may not even
have the ref at all).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 t/t5800-remote-helpers.sh |    2 +-
 transport-helper.c        |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index ca115cc..ceb0010 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -106,7 +106,7 @@ test_expect_failure 'fetch multiple branches' '
 	compare_refs server new localclone refs/remotes/origin/new
 '
 
-test_expect_failure 'push when remote has extra refs' '
+test_expect_success 'push when remote has extra refs' '
 	(cd clone &&
 	 echo content >>file &&
 	 git commit -a -m six &&
diff --git a/transport-helper.c b/transport-helper.c
index b560b64..34d18aa 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -730,7 +730,8 @@ static int push_refs_with_export(struct transport *transport,
 		}
 		free(private);
 
-		string_list_append(&revlist_args, ref->name);
+		if (ref->peer_ref)
+			string_list_append(&revlist_args, ref->peer_ref->name);
 
 	}
 
-- 
1.7.5.1.292.g728120

[PATCH v2 07/20] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

From: Jeff King <peff@peff.net>

When a remote helper exports to a non-local git repo, the
steps are roughly:

  1. fast-export into a local staging area; the set of
     interesting refs is defined by what is in the fast-export
     stream

  2. git push from the staging area to the non-local repo

In the second step, we should explicitly push all refs, not
just matching ones. This will let us push refs that do not
yet exist in the remote repo.

This does not deal with forced (not-fast-forward) pushes.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Added last line to the commit message.

 git_remote_helpers/git/non_local.py |    2 +-
 t/t5800-remote-helpers.sh           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git_remote_helpers/git/non_local.py b/git_remote_helpers/git/non_local.py
index f27389b..c53e074 100644
--- a/git_remote_helpers/git/non_local.py
+++ b/git_remote_helpers/git/non_local.py
@@ -63,7 +63,7 @@ class NonLocalGit(object):
         if not os.path.exists(path):
             die("could not find repo at %s", path)
 
-        args = ["git", "--git-dir=" + path, "push", "--quiet", self.repo.gitpath]
+        args = ["git", "--git-dir=" + path, "push", "--quiet", self.repo.gitpath, "--all"]
         child = subprocess.Popen(args)
         if child.wait() != 0:
             raise CalledProcessError
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index ceb0010..12f471c 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -115,7 +115,7 @@ test_expect_success 'push when remote has extra refs' '
 	compare_refs clone master server master
 '
 
-test_expect_failure 'push new branch by name' '
+test_expect_success 'push new branch by name' '
 	(cd clone &&
 	 git checkout -b new-name  &&
 	 echo content >>file &&
-- 
1.7.5.1.292.g728120

[PATCH v2 08/20] remote-curl: accept empty line as terminator

[Sverre Rabbelier]

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

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

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

diff --git a/remote-curl.c b/remote-curl.c
index 17d8a9b..7e8d50f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -857,6 +857,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");
@@ -895,6 +897,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.5.1.292.g728120

Re: [PATCH v2 08/20] remote-curl: accept empty line as terminator

[Jonathan Nieder]

Sverre Rabbelier wrote:

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

Thanks.  I wonder if that first "if" should be something like

	if (strbuf_getline(&buf, stdin, '\n') == EOF) {
		if (ferror(stdin))
			fprintf(stderr, "Error reading command stream\n");
		else
			fprintf(stderr, "Unexpected end of command stream\n");
		return 1;
	}

to catch I/O errors (e.g., the transport-helper exiting early).

Re: [PATCH v2 08/20] remote-curl: accept empty line as terminator

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 00:42, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thanks.  I wonder if that first "if" should be something like
>
>        if (strbuf_getline(&buf, stdin, '\n') == EOF) {
>                if (ferror(stdin))
>                        fprintf(stderr, "Error reading command stream\n");
>                else
>                        fprintf(stderr, "Unexpected end of command stream\n");
>                return 1;
>        }
>
> to catch I/O errors (e.g., the transport-helper exiting early).

Good idea, done. Tests still pass too ;).

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 08/20] remote-curl: accept empty line as terminator

[Dmitry Ivankov]

As a next step, won't it be better to use "exit\n" or "quit\n"
as a terminator? I don't see a convention of terminating on a
blank line in docs, only on EOF. Also I can imagine a blank
line being read in a case of communication error, I mean a
helper can miss the error and see "exit normally" terminator.

Re: [PATCH v2 08/20] remote-curl: accept empty line as terminator

[Jonathan Nieder]

Hi,

Dmitry Ivankov wrote:

> As a next step, won't it be better to use "exit\n" or "quit\n"
> as a terminator?

If we were starting over, a "done" command (or "quit" or some other
self-descriptive name) would be a no-brainer.  Unfortunately, there
are existing remote helpers out there --- based on a web search:

 - mediawiki
 - ccnx
 - couch
 - cvs
 - bzr
 - hg

Presumably most already ignore a final blank line and do not check for
"quit".

One possibility would be to emit "done" instead of a blank line if the
remote helper has declared a "done" capability.  But what does it buy?

Side note: a "done" capability doesn't sound like a bad idea, though,
for another reason.  The transport-helper could tell fast-import to
expect a "done" command at the end when importing from a remote helper
declaring it, to catch situations in which the pipe prematurely closes
(for example, because the remote helper has segfaulted).

> I don't see a convention of terminating on a
> blank line in docs,

Yes, this would be nice to document.

> only on EOF.  Also I can imagine a blank
> line being read in a case of communication error

A spurious NL, NL, EOF sequence does not sound likely to me.  If the
command stream is passing through a noisy channel, there are worse
corruptions to worry about (e.g., fetching to the wrong ref).

Re: [PATCH v2 08/20] remote-curl: accept empty line as terminator

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Side note: a "done" capability doesn't sound like a bad idea, though,
> for another reason.  The transport-helper could tell fast-import to
> expect a "done" command at the end when importing from a remote helper
> declaring it, to catch situations in which the pipe prematurely closes
> (for example, because the remote helper has segfaulted).
>
>> I don't see a convention of terminating on a
>> blank line in docs,
>
> Yes, this would be nice to document.
>
>> only on EOF.  Also I can imagine a blank
>> line being read in a case of communication error
>
> A spurious NL, NL, EOF sequence does not sound likely to me.  If the
> command stream is passing through a noisy channel, there are worse
> corruptions to worry about (e.g., fetching to the wrong ref).

I think everything you said in this message makes sense, especially the
part about "premature EOF detection".

The spurious LF is an issue if you are trying to drive the backend by hand
for testing, but otherwise probably not.

[PATCH v2 09/20] 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 as that is only used for
non-local repositories (local repositories are pushed to directly).

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

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 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 e4a99a3..9658355 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -147,7 +147,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)
 
 
 def do_gitdir(repo, args):
-- 
1.7.5.1.292.g728120

[PATCH v2 10/20] git-remote-testgit: fix error handling

[Sverre Rabbelier]

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

This was broken in commit 23b093ee0 (Brandon Casey, Wed Jun 9 2010,
Remove python 2.5'isms). Revert that commit an introduce our own copy
of check_call in util.py instead.

Tested by changing 'if retcode' to 'if not retcode' temporarily.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  This was broken in many places, not just the one place I fixed it
  in the last version of this commit.

 git_remote_helpers/git/exporter.py  |    6 +++---
 git_remote_helpers/git/importer.py  |    6 +++---
 git_remote_helpers/git/non_local.py |   18 +++++-------------
 git_remote_helpers/git/repo.py      |    7 ++++---
 git_remote_helpers/util.py          |   32 ++++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
index bc39163..9ee5f96 100644
--- a/git_remote_helpers/git/exporter.py
+++ b/git_remote_helpers/git/exporter.py
@@ -2,6 +2,8 @@ import os
 import subprocess
 import sys
 
+from git_remote_helpers.util import check_call
+
 
 class GitExporter(object):
     """An exporter for testgit repositories.
@@ -53,6 +55,4 @@ class GitExporter(object):
 
         args = ["sed", "s_refs/heads/_" + self.repo.prefix + "_g"]
 
-        child = subprocess.Popen(args, stdin=p1.stdout)
-        if child.wait() != 0:
-            raise CalledProcessError
+        check_call(args, stdin=p1.stdout)
diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
index 70a7127..02a719a 100644
--- a/git_remote_helpers/git/importer.py
+++ b/git_remote_helpers/git/importer.py
@@ -1,6 +1,8 @@
 import os
 import subprocess
 
+from git_remote_helpers.util import check_call
+
 
 class GitImporter(object):
     """An importer for testgit repositories.
@@ -35,6 +37,4 @@ class GitImporter(object):
         if os.path.exists(path):
             args.append("--import-marks=" + path)
 
-        child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        check_call(args)
diff --git a/git_remote_helpers/git/non_local.py b/git_remote_helpers/git/non_local.py
index c53e074..e700250 100644
--- a/git_remote_helpers/git/non_local.py
+++ b/git_remote_helpers/git/non_local.py
@@ -1,7 +1,7 @@
 import os
 import subprocess
 
-from git_remote_helpers.util import die, warn
+from git_remote_helpers.util import check_call, die, warn
 
 
 class NonLocalGit(object):
@@ -29,9 +29,7 @@ class NonLocalGit(object):
         os.makedirs(path)
         args = ["git", "clone", "--bare", "--quiet", self.repo.gitpath, path]
 
-        child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        check_call(args)
 
         return path
 
@@ -45,14 +43,10 @@ class NonLocalGit(object):
             die("could not find repo at %s", path)
 
         args = ["git", "--git-dir=" + path, "fetch", "--quiet", self.repo.gitpath]
-        child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        check_call(args)
 
         args = ["git", "--git-dir=" + path, "update-ref", "refs/heads/master", "FETCH_HEAD"]
-        child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        child = check_call(args)
 
     def push(self, base):
         """Pushes from the non-local repo to base.
@@ -64,6 +58,4 @@ class NonLocalGit(object):
             die("could not find repo at %s", path)
 
         args = ["git", "--git-dir=" + path, "push", "--quiet", self.repo.gitpath, "--all"]
-        child = subprocess.Popen(args)
-        if child.wait() != 0:
-            raise CalledProcessError
+        child = check_call(args)
diff --git a/git_remote_helpers/git/repo.py b/git_remote_helpers/git/repo.py
index 58e1cdb..acbf8d7 100644
--- a/git_remote_helpers/git/repo.py
+++ b/git_remote_helpers/git/repo.py
@@ -1,6 +1,9 @@
 import os
 import subprocess
 
+from git_remote_helpers.util import check_call
+
+
 def sanitize(rev, sep='\t'):
     """Converts a for-each-ref line to a name/value pair.
     """
@@ -53,9 +56,7 @@ class GitRepo(object):
         path = ".cached_revs"
         ofile = open(path, "w")
 
-        child = subprocess.Popen(args, stdout=ofile)
-        if child.wait() != 0:
-            raise CalledProcessError
+        check_call(args, stdout=ofile)
         output = open(path).readlines()
         self.revmap = dict(sanitize(i) for i in output)
         if "HEAD" in self.revmap:
diff --git a/git_remote_helpers/util.py b/git_remote_helpers/util.py
index dce83e6..8b9f302 100644
--- a/git_remote_helpers/util.py
+++ b/git_remote_helpers/util.py
@@ -128,6 +128,38 @@ def run_command (args, cwd = None, shell = False, add_env = None,
     return (exit_code, output, errors)
 
 
+# from python2.7:subprocess.py
+def call(*popenargs, **kwargs):
+    """Run command with arguments.  Wait for command to complete, then
+    return the returncode attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    retcode = call(["ls", "-l"])
+    """
+    return subprocess.Popen(*popenargs, **kwargs).wait()
+
+
+# from python2.7:subprocess.py
+def check_call(*popenargs, **kwargs):
+    """Run command with arguments.  Wait for command to complete.  If
+    the exit code was zero then return, otherwise raise
+    CalledProcessError.  The CalledProcessError object will have the
+    return code in the returncode attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    check_call(["ls", "-l"])
+    """
+    retcode = call(*popenargs, **kwargs)
+    if retcode:
+        cmd = kwargs.get("args")
+        if cmd is None:
+            cmd = popenargs[0]
+        raise subprocess.CalledProcessError(retcode, cmd)
+    return 0
+
+
 def file_reader_method (missing_ok = False):
     """Decorator for simplifying reading of files.
 
-- 
1.7.5.1.292.g728120

Re: [PATCH v2 10/20] git-remote-testgit: fix error handling

[Jonathan Nieder]

Hi,

Sverre Rabbelier wrote:

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

That sounds like a problem indeed.  What error message does it produce?

> This was broken in commit 23b093ee0 (Brandon Casey, Wed Jun 9 2010,
> Remove python 2.5'isms). Revert that commit an introduce our own copy
> of check_call in util.py instead.

The "an" should probably be "and".  More importantly, it's probably
worth mentioning that this is only a partial revert or rephrasing some
other way.

Have you checked if this still works with python 2.4?  Cc-ing Brandon
in case he has advice.

[...]
> +++ b/git_remote_helpers/git/exporter.py
[...]
> @@ -53,6 +55,4 @@ class GitExporter(object):
>  
>          args = ["sed", "s_refs/heads/_" + self.repo.prefix + "_g"]
>  
> -        child = subprocess.Popen(args, stdin=p1.stdout)
> -        if child.wait() != 0:
> -            raise CalledProcessError
> +        check_call(args, stdin=p1.stdout)
[...]
> +++ b/git_remote_helpers/util.py
> @@ -128,6 +128,38 @@ def run_command (args, cwd = None, shell = False, add_env = None,
>      return (exit_code, output, errors)
>  
>  
> +# from python2.7:subprocess.py
> +def call(*popenargs, **kwargs):
[...]
> +    return subprocess.Popen(*popenargs, **kwargs).wait()
> +
> +
> +# from python2.7:subprocess.py
> +def check_call(*popenargs, **kwargs):
[...]
> +    retcode = call(*popenargs, **kwargs)
> +    if retcode:
> +        cmd = kwargs.get("args")
> +        if cmd is None:
> +            cmd = popenargs[0]
> +        raise subprocess.CalledProcessError(retcode, cmd)
> +    return 0

So IIUC the existing code is not providing arguments to the
CalledProcessError constructor, and your patch fixes that.  Good.

Based on <http://pydoc.org/2.4.1/subprocess.html>, Python 2.4 doesn't
seem to have a CalledProcessError type.  Maybe these code paths
weren't being exercised before.

Regards,
Jonathan

Re: [PATCH v2 10/20] git-remote-testgit: fix error handling

[Brandon Casey]

On 06/19/2011 05:58 PM, Jonathan Nieder wrote:

> Sverre Rabbelier wrote:
> 
> Have you checked if this still works with python 2.4?  Cc-ing Brandon
> in case he has advice.
> 

> Based on <http://pydoc.org/2.4.1/subprocess.html>, Python 2.4 doesn't
> seem to have a CalledProcessError type.  Maybe these code paths
> weren't being exercised before.

Well, I don't see anywhere where CalledProccessError is actually caught.
i.e. I don't see

   except subprocess.CalledProcessError:

anywhere.  So, on python 2.5+ if this exception is ever raised, the
script would just exit and produce a backtrace right?  And the last
line would look something like this:

     File "XXX", line XX, in check_call
       raise subprocess.CalledProcessError(retcode, cmd)
   subprocess.CalledProcessError: Command '['XXX', 'YYY', 'ZZZ']' returned non-zero exit status 1

On python 2.4, it would also exit and produce a backtrace that
looks like this:

     File "test.py", line 11, in check_call
       raise subprocess.CalledProcessError(retcode, cmd)
   AttributeError: 'module' object has no attribute 'CalledProcessError'

So, maybe it would work on python 2.4, but for the wrong reasons.
Since the CalledProcessError is never caught, couldn't we just
print an error message and exit(1) within Sverre's check_call
implementation?

Btw. the only reason I submitted those changes to support python 2.4
was because RHEL 5.X ships with python 2.4, and the changes were not
too intrusive.  So, it should be considered whether supporting 2.4
is desirable.  I wouldn't want to increase the maintenance burden
on the real python developers (i.e. not me).

Also, sorry about introducing this bug.  I'm definitely no python guru.

-Brandon

Re: [PATCH v2 10/20] git-remote-testgit: fix error handling

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 19:50, Brandon Casey
<brandon.casey.ctr@nrlssc.navy.mil> wrote:
> Well, I don't see anywhere where CalledProccessError is actually caught.
> i.e. I don't see
>
>   except subprocess.CalledProcessError:
>
> anywhere.  So, on python 2.5+ if this exception is ever raised, the
> script would just exit and produce a backtrace right?

We're not trying to catch it, but we raise it (since that's what the
python 2.5 implementation of check_call does). But if it's not defined
in python 2.4, then my patch broke 2.4 again :).

> On python 2.4, it would also exit and produce a backtrace that
> looks like this:
>
>     File "test.py", line 11, in check_call
>       raise subprocess.CalledProcessError(retcode, cmd)
>   AttributeError: 'module' object has no attribute 'CalledProcessError'

Yeah, it would, my bad :(. I'll just define a dummy CalledProcessError
if it isn't defined to support python 2.4

> Btw. the only reason I submitted those changes to support python 2.4
> was because RHEL 5.X ships with python 2.4, and the changes were not
> too intrusive.  So, it should be considered whether supporting 2.4
> is desirable.  I wouldn't want to increase the maintenance burden
> on the real python developers (i.e. not me).

Wikipedia says RHEL 5.x will ready End of Production 1 on Q4 2011, but
RHEL 6.x has been out only a little over half a year, so it's probably
worth doing.

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 11/20] fast-import: introduce 'done' command

[Sverre Rabbelier]

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>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 Documentation/git-fast-import.txt |   25 ++++++++++++++++++++++
 fast-import.c                     |    8 +++++++
 t/t9300-fast-import.sh            |   42 +++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 249249a..0fc68a9 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
@@ -330,6 +336,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
@@ -1015,6 +1026,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`
 ~~~~~~~~
@@ -1044,6 +1060,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 78d9786..8a8a915 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -354,6 +354,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;
@@ -3139,6 +3140,8 @@ static int parse_one_feature(const char *feature, int from_stream)
 		relative_marks_paths = 1;
 	} else if (!strcmp(feature, "no-relative-marks")) {
 		relative_marks_paths = 0;
+	} else if (!strcmp(feature, "done")) {
+		require_explicit_termination = 1;
 	} else if (!strcmp(feature, "force")) {
 		force_update = 1;
 	} else if (!strcmp(feature, "notes") || !strcmp(feature, "ls")) {
@@ -3288,6 +3291,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 "))
@@ -3307,6 +3312,9 @@ int main(int argc, const char **argv)
 	if (!seen_data_command)
 		parse_argv();
 
+	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 6b1ba6c..526231b 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2197,6 +2197,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.5.1.292.g728120

[PATCH v2 12/20] 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'.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 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 781bd6e..e3f8453 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 daf1945..becef85 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 full_tree;
 
@@ -627,6 +628,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			     "Fake a tagger when tags lack one"),
 		OPT_BOOLEAN(0, "full-tree", &full_tree,
 			     "Output full tree for each commit"),
+		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 },
@@ -648,6 +651,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);
 
@@ -675,5 +681,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.5.1.292.g728120

[PATCH v2 13/20] 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.

Also factor out push_update_ref_status to avoid a long loop without
an explicit condition with a non-trivial body.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 transport-helper.c |  153 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 84 insertions(+), 69 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 34d18aa..ecb44f6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -554,6 +554,88 @@ static int fetch(struct transport *transport,
 	return -1;
 }
 
+static void push_update_ref_status(struct strbuf *buf,
+				   struct ref **ref,
+				   struct ref *remote_refs)
+{
+	char *refname, *msg;
+	int status;
+
+	if (!prefixcmp(buf->buf, "ok ")) {
+		status = REF_STATUS_OK;
+		refname = buf->buf + 3;
+	} else if (!prefixcmp(buf->buf, "error ")) {
+		status = REF_STATUS_REMOTE_REJECT;
+		refname = buf->buf + 6;
+	} else
+		die("expected ok/error, helper said '%s'\n", buf->buf);
+
+	msg = strchr(refname, ' ');
+	if (msg) {
+		struct strbuf msg_buf = STRBUF_INIT;
+		const char *end;
+
+		*msg++ = '\0';
+		if (!unquote_c_style(&msg_buf, msg, &end))
+			msg = strbuf_detach(&msg_buf, NULL);
+		else
+			msg = xstrdup(msg);
+		strbuf_release(&msg_buf);
+
+		if (!strcmp(msg, "no match")) {
+			status = REF_STATUS_NONE;
+			free(msg);
+			msg = NULL;
+		}
+		else if (!strcmp(msg, "up to date")) {
+			status = REF_STATUS_UPTODATE;
+			free(msg);
+			msg = NULL;
+		}
+		else if (!strcmp(msg, "non-fast forward")) {
+			status = REF_STATUS_REJECT_NONFASTFORWARD;
+			free(msg);
+			msg = NULL;
+		}
+	}
+
+	if (*ref)
+		*ref = find_ref_by_name(*ref, refname);
+	if (!*ref)
+		*ref = find_ref_by_name(remote_refs, refname);
+	if (!*ref) {
+		warning("helper reported unexpected status of %s", refname);
+		return;
+	}
+
+	if ((*ref)->status != REF_STATUS_NONE) {
+		/*
+		 * Earlier, the ref was marked not to be pushed, so ignore the ref
+		 * status reported by the remote helper if the latter is 'no match'.
+		 */
+		if (status == REF_STATUS_NONE)
+			return;
+	}
+
+	(*ref)->status = status;
+	(*ref)->remote_status = msg;
+}
+
+static void push_update_refs_status(struct helper_data *data,
+				    struct ref *remote_refs)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct ref *ref = remote_refs;
+	for (;;) {
+		recvline(data, &buf);
+		if (!buf.len)
+			break;
+
+		push_update_ref_status(&buf, &ref, remote_refs);
+	}
+	strbuf_release(&buf);
+}
+
 static int push_refs_with_push(struct transport *transport,
 		struct ref *remote_refs, int flags)
 {
@@ -608,76 +690,9 @@ static int push_refs_with_push(struct transport *transport,
 
 	strbuf_addch(&buf, '\n');
 	sendline(data, &buf);
-
-	ref = remote_refs;
-	while (1) {
-		char *refname, *msg;
-		int status;
-
-		recvline(data, &buf);
-		if (!buf.len)
-			break;
-
-		if (!prefixcmp(buf.buf, "ok ")) {
-			status = REF_STATUS_OK;
-			refname = buf.buf + 3;
-		} else if (!prefixcmp(buf.buf, "error ")) {
-			status = REF_STATUS_REMOTE_REJECT;
-			refname = buf.buf + 6;
-		} else
-			die("expected ok/error, helper said '%s'\n", buf.buf);
-
-		msg = strchr(refname, ' ');
-		if (msg) {
-			struct strbuf msg_buf = STRBUF_INIT;
-			const char *end;
-
-			*msg++ = '\0';
-			if (!unquote_c_style(&msg_buf, msg, &end))
-				msg = strbuf_detach(&msg_buf, NULL);
-			else
-				msg = xstrdup(msg);
-			strbuf_release(&msg_buf);
-
-			if (!strcmp(msg, "no match")) {
-				status = REF_STATUS_NONE;
-				free(msg);
-				msg = NULL;
-			}
-			else if (!strcmp(msg, "up to date")) {
-				status = REF_STATUS_UPTODATE;
-				free(msg);
-				msg = NULL;
-			}
-			else if (!strcmp(msg, "non-fast forward")) {
-				status = REF_STATUS_REJECT_NONFASTFORWARD;
-				free(msg);
-				msg = NULL;
-			}
-		}
-
-		if (ref)
-			ref = find_ref_by_name(ref, refname);
-		if (!ref)
-			ref = find_ref_by_name(remote_refs, refname);
-		if (!ref) {
-			warning("helper reported unexpected status of %s", refname);
-			continue;
-		}
-
-		if (ref->status != REF_STATUS_NONE) {
-			/*
-			 * Earlier, the ref was marked not to be pushed, so ignore the ref
-			 * status reported by the remote helper if the latter is 'no match'.
-			 */
-			if (status == REF_STATUS_NONE)
-				continue;
-		}
-
-		ref->status = status;
-		ref->remote_status = msg;
-	}
 	strbuf_release(&buf);
+
+	push_update_refs_status(data, remote_refs);
 	return 0;
 }
 
-- 
1.7.5.1.292.g728120

[PATCH v2 14/20] 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.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

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

diff --git a/transport-helper.c b/transport-helper.c
index ecb44f6..7f3c6c6 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;
 
@@ -755,8 +760,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.5.1.292.g728120

[PATCH v2 15/20] transport-helper: use the new done feature where possible

[Sverre Rabbelier]

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.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 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 9658355..a8e47d9 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, args)
 
+    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 7f3c6c6..b0361c2 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;
 
@@ -762,8 +760,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.5.1.292.g728120

Re: [PATCH v2 15/20] transport-helper: use the new done feature where possible

[Jonathan Nieder]

Hi,

Sverre Rabbelier 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.

Sounds like a reasonable thing to do.  One puzzle:

[...]
> +++ 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");
> -

What is this change about?  Is the plan to allow other commands after
a fetch_with_import?  Sounds reasonable; I think it should be
advertised in the log message, though.

When does the disconnect_helper call happen (to avoid leaks)?  Ah, in
release_helper; phew.

The disconnect_helper call writes the blank line that terminates the
list of "import %s" commands to start the import, so there would need
to be a

	strbuf_reset(&buf);
	strbuf_addf(&buf, "\n");
	sendline(data, &buf);

in its place.

Re: [PATCH v2 15/20] transport-helper: use the new done feature where possible

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> @@ -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");
>> -
>
> What is this change about?  Is the plan to allow other commands after
> a fetch_with_import?  Sounds reasonable; I think it should be
> advertised in the log message, though.
>
> When does the disconnect_helper call happen (to avoid leaks)?  Ah, in
> release_helper; phew.
>
> The disconnect_helper call writes the blank line that terminates the
> list of "import %s" commands to start the import, so there would need
> to be a
>
> 	strbuf_reset(&buf);
> 	strbuf_addf(&buf, "\n");
> 	sendline(data, &buf);
>
> in its place.

All sensible suggestions for a potential reroll.

Re: [PATCH v2 15/20] transport-helper: use the new done feature where possible

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 13:45, Jonathan Nieder <jrnieder@gmail.com> wrote:
> What is this change about?  Is the plan to allow other commands after
> a fetch_with_import?  Sounds reasonable; I think it should be
> advertised in the log message, though.

Yeah, the order/granularity of these commits is perhaps suboptimal.
The actual changes is by now explained in "export is no longer always
the last command" and "change import semantics". Perhaps they should
just be squashed together?

> The disconnect_helper call writes the blank line that terminates the
> list of "import %s" commands to start the import, so there would need

Terminating the list of import commands isn't needed until the "change
import semantics" commit, in which a write_constant(data->helper->in,
"\n") is added.

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 16/20] transport-helper: update ref status after push with export

[Sverre Rabbelier]

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  This time a proper implementation of this patch.

 git-remote-testgit.py              |    6 +++++-
 git_remote_helpers/git/importer.py |   28 +++++++++++++++++++++++++++-
 git_remote_helpers/util.py         |   34 ++++++++++++++++++++++++++++++++++
 transport-helper.c                 |    1 +
 4 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index a8e47d9..854e27e 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -148,11 +148,15 @@ def do_export(repo, args):
     sys.stdout.flush()
 
     update_local_repo(repo)
-    repo.importer.do_import(repo.gitdir)
+    changed = repo.importer.do_import(repo.gitdir)
 
     if not repo.local:
         repo.non_local.push(repo.gitdir)
 
+    for ref in changed:
+        print "ok %s" % ref
+    print
+
 
 def do_gitdir(repo, args):
     """Stores the location of the gitdir.
diff --git a/git_remote_helpers/git/importer.py b/git_remote_helpers/git/importer.py
index 02a719a..5c6b595 100644
--- a/git_remote_helpers/git/importer.py
+++ b/git_remote_helpers/git/importer.py
@@ -1,7 +1,7 @@
 import os
 import subprocess
 
-from git_remote_helpers.util import check_call
+from git_remote_helpers.util import check_call, check_output
 
 
 class GitImporter(object):
@@ -16,6 +16,18 @@ class GitImporter(object):
 
         self.repo = repo
 
+    def get_refs(self, gitdir):
+        """Returns a dictionary with refs.
+        """
+        args = ["git", "--git-dir=" + gitdir, "for-each-ref", "refs/heads"]
+        lines = check_output(args).strip().split('\n')
+        refs = {}
+        for line in lines:
+            value, name = line.split(' ')
+            name = name.strip('commit\t')
+            refs[name] = value
+        return refs
+
     def do_import(self, base):
         """Imports a fast-import stream to the given directory.
 
@@ -32,9 +44,23 @@ class GitImporter(object):
         if not os.path.exists(dirname):
             os.makedirs(dirname)
 
+        refs_before = self.get_refs(gitdir)
+
         args = ["git", "--git-dir=" + gitdir, "fast-import", "--quiet", "--export-marks=" + path]
 
         if os.path.exists(path):
             args.append("--import-marks=" + path)
 
         check_call(args)
+
+        refs_after = self.get_refs(gitdir)
+
+        changed = {}
+
+        for name, value in refs_after.iteritems():
+            if refs_before.get(name) == value:
+                continue
+
+            changed[name] = value
+
+        return changed
diff --git a/git_remote_helpers/util.py b/git_remote_helpers/util.py
index 8b9f302..6a9ec5c 100644
--- a/git_remote_helpers/util.py
+++ b/git_remote_helpers/util.py
@@ -160,6 +160,40 @@ def check_call(*popenargs, **kwargs):
     return 0
 
 
+# from python2.7:subprocess.py
+def check_output(*popenargs, **kwargs):
+    r"""Run command with arguments and return its output as a byte string.
+
+    If the exit code was non-zero it raises a CalledProcessError.  The
+    CalledProcessError object will have the return code in the returncode
+    attribute and output in the output attribute.
+
+    The arguments are the same as for the Popen constructor.  Example:
+
+    >>> check_output(["ls", "-l", "/dev/null"])
+    'crw-rw-rw- 1 root root 1, 3 Oct 18  2007 /dev/null\n'
+
+    The stdout argument is not allowed as it is used internally.
+    To capture standard error in the result, use stderr=STDOUT.
+
+    >>> check_output(["/bin/sh", "-c",
+    ...               "ls -l non_existent_file ; exit 0"],
+    ...              stderr=STDOUT)
+    'ls: non_existent_file: No such file or directory\n'
+    """
+    if 'stdout' in kwargs:
+        raise ValueError('stdout argument not allowed, it will be overridden.')
+    process = subprocess.Popen(stdout=subprocess.PIPE, *popenargs, **kwargs)
+    output, unused_err = process.communicate()
+    retcode = process.poll()
+    if retcode:
+        cmd = kwargs.get("args")
+        if cmd is None:
+            cmd = popenargs[0]
+        raise subprocess.CalledProcessError(retcode, cmd)
+    return output
+
+
 def file_reader_method (missing_ok = False):
     """Decorator for simplifying reading of files.
 
diff --git a/transport-helper.c b/transport-helper.c
index b0361c2..bb1b97f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -760,6 +760,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.5.1.292.g728120

Re: [PATCH v2 16/20] transport-helper: update ref status after push with export

[Jonathan Nieder]

Sverre Rabbelier wrote:

>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
>
>   This time a proper implementation of this patch.

Nice. :)

I assume this still does not return "error <dst> <reason>" responses,
instead disconnecting the helper on error, which is fine but probably
worth mentioning.

> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -148,11 +148,15 @@ def do_export(repo, args):
>      sys.stdout.flush()
>  
>      update_local_repo(repo)
> -    repo.importer.do_import(repo.gitdir)
> +    changed = repo.importer.do_import(repo.gitdir)
>  
>      if not repo.local:
>          repo.non_local.push(repo.gitdir)
>  
> +    for ref in changed:
> +        print "ok %s" % ref
> +    print
> +

The remote-helpers(1) manual says, concerning "push":

	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.

Ideally we would want "export" to also print all refs intended for
export, even if they did not actually change, but there's no obvious
way to get that information.  In the "not repo.local" case, seeing
which refs changed in .git/info/fast-import/<repo>.git like you do
here seems like a reasonable enough heuristic.

What happens if no ref changed (the "already up to date" case)?

Re: [PATCH v2 16/20] transport-helper: update ref status after push with export

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> +        changed = {}
> +
> +        for name, value in refs_after.iteritems():
> +            if refs_before.get(name) == value:
> +                continue
> +
> +            changed[name] = value

This assumes that nobody will ever _remove_ refs. I think it is a sensible
assumption but somebody might want to give a power to the fast-import stream
to say "I do not want that ref anymore".

I dunno; it is not a big deal to me either way.  Just something to think
about.

Re: [PATCH v2 16/20] transport-helper: update ref status after push with export

[Sverre Rabbelier]

Heya,

On Tue, Jun 21, 2011 at 22:05, Junio C Hamano <gitster@pobox.com> wrote:
> This assumes that nobody will ever _remove_ refs. I think it is a sensible
> assumption but somebody might want to give a power to the fast-import stream
> to say "I do not want that ref anymore".
>
> I dunno; it is not a big deal to me either way.  Just something to think
> about.

I'll have a look at addressing your and Jonathan's comments (the case
of 'already up to date' and 'refs removed').

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 17/20] 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'.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

 git-remote-testgit.py     |   14 +++++++++++---
 t/t5800-remote-helpers.sh |    2 +-
 transport-helper.c        |    7 ++++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 854e27e..4939853 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -115,14 +115,22 @@ 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, 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 bb1b97f..c089928 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.5.1.292.g728120

Re: [PATCH v2 17/20] transport-helper: change import semantics

[Jonathan Nieder]

Sverre Rabbelier wrote:

> 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'.

I think that for learnability, it would be better to use

	import <ref>
	import <ref>
	...
	[blank line]

since that is consistent with "fetch" and "push".  I'll try mocking up
a patch for that tonight.

Re: [PATCH v2 17/20] transport-helper: change import semantics

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 01:38, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I think that for learnability, it would be better to use
>
>        import <ref>
>        import <ref>
>        ...
>        [blank line]
>
> since that is consistent with "fetch" and "push".  I'll try mocking up
> a patch for that tonight.

That would mean the only change compared to the current behavior is
the additional blank line, yes?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 17/20] transport-helper: change import semantics

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Mon, Jun 20, 2011 at 01:38, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> I think that for learnability, it would be better to use
>>
>>        import <ref>
>>        import <ref>
>>        ...
>>        [blank line]
>>
>> since that is consistent with "fetch" and "push".  I'll try mocking up
>> a patch for that tonight.
>
> That would mean the only change compared to the current behavior is
> the additional blank line, yes?

It would mean no change compared to the current behavior. :)
disconnect_helper() writes the blank line.

That's why no patch came; sorry, I should have mentioned so.

Re: [PATCH v2 17/20] transport-helper: change import semantics

[Sverre Rabbelier]

Heya,

On Mon, Jul 4, 2011 at 23:58, Jonathan Nieder <jrnieder@gmail.com> wrote:
> It would mean no change compared to the current behavior. :)
> disconnect_helper() writes the blank line.
>
> That's why no patch came; sorry, I should have mentioned so.

With the only difference that currently that \n signifies the end of
the entire stream, whereas now it will mean the end of the imports?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 17/20] transport-helper: change import semantics

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Mon, Jul 4, 2011 at 23:58, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> It would mean no change compared to the current behavior. :)
[...]
> With the only difference that currently that \n signifies the end of
> the entire stream, whereas now it will mean the end of the imports?

Exactly.  The git-remote-testgit part of the patch would be pretty
similar to what you have and could even go earlier in the series if
wanted (since it does not require transport-helper changes), while the
transport-helper part should be unnecessary as long as patch 15/20
"use the new done feature where possible" does not remove the \n in
its output (as discussed in its subthread).

Meanwhile I should probably implement a "capability done" as Dmitry
hinted, to make blank lines no longer act as a terminator as a way of
helping people debugging by hand that hit <ENTER> too many times by
mistake.

[PATCH v2 18/20] transport-helper: export is no longer always the last command

[Sverre Rabbelier]

The trailing \n in the protocol signals the helper that the
connection is about to close, allowing it to do whatever cleanup
neccesary.

Previously, the connection would already be closed by the
time the trailing \n was to be written. Now that the remote-helper
protocol uses the new done command in its fast-import streams, this
is no longer the case and we can safely write the trailing \n.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Updated commit message.

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

diff --git a/transport-helper.c b/transport-helper.c
index c089928..f78b670 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -762,7 +762,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.5.1.292.g728120

[PATCH v2 19/20] 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.

Most likely 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.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Updated commit message.

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

diff --git a/transport-helper.c b/transport-helper.c
index f78b670..ddf0ffa 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.5.1.292.g728120

[PATCH v2 20/20] 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.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged.

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

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 4939853..30f775e 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -71,8 +71,26 @@ 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")
+    if not repo.gitdir:
+        die("Expected gitdir, got something else")
+
     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 +160,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)
     changed = repo.importer.do_import(repo.gitdir)
 
diff --git a/transport-helper.c b/transport-helper.c
index ddf0ffa..12f08a1 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;
@@ -710,7 +721,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 = STRING_LIST_INIT_NODUP;
 	struct strbuf buf = STRBUF_INIT;
 
@@ -718,26 +728,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) {
@@ -758,8 +748,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.5.1.292.g728120

Re: [PATCH v2 20/20] transport-helper: implement marks location as capability

[Jonathan Nieder]

Sverre Rabbelier wrote:

> [Subject: transport-helper: implement marks location as capability]
>
> 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.

Probably I'm not thinking straight, but it's not obvious how the subject
connects to the content.  I suppose the pieces I'm missing are that:

 - this patch introduces new undocumented "import-marks" and
   "export-marks" capabilities that take a pathname as parameter;

 - for remote-testgit, that pathname is based on $GIT_DIR, so to
   retrieve that value we flush stdout and read the gitdir immediately
   rather than hoping the gitdir arrives on stdin in time for it to
   be used later.

But that still leaves a couple of other questions unanswered:

Why do we use a "gitdir" capability for this at all, instead of
exporting the GIT_DIR environment variable as Tomas has suggested?
(Not about this patch, but a separate patch explaining that in the
documentation would be nice.)

How does this interact with fast-import's --relative-marks feature
(if at all)?

[...]
> +++ b/transport-helper.c
[...]
> @@ -718,26 +728,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);

Hm, it seems there is a silent (but good) change in the behavior of
the "export" command, too.

Except where noted already, the patches look good, and the series
seems to make the remote helper infrastructure a little saner.
Thanks.

Re: [PATCH v2 20/20] transport-helper: implement marks location as capability

[Sverre Rabbelier]

Heya,

On Mon, Jun 20, 2011 at 03:29, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Why do we use a "gitdir" capability for this at all, instead of
> exporting the GIT_DIR environment variable as Tomas has suggested?
> (Not about this patch, but a separate patch explaining that in the
> documentation would be nice.)

Mainly because I didn't take the time to implement that, will do.

> How does this interact with fast-import's --relative-marks feature
> (if at all)?

They're a supplement to it, the exporter uses all three features (see
git_remote_helpers/git/exporter.py).

-- 
Cheers,

Sverre Rabbelier