[PATCH v3 00/23] remote-helper improvements

[PATCH v3 00/23] remote-helper improvements

[Sverre Rabbelier]

Incorperated feedback from Junio, Johannes and Peff. I also included
Dmitry's patch that adds GITDIR, which I modified to just remove the
support for a gitdir command.

Dmitry Ivankov (1):
  remote-helpers: export GIT_DIR variable to helpers

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
  git-remote-testgit: 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 (16):
  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: Use capname for refspec capability too
  transport-helper: implement marks location as capability
  transport-helper: die early on encountering deleted refs
  t5800: document inability to push new branch with old content
  t5800: point out that deleting branches does not work

 Documentation/git-fast-export.txt    |    4 +
 Documentation/git-fast-import.txt    |   25 ++++
 Documentation/git-remote-helpers.txt |    3 +
 builtin/fast-export.c                |    9 ++
 fast-import.c                        |    8 +
 git-remote-testgit.py                |   62 +++++----
 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           |   81 +++++++++++
 remote-curl.c                        |   10 ++-
 t/t5800-remote-helpers.sh            |  112 +++++++++++++---
 t/t9300-fast-import.sh               |   42 ++++++
 transport-helper.c                   |  251 ++++++++++++++++++----------------
 15 files changed, 494 insertions(+), 187 deletions(-)

-- 
1.7.5.1.292.g728120

[PATCH v3 01/23] transport-helper: fix minor leak in push_refs_with_export

[Sverre Rabbelier]

From: 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 v3 02/23] 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 v3 03/23] 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>
---

  Unchanged

 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 v3 04/23] 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>
---

  Unchanged

 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

[PATCH v3 05/23] git-remote-testgit: 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>
---

  Unchanged

 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 v3 06/23] 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 v3 07/23] 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.

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

  Unchanged

 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

Re: [PATCH v3 07/23] git_remote_helpers: push all refs during a non-local export

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> 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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
>
>   Unchanged

There used to be "This does not deal with forced (not-fast-forward) pushes."
at the end of the message, no?

Re: [PATCH v3 07/23] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Mon, Jul 18, 2011 at 01:36, Junio C Hamano <gitster@pobox.com> wrote:
> There used to be "This does not deal with forced (not-fast-forward) pushes."
> at the end of the message, no?

Yes it did, not sure what happened to that. FIxed.

-- 
Cheers,

Sverre Rabbelier

[PATCH v3 08/23] remote-helpers: export GIT_DIR variable to helpers

[Sverre Rabbelier]

From: Dmitry Ivankov <divanorama@gmail.com>

The gitdir capability is recognized by git and can be used to tell
the helper where the .git directory is. But it is not mentioned in
the documentation and considered worse than if gitdir was passed
via GIT_DIR environment variable.

Remove support for the gitdir capability and export GIT_DIR instead.
Teach testgit to use env instead of the now-removed gitdir command.

[sr: fixed up documentation]

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

  New in this series.

 Documentation/git-remote-helpers.txt |    3 +++
 git-remote-testgit.py                |   14 +-------------
 transport-helper.c                   |   15 ++++++++++-----
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 58f6ad4..18b8341 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -47,6 +47,9 @@ arguments. The first argument specifies a remote repository as in git;
 it is either the name of a configured remote or a URL. The second
 argument specifies a URL; it is usually of the form
 '<transport>://<address>', but any arbitrary string is possible.
+The 'GIT_DIR' environment variable is set up for the remote helper
+and can be used to determine where to store additional data or from
+which directory to invoke auxiliary git commands.
 
 When git encounters a URL of the form '<transport>://<address>', where
 '<transport>' is a protocol that it cannot handle natively, it
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index e4a99a3..b0c1e9b 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -35,7 +35,7 @@ def get_repo(alias, url):
     prefix = 'refs/testgit/%s/' % alias
     debug("prefix: '%s'", prefix)
 
-    repo.gitdir = ""
+    repo.gitdir = os.environ["GIT_DIR"]
     repo.alias = alias
     repo.prefix = prefix
 
@@ -70,7 +70,6 @@ def do_capabilities(repo, args):
 
     print "import"
     print "export"
-    print "gitdir"
     print "refspec refs/heads/*:%s*" % repo.prefix
 
     print # end capabilities
@@ -150,22 +149,11 @@ def do_export(repo, args):
     repo.non_local.push(repo.gitdir)
 
 
-def do_gitdir(repo, args):
-    """Stores the location of the gitdir.
-    """
-
-    if not args:
-        die("gitdir needs an argument")
-
-    repo.gitdir = ' '.join(args)
-
-
 COMMANDS = {
     'capabilities': do_capabilities,
     'list': do_list,
     'import': do_import,
     'export': do_export,
-    'gitdir': do_gitdir,
 }
 
 
diff --git a/transport-helper.c b/transport-helper.c
index 34d18aa..6cccb20 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -105,6 +105,12 @@ static struct child_process *get_helper(struct transport *transport)
 	int refspec_alloc = 0;
 	int duped;
 	int code;
+	char git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];
+	const char *helper_env[] = {
+		git_dir_buf,
+		NULL
+	};
+
 
 	if (data->helper)
 		return data->helper;
@@ -120,6 +126,10 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->argv[2] = remove_ext_force(transport->url);
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
+
+	snprintf(git_dir_buf, sizeof(git_dir_buf), "%s=%s", GIT_DIR_ENVIRONMENT, get_git_dir());
+	helper->env = helper_env;
+
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)
 		die("Unable to find remote helper for '%s'", data->name);
@@ -174,11 +184,6 @@ static struct child_process *get_helper(struct transport *transport)
 			refspecs[refspec_nr++] = strdup(buf.buf + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
-		} else if (!strcmp(buf.buf, "gitdir")) {
-			struct strbuf gitdir = STRBUF_INIT;
-			strbuf_addf(&gitdir, "gitdir %s\n", get_git_dir());
-			sendline(data, &gitdir);
-			strbuf_release(&gitdir);
 		} else if (mandatory) {
 			die("Unknown mandatory capability %s. This remote "
 			    "helper probably needs newer version of Git.\n",
-- 
1.7.5.1.292.g728120

[PATCH v3 09/23] 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.

While at it also make sure that the stream doesn't end unexpectedly.

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

  Added the 'check fo unexpected EOF' chunk.

 remote-curl.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index b5be25c..30554f4 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -857,7 +857,14 @@ int main(int argc, const char **argv)
 	http_init(remote);
 
 	do {
-		if (strbuf_getline(&buf, stdin, '\n') == EOF)
+		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;
+		}
+		if (buf.len == 0)
 			break;
 		if (!prefixcmp(buf.buf, "fetch ")) {
 			if (nongit)
@@ -897,6 +904,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

[PATCH v3 10/23] 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 b0c1e9b..cdbc494 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -146,7 +146,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)
 
 
 COMMANDS = {
-- 
1.7.5.1.292.g728120

[PATCH v3 11/23] 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>
---

  Included the definition of CalledProcessError if subprocess does
  not already provide it to make sure everything still works on
  python 2.4.

 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          |   47 +++++++++++++++++++++++++++++++++++
 5 files changed, 62 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..1652c65 100644
--- a/git_remote_helpers/util.py
+++ b/git_remote_helpers/util.py
@@ -11,6 +11,21 @@ import sys
 import os
 import subprocess
 
+try:
+    from subprocess import CalledProcessError
+except ImportError:
+    # from python2.7:subprocess.py
+    # Exception classes used by this module.
+    class CalledProcessError(Exception):
+        """This exception is raised when a process run by check_call() returns
+        a non-zero exit status.  The exit status will be stored in the
+        returncode attribute."""
+        def __init__(self, returncode, cmd):
+            self.returncode = returncode
+            self.cmd = cmd
+        def __str__(self):
+            return "Command '%s' returned non-zero exit status %d" % (self.cmd, self.returncode)
+
 
 # Whether or not to show debug messages
 DEBUG = False
@@ -128,6 +143,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 CalledProcessError(retcode, cmd)
+    return 0
+
+
 def file_reader_method (missing_ok = False):
     """Decorator for simplifying reading of files.
 
-- 
1.7.5.1.292.g728120

[PATCH v3 12/23] 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 2a53640..f256475 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 v3 13/23] 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 v3 14/23] 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 6cccb20..dd8dd2c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -559,6 +559,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)
 {
@@ -613,76 +695,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 v3 15/23] 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 dd8dd2c..e02f4a3 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -209,6 +209,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)
@@ -220,13 +221,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[] = {
@@ -304,12 +305,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,
@@ -415,8 +417,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;
 
@@ -760,8 +765,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 v3 16/23] 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.

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

  This is a squash of "18/20 transport-helper: export is no longer
  always the last command" which added the no_disconnect_req.

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

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index cdbc494..af4d040 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -123,6 +123,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 e02f4a3..4c0d861 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -380,8 +380,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)
@@ -417,11 +418,8 @@ static int fetch_with_import(struct transport *transport,
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
-	if (disconnect_helper(transport))
-		die("Error while disconnecting helper");
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
-
 	free(fastimport.argv);
 	fastimport.argv = NULL;
 
@@ -764,11 +762,8 @@ 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");
-	if (disconnect_helper(transport))
-		die("Error while disconnecting helper");
 	return 0;
 }
 
-- 
1.7.5.1.292.g728120

[PATCH v3 17/23] transport-helper: update ref status after push with export

[Sverre Rabbelier]

Also add check_output from python 2.7.

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

  Unchanged, although see later patches that demonstrate that
  deleted refs don't work.

 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 af4d040..0b5928d 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -147,11 +147,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
+
 
 COMMANDS = {
     'capabilities': do_capabilities,
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 1652c65..fbbb01b 100644
--- a/git_remote_helpers/util.py
+++ b/git_remote_helpers/util.py
@@ -175,6 +175,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 4c0d861..a8f69b0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -764,6 +764,7 @@ static int push_refs_with_export(struct transport *transport,
 
 	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

[PATCH v3 18/23] 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 push statement. That is, the import statement is followed
by a series of import statements that are terminated by a '\n'.

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

  As Jonathan suggested we now follow push' example, rather than
  'list'. It makes the remote-testgit code a bit longer, but it means
  less changes to remote-helper.c.

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

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 0b5928d..1ed7a56 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -120,8 +120,22 @@ def do_import(repo, args):
     if not repo.gitdir:
         die("Need gitdir to import")
 
+    ref = args[0]
+    refs = [ref]
+
+    while True:
+        line = sys.stdin.readline()
+        if line == '\n':
+            break
+        if not line.startswith('import '):
+            die("Expected import line.")
+
+        # strip of leading 'import '
+        ref = line[7:].strip()
+        refs.append(ref)
+
     repo = update_local_repo(repo)
-    repo.exporter.export_repo(repo.gitdir, args)
+    repo.exporter.export_repo(repo.gitdir, refs)
 
     print "done"
 
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 12f471c..1c62001 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -98,7 +98,7 @@ test_expect_success 'fetch new branch' '
 	compare_refs public HEAD localclone FETCH_HEAD
 '
 
-test_expect_failure 'fetch multiple branches' '
+test_expect_success 'fetch multiple branches' '
 	(cd localclone &&
 	 git fetch
 	) &&
diff --git a/transport-helper.c b/transport-helper.c
index a8f69b0..0c00be9 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -418,6 +418,9 @@ static int fetch_with_import(struct transport *transport,
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
+
+	write_constant(data->helper->in, "\n");
+
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
 	free(fastimport.argv);
-- 
1.7.5.1.292.g728120

Re: [PATCH v3 18/23] transport-helper: change import semantics

[Dmitry Ivankov]

Hi,

I'll comment on "[PATCH v3 16/23] transport-helper: use the new done
feature where possible" (
http://thread.gmane.org/gmane.comp.version-control.git/177255/focus=177274)
first. There is a hunk for transport-helper.c
@@ -417,11 +418,8 @@ static int fetch_with_import(struct transport *transport,
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
-	if (disconnect_helper(transport))
-		die("Error while disconnecting helper");
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
-
 	free(fastimport.argv);
 	fastimport.argv = NULL;

This is related to the done feature but a helper can use the done
feature without this hunk.
And it does change the import semantics, so I'd suggest moving the
hunk here, to
"[PATCH v3 18/23] transport-helper: change import semantics".

On Sat, Jul 16, 2011 at 7:03 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Currently the helper must somehow guess how many import statements to
> read before it starts outputting its fast-export stream. This is
> because the remote helper infrastructure runs fast-import only once,
> so the helper is forced to output one stream for all import commands
> it will receive. The only reason this worked in the past was because
> only one ref was imported at a time.
>
> Change the semantics of the import statement such that it matches
> that of the push statement. That is, the import statement is followed
> by a series of import statements that are terminated by a '\n'.

I'll add more comments to the "import" job termination.

Before these two patches:
The helper input was closed which caused it to exit just after writing
the import stream.
(first, helper can terminate on eof; second it can terminate on '\n'
just before eof; third, it may know that the import always was the
last command and terminate before reading '\n').
The transport-helper.o didn't held a copy of helper's stdout, so once
the helper exits this end of import-stream pipe becomes closed.
And then the importer saw EOF on stdin and could terminate normally.

After these patches:
transport-helper waits for importer to terminate, so the helper must either
a) crash or exit during or just after the import and reading no more
than '\n' (blocking read beyond this character may hang us up)
or
b) use the 'done' feature to make importer terminate normally

So the old helper won't hang us up if it terminates (or at least
closes stdout) on just after the import or if it can read '\n' (but
not blocking read further) and terminate.
But if it just ignores empty lines and waits for new commands or eof
as an exit indicator, it all hangs.

Sadly, old documentation didn't mention many of these quirks, but on
the other hand the old semantic is a bit broken and not documented so
it's all ok.

We should definitely add a new description for import command to
Documentation/git-remote-helpers.txt
Something roughly like this addition:

A batch sequence of one or more import commands is terminated
with a blank line. Single fast-import stream should be produced for
the whole batch.
If the helper is able to proceed with more commands after the import
(in earlier versions import used to be the last command for a helper)
it must use the "done" feature to indicate the end of this batch's
import stream for the importer, otherwise the importer will wait forever
and the caller will wait for the importer to finish.

Aside from documenting and squashing I think this patch is the best
way to proceed with improving the import command.

>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
>
>  As Jonathan suggested we now follow push' example, rather than
>  'list'. It makes the remote-testgit code a bit longer, but it means
>  less changes to remote-helper.c.
>
>  git-remote-testgit.py     |   16 +++++++++++++++-
>  t/t5800-remote-helpers.sh |    2 +-
>  transport-helper.c        |    3 +++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 0b5928d..1ed7a56 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -120,8 +120,22 @@ def do_import(repo, args):
>     if not repo.gitdir:
>         die("Need gitdir to import")
>
> +    ref = args[0]
> +    refs = [ref]
> +
> +    while True:
> +        line = sys.stdin.readline()
> +        if line == '\n':
> +            break
> +        if not line.startswith('import '):
> +            die("Expected import line.")
> +
> +        # strip of leading 'import '
> +        ref = line[7:].strip()
> +        refs.append(ref)
> +
>     repo = update_local_repo(repo)
> -    repo.exporter.export_repo(repo.gitdir, args)
> +    repo.exporter.export_repo(repo.gitdir, refs)
>
>     print "done"
>
> diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
> index 12f471c..1c62001 100755
> --- a/t/t5800-remote-helpers.sh
> +++ b/t/t5800-remote-helpers.sh
> @@ -98,7 +98,7 @@ test_expect_success 'fetch new branch' '
>        compare_refs public HEAD localclone FETCH_HEAD
>  '
>
> -test_expect_failure 'fetch multiple branches' '
> +test_expect_success 'fetch multiple branches' '
>        (cd localclone &&
>         git fetch
>        ) &&
> diff --git a/transport-helper.c b/transport-helper.c
> index a8f69b0..0c00be9 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -418,6 +418,9 @@ static int fetch_with_import(struct transport *transport,
>                sendline(data, &buf);
>                strbuf_reset(&buf);
>        }
> +
> +       write_constant(data->helper->in, "\n");
> +
>        if (finish_command(&fastimport))
>                die("Error while running fast-import");
>        free(fastimport.argv);
> --
> 1.7.5.1.292.g728120
>
>

[PATCH v3 19/23] transport-helper: Use capname for refspec capability too

[Sverre Rabbelier]

Previously the refspec capability 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 'refspec' as capability, and the one that added
required capabilities were done in parallel.

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

  Unchanged.

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

diff --git a/transport-helper.c b/transport-helper.c
index 0c00be9..0cfc9ae 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -181,7 +181,7 @@ 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 (mandatory) {
-- 
1.7.5.1.292.g728120

[PATCH v3 20/23] transport-helper: implement marks location as capability

[Sverre Rabbelier]

Now that the gitdir location is exported as an environment variable
this can be implemented elegantly without requiring any explicit
flushes nor an ad-hoc exchange of values.

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

  Unchanged.

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

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 1ed7a56..e9c832b 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -72,6 +72,17 @@ def do_capabilities(repo, args):
     print "export"
     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
 
 
@@ -147,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)
     changed = repo.importer.do_import(repo.gitdir)
 
diff --git a/transport-helper.c b/transport-helper.c
index 0cfc9ae..74c3122 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;
@@ -184,6 +186,16 @@ static struct child_process *get_helper(struct transport *transport)
 			refspecs[refspec_nr++] = strdup(capname + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
+		} 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",
@@ -369,10 +381,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));
@@ -383,10 +394,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;
@@ -713,7 +724,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;
 
@@ -721,26 +731,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) {
@@ -761,8 +751,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

[PATCH v3 21/23] transport-helper: die early on encountering deleted refs

[Sverre Rabbelier]

Remote helpers do not support deleting refs by means of the 'export'
command sincethe fast-import protocol does not support it.

Check explicitly for deleted refs and die early.

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

  New in this series.

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

diff --git a/transport-helper.c b/transport-helper.c
index 74c3122..4eab844 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -746,6 +746,10 @@ static int push_refs_with_export(struct transport *transport,
 		}
 		free(private);
 
+		if (ref->deletion) {
+			die("remote-helpers do not support ref deletion");
+		}
+
 		if (ref->peer_ref)
 			string_list_append(&revlist_args, ref->peer_ref->name);
 
-- 
1.7.5.1.292.g728120

[RFD/PATCH v3 22/23] t5800: document inability to push new branch with old content

[Sverre Rabbelier]

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

  So, I'm not sure what's going on here. It just says:
  "Everything up to date" when you try to do a push like this.

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

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 1c62001..68f8418 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -125,6 +125,14 @@ test_expect_success 'push new branch by name' '
 	compare_refs clone HEAD server refs/heads/new-name
 '
 
+test_expect_failure 'push new branch with old content' '
+	(cd clone &&
+	 git checkout -b existing &&
+	 git push origin existing
+	) &&
+	compare_refs clone refs/heads/existing server refs/heads/existing
+'
+
 test_expect_failure 'push new branch with old:new refspec' '
 	(cd clone &&
 	 git push origin new-name:new-refspec
-- 
1.7.5.1.292.g728120

[RDD/PATCH v3 23/23] t5800: point out that deleting branches does not work

[Sverre Rabbelier]

This test actually breaks the repositories involved somehow, so it is
not enabled by default.
---

  Not meant for inclusion.

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

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 68f8418..ad85888 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -133,6 +133,17 @@ test_expect_failure 'push new branch with old content' '
 	compare_refs clone refs/heads/existing server refs/heads/existing
 '
 
+test_expect_failure BROKEN 'delete branch' '
+	(cd clone &&
+	 git checkout -b delete-me &&
+	 echo content >>file &&
+	 git commit -a -m eight &&
+	 git push origin delete-me
+	 git push origin :delete-me) &&
+	test_must_fail git --git-dir="server/.git" rev-parse --verify delete-me
+'
+
+
 test_expect_failure 'push new branch with old:new refspec' '
 	(cd clone &&
 	 git push origin new-name:new-refspec
-- 
1.7.5.1.292.g728120

Re: [PATCH v3 00/23] remote-helper improvements

[Jeff King]

On Sat, Jul 16, 2011 at 03:03:20PM +0200, Sverre Rabbelier wrote:

> Incorperated feedback from Junio, Johannes and Peff. I also included
> Dmitry's patch that adds GITDIR, which I modified to just remove the
> support for a gitdir command.

I read through these, and with the giant disclaimer that:

  1. The entirety of my remote helper knowledge is from working on the
     patches in this series that are mine.

  2. I don't really know anything about writing good-looking python
     code.

The patches up to 21 (i.e., not the RFD ones) all made sense to me. At
least, the goals from the commit messages looked sane, and the patches
seemed to implement the goals reasonably.

-Peff

Re: [PATCH v3 00/23] remote-helper improvements

[Sverre Rabbelier]

Heya,

On Mon, Jul 18, 2011 at 05:28, Jeff King <peff@peff.net> wrote:
> The patches up to 21 (i.e., not the RFD ones) all made sense to me. At
> least, the goals from the commit messages looked sane, and the patches
> seemed to implement the goals reasonably.

Thanks for reviewing :).

-- 
Cheers,

Sverre Rabbelier