[PATCH 00/19] remote-helper improvements

[PATCH 00/19] remote-helper improvements

[Sverre Rabbelier]

This is a re-roll of Peff's recent series [0] and the series [1] I sent out
well over half a year ago. My series replaces two from Peff's.

[0] http://thread.gmane.org/gmane.comp.version-control.git/174904
[1] http://thread.gmane.org/gmane.comp.version-control.git/154669

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 (13):
      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               |   54 ++++++---
 git_remote_helpers/git/exporter.py  |    8 +-
 git_remote_helpers/git/importer.py  |    5 +-
 git_remote_helpers/git/non_local.py |    2 +-
 remote-curl.c                       |    3 +
 t/t5800-remote-helpers.sh           |   59 ++++++++-
 t/t9300-fast-import.sh              |   42 ++++++
 transport-helper.c                  |  238 ++++++++++++++++++----------------
 12 files changed, 320 insertions(+), 137 deletions(-)

[PATCH 01/19] 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 from Peff's series.

 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

Re: [PATCH 01/19] transport-helper: fix minor leak in push_refs_with_export

[Jeff King]

On Wed, Jun 08, 2011 at 08:48:32PM +0200, Sverre Rabbelier wrote:

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

Hmm. This and the other ones from me lack my S-o-b. I'm pretty sure my
originals included it, so I'm not sure what stripped it out.

-Peff

Re: [PATCH 01/19] transport-helper: fix minor leak in push_refs_with_export

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 23:57, Jeff King <peff@peff.net> wrote:
> Hmm. This and the other ones from me lack my S-o-b. I'm pretty sure my
> originals included it, so I'm not sure what stripped it out.

I'm using gmail, so since I can't really apply from the list I pulled
from your github, where you hadn't added the s-o-b it seems :).

-- 
Cheers,

Sverre Rabbelier

[PATCH 02/19] 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: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged from Peff's series.

 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 03/19] 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 new refs without us.
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".

There are two possible solutions:

  1. Indicate the mapping to fast-export, so that it can
     generate the "mapped" names. Unfortunately, this is
     somewhat difficult due to the way fast-import is
     implemented. It feeds its revision parameters to the
     regular rev-walking machinery, asking the revision code
     to mark the "source" of each commit. So if fast-export
     sees that we want commits from "refs/heads/old", but
     also that we want to call commits from old as
     "refs/heads/new", it can't distinguish between the
     case of "I want _only_ new, using the commits of old"
     and "I want _both_ old and new, which happen to point
     to the same commits".

     And it's important to distinguish those cases, because
     in one, the remote will update master, and in the
     other, it will not.

     This isn't an insurmountable obstacle, but it's going
     to mean either fast-export has to take on more of the
     revision argument parsing and revwalking
     responsibility, or the revision walker's show_source
     code will have to learn about mapping names.

  2. Indicate the mapping to the remote helper, who can then
     apply the mapping to the results of the export. Right
     now the export remote helper command has no arguments;
     it simply says "hey, I'm going to dump a fast-export
     stream on you". And the remote-helper looks through the
     results of the stream to decide which refs are being
     pushed. This could be extended instead to something
     like:

       export refs/heads/foo
       export refs/heads/old:refs/heads/new
       <blank line>

     which would allow the remote helper to map names as
     appropriate. This a reasonably elegant solution; the
     downside is that it breaks compatibility with existing
     remote helpers.

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

  Unchanged from Peff's series.

 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 3a37ad0..a10a48d 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -81,4 +81,51 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
+test_expect_failure PYTHON_24 '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 PYTHON_24 '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 PYTHON_24 '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 PYTHON_24 '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 PYTHON_24 '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 03/19] t5800: document some non-functional parts of remote helpers

[Jonathan Nieder]

Sverre Rabbelier wrote:

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

Thanks.  Makes sense up to here (and the previous two patches are
obviously good, too).

> The problem is that we push all of the work of generating
> the export stream onto fast-export,

Here "we" means remote-testgit, not transport-helper, right?

> but we have no way of
> communicating to fast-export that this name mapping is
> happening.

It's not just name mapping, no?  E.g., I could try

	git push testgit::/path/to/repo $(git rev-parse HEAD):new

So I think the current implementation of "export" by testgit
is just wrong. ;-)

[...]
> There are two possible solutions:
>
>   1. Indicate the mapping to fast-export, so that it can
>      generate the "mapped" names. Unfortunately, this is
>      somewhat difficult due to the way fast-import is
>      implemented. It feeds its revision parameters to the
>      regular rev-walking machinery

This seems like a reasonable solution.  One way would be to teach
fast-export about refspecs; another way would be to set up refs in a
private namespace and then munge the stream that fast-export spits out
before passing it back to the transport-helper; another (more ugly and
hackish) way would be to set up a private repository with the real
repository set up as an alternate, put the appropriate refs there, and
point fast-export to that.

Anyway, I don't think this should be in the commit message for the new
test which doesn't even know about the remote helper protocol. :)

> --- a/t/t5800-remote-helpers.sh
> +++ b/t/t5800-remote-helpers.sh
> @@ -81,4 +81,51 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
>  	compare_refs clone HEAD server HEAD
>  '
>  
> +test_expect_failure PYTHON_24 'fetch new branch' '

Side note: this repeated PYTHON_24 implementation detail as a
prerequisite feels wrong.  Would it make sense to do something like

	if test_have_prereq PYTHON_24
	then
		test_set_prereq REMOTE_TESTGIT
	fi

at the start and use that?

[...]
> +test_expect_failure PYTHON_24 '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
> +'

It would also be interesting to test tag pushes and pushes by object
name.

	(cd clone &&
	 git tag -a -m 'example tag' example-tag &&
	 git push origin tag example-tag
	) &&
	compare_refs clone example-tag server refs/tags/example-tag

	(cd clone &&
	 file_blob=$(git rev-parse --verify HEAD:file) &&
	 git push origin "$file_blob":refs/blobs/file
	) &&
	compare_refs clone HEAD:file server refs/blobs/file

	(cd clone &&
	 echo more >>file &&
	 git add file &&
	 file_blob=$(git rev-parse --verify :file) &&
	 git push origin "$file_blob":refs/blobs/newfile
	) &&
	compare_refs clone :file server refs/blobs/newfile

	(cd clone &&
	 echo more >>file &&
	 git commit -a -m another &&
	 git push origin HEAD^0:master
	) &&
	compare_refs clone HEAD server HEAD

Re: [PATCH 03/19] t5800: document some non-functional parts of remote helpers

[Jonathan Nieder]

Jonathan Nieder wrote:
> Sverre Rabbelier wrote:

>> The problem is that we push all of the work of generating
>> the export stream onto fast-export,
>
> Here "we" means remote-testgit, not transport-helper, right?

Gah!  Clearly I am not thinking straight and it means the
transport-helper.

> It's not just name mapping, no?  E.g., I could try
>
> 	git push testgit::/path/to/repo $(git rev-parse HEAD):new
>
> So I think the current implementation of "export" by testgit
> is just wrong. ;-)

So this refers to transport-helper, too, and...

[...]
> This seems like a reasonable solution.  One way would be to teach
> fast-export about refspecs

... the level of permissible hackishness goes down :), making teaching
fast-export about refspecs the only obviously reasonable fix at first
glance.

[...]
> Anyway, I don't think this should be in the commit message for the new
> test which doesn't even know about the remote helper protocol. :)

Sorry for the nonsense.  At least this last part still applies.

Re: [PATCH 03/19] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 21:36, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Gah!  Clearly I am not thinking straight and it means the
> transport-helper.

Yup.

> ... the level of permissible hackishness goes down :), making teaching
> fast-export about refspecs the only obviously reasonable fix at first
> glance.

Or, as I mentioned in the nearby thread, by having
fast-import/fast-export not modify any refs at all, and do that
separately.

>> Anyway, I don't think this should be in the commit message for the new
>> test which doesn't even know about the remote helper protocol. :)
>
> Sorry for the nonsense.  At least this last part still applies.

Sure, this was just a resend of Peff's patch, it does need a new
commit message :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 03/19] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 21:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> +test_expect_failure PYTHON_24 'fetch new branch' '
>
> Side note: this repeated PYTHON_24 implementation detail as a
> prerequisite feels wrong.  Would it make sense to do something like
>
>        if test_have_prereq PYTHON_24
>        then
>                test_set_prereq REMOTE_TESTGIT
>        fi
>
> at the start and use that?

I don't know, it already says:

if test_have_prereq PYTHON && "$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

At the start. Are you suggesting to rename PYTHON_24 to REMOTE_TESTGIT?

> It would also be interesting to test tag pushes and pushes by object
> name.

I was going to test this, but it seems there's a problem with my
series. Peff, if you want to look into it, (if not I'll try to look at
it tomorrow) it's up at my github fork [0].

ok 12 - push when remote has extra refs

expecting success:
	(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

Switched to a new branch 'new-name'
[new-name f29f436] seven
 Author: A U Thor <author@example.com>
 1 files changed, 1 insertions(+), 0 deletions(-)
Unpacking objects: 100% (3/3), done.
To testgit::file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server
 * [new branch]      master
error: Trying to write ref refs/remotes/origin/master with nonexistant
object 0000000000000000000000000000000000000000
error: Cannot update the ref 'refs/remotes/origin/master'.

[0] https://github.com/SRabbelier/git/commits/remote-helpers

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 03/19] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 23:13, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> I was going to test this, but it seems there's a problem with my
> series. Peff, if you want to look into it, (if not I'll try to look at
> it tomorrow) it's up at my github fork [0].

Turns out this was caused by the sloppy implementation of [PATCH
15/19] transport-helper: update ref status after push with export.
After implementing it properly the breakage is fixed although I'm not
entirely sure the output is (always) correct.

Follows are the test results (ran with GIT_TRACE=1 GIT_DEBUG_TESTGIT=1):


expecting success:
       (cd clone &&
        git tag -a -m "example tag" example-tag &&
        git push origin tag example-tag
       ) &&
       compare_refs clone example-tag server refs/tags/example-tag

trace: built-in: git 'tag' '-a' '-m' 'example tag' 'example-tag'
trace: built-in: git 'push' 'origin' 'tag' 'example-tag'
trace: run_command: 'git-remote-testgit' 'origin'
'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server'
trace: built-in: git 'ls-remote' 'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'
trace: run_command: 'git-upload-pack '\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: exec: 'sh' '-c' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\''' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
prefix: 'refs/testgit/origin/'
Got arguments ['origin', 'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server']
Got command 'capabilities' with args ''
Got command 'gitdir' with args '.git'
Got command 'list' with args ''
? refs/heads/new
? refs/heads/master
? refs/heads/new-name
@refs/heads/master HEAD
Got command 'export' with args ''
trace: run_command: 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'refs/tags/example-tag'
trace: exec: 'git' 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'refs/tags/example-tag'
trace: built-in: git 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'refs/tags/example-tag'
trace: built-in: git 'fetch' '--quiet'
'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'
trace: run_command: 'git-upload-pack '\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: exec: 'sh' '-c' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\''' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: run_command: 'rev-list' '--quiet' '--objects' '--stdin' '--not' '--all'
trace: built-in: git 'update-ref' 'refs/heads/master' 'FETCH_HEAD'
trace: built-in: git 'for-each-ref' 'refs/heads'
trace: exec: 'git-fast-import' '--quiet'
'--export-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
'--import-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
trace: run_command: 'git-fast-import' '--quiet'
'--export-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
'--import-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
trace: built-in: git 'for-each-ref' 'refs/heads'
trace: built-in: git 'push' '--quiet'
'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git' '--all'
trace: run_command: 'git-receive-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: exec: 'sh' '-c' 'git-receive-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\''' 'git-receive-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: built-in: git 'receive-pack' '/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'
Everything up-to-date
trace: built-in: git 'rev-parse' '--verify' 'example-tag'
trace: built-in: git 'rev-parse' '--verify' 'refs/tags/example-tag'
fatal: Needed a single revision
not ok - 15 test pushing tags

Similar output for the following two:

<snip>
Everything up-to-date
trace: built-in: git 'rev-parse' '--verify' 'HEAD:file'
trace: built-in: git 'rev-parse' '--verify' 'refs/blobs/file'
fatal: Needed a single revision
not ok - 16 test pushing a blob

<snip>
Everything up-to-date
trace: built-in: git 'rev-parse' '--verify' ':file'
trace: built-in: git 'rev-parse' '--verify' 'refs/blobs/newfile'
fatal: Needed a single revision
not ok - 17 test pushing an updated blob

And then finally a rather different failure:

expecting success:
       (cd clone &&
        echo more >>file &&
        git commit -a -m another &&
        git push origin HEAD^0:master
       ) &&
       compare_refs clone HEAD server HEAD


trace: built-in: git 'commit' '-a' '-m' 'another'
[new-name a02c029] another
 Author: A U Thor <author@example.com>
 1 files changed, 1 insertions(+), 0 deletions(-)
trace: built-in: git 'push' 'origin' 'HEAD^0:master'
trace: run_command: 'git-remote-testgit' 'origin'
'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server'
trace: built-in: git 'ls-remote' 'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'
trace: run_command: 'git-upload-pack '\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: exec: 'sh' '-c' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\''' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
prefix: 'refs/testgit/origin/'
Got arguments ['origin', 'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server']
Got command 'capabilities' with args ''
Got command 'gitdir' with args '.git'
Got command 'list' with args ''
? refs/heads/new
? refs/heads/master
? refs/heads/new-name
@refs/heads/master HEAD
Got command 'export' with args ''
trace: run_command: 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'HEAD^0'
trace: exec: 'git' 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'HEAD^0'
trace: built-in: git 'fast-export' '--use-done-feature'
'--export-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'--import-marks=.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/testgit.marks'
'^refs/testgit/origin/master' 'HEAD^0'
trace: built-in: git 'fetch' '--quiet'
'file:///home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'
trace: run_command: 'git-upload-pack '\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: exec: 'sh' '-c' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\''' 'git-upload-pack
'\''/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/server/.git'\'''
trace: run_command: 'rev-list' '--quiet' '--objects' '--stdin' '--not' '--all'
trace: built-in: git 'update-ref' 'refs/heads/master' 'FETCH_HEAD'
trace: built-in: git 'for-each-ref' 'refs/heads'
trace: exec: 'git-fast-import' '--quiet'
'--export-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
'--import-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
trace: run_command: 'git-fast-import' '--quiet'
'--export-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
'--import-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks'
fatal: Branch name doesn't conform to GIT standards: HEAD^0
fast-import: dumping crash report to /home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/.git/fast_import_crash_2016
Traceback (most recent call last):
  File "/home/sverre/code/git/git-remote-testgit", line 265, in <module>
    sys.exit(main(sys.argv))
  File "/home/sverre/code/git/git-remote-testgit", line 262, in main
    more = read_one_line(repo)
  File "/home/sverre/code/git/git-remote-testgit", line 227, in read_one_line
    func(repo, cmdline)
  File "/home/sverre/code/git/git-remote-testgit", line 164, in do_export
    changed = repo.importer.do_import(repo.gitdir)
  File "/home/sverre/code/git/t/../git_remote_helpers/build/lib/git_remote_helpers/git/importer.py",
line 54, in do_import
    check_call(args)
  File "/home/sverre/code/git/t/../git_remote_helpers/build/lib/git_remote_helpers/util.py",
line 159, in check_call
    raise subprocess.CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git',
'--git-dir=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/.git',
'fast-import', '--quiet',
'--export-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks',
'--import-marks=/home/sverre/code/git/t/trash
directory.t5800-remote-helpers/clone/.git/info/fast-import/27c45ccde749c9cc424db9cf911f01688e75d056/git.marks']'
returned non-zero exit status 128
not ok - 18 test pushing HEAD^0

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 03/19] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Wed, Jun 08, 2011 at 02:28:51PM -0500, Jonathan Nieder wrote:

> > but we have no way of
> > communicating to fast-export that this name mapping is
> > happening.
> 
> It's not just name mapping, no?  E.g., I could try
> 
> 	git push testgit::/path/to/repo $(git rev-parse HEAD):new
> 
> So I think the current implementation of "export" by testgit
> is just wrong. ;-)

Yeah, it's the same issue. We have no way of communicating the name of
the right-hand ref at all to a helper, aside from asking fast-export to
munge the refname in its output stream. But we have no way of telling
fast-export that even though we asked for "foo", it needs to write the
result into "bar" (and potentially _also_ into "foo", if it's a ref
found on the RHS of a different refspec).

> This seems like a reasonable solution.  One way would be to teach
> fast-export about refspecs; another way would be to set up refs in a
> private namespace and then munge the stream that fast-export spits out
> before passing it back to the transport-helper; another (more ugly and
> hackish) way would be to set up a private repository with the real
> repository set up as an alternate, put the appropriate refs there, and
> point fast-export to that.

Munging the stream is a little tricky. We have to correctly parse the
stream and not just do something hack-ish like sed (which is what
git-remote-testgit does, but that's OK, because it's a hack-ish
test script).

But we also have to take care to handle duplicates. So exporting
"refs/heads/foo:refs/heads/bar" would mean that we rewrite all of
refs/heads/foo into refs/heads/bar on commit lines. But two refspecs
like "refs/heads/foo:refs/heads/bar refs/heads/foo:refs/heads/foo" mean
we must write all of the commit lines as "foo" and then do a "reset" of
bar to foo at the end (at least this is what fast-export already does).
But we also need to handle criss-crosses, like:

  refs/heads/foo:refs/heads/bar refs/heads/baz:refs/heads/foo

In that case we want change all commit lines for "foo" to "bar", but
also all for "baz" to "foo".

So implementation-wise, the simplest way is if you can give each tip a
name, and then at the end assign all of the tips to refnames (this is
more or less what regular push does internally, of course).

I'm not too familiar with the fast-import protocol or what other
systems' fast importers are capable of. Is it OK to just make a bunch of
commits with marks, and then at the end do our resets? That would make
the problem much simpler, I think.

> Anyway, I don't think this should be in the commit message for the new
> test which doesn't even know about the remote helper protocol. :)

Yeah, maybe not. I wanted to stick the discussion somewhere, and since I
never actually fixed this bug, there wasn't another relevant commit. :)

> > --- a/t/t5800-remote-helpers.sh
> > +++ b/t/t5800-remote-helpers.sh
> > @@ -81,4 +81,51 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
> >  	compare_refs clone HEAD server HEAD
> >  '
> >  
> > +test_expect_failure PYTHON_24 'fetch new branch' '
> 
> Side note: this repeated PYTHON_24 implementation detail as a
> prerequisite feels wrong.  Would it make sense to do something like
> 
> 	if test_have_prereq PYTHON_24
> 	then
> 		test_set_prereq REMOTE_TESTGIT
> 	fi
> 
> at the start and use that?

Really, I think you could just exit early if PYTHON_24 isn't set, as all
of the tests require it. If we end up with other non-python helper
tests, they will probably go in a separate script anyway.

> > +test_expect_failure PYTHON_24 '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
> > +'
> 
> It would also be interesting to test tag pushes and pushes by object
> name.
> [examples]

Agreed.

-Peff

[PATCH 04/19] 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: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged from Peff's series.

 git-remote-testgit.py              |    2 +-
 git_remote_helpers/git/exporter.py |    8 +++++++-
 t/t5800-remote-helpers.sh          |    2 +-
 3 files changed, 9 insertions(+), 3 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..8fd83c7 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'))
 
@@ -38,6 +43,7 @@ class GitExporter(object):
         sys.stdout.flush()
 
         args = ["git", "--git-dir=" + self.repo.gitpath, "fast-export", "--export-marks=" + path]
+        args.extend(refs)
 
         if os.path.exists(path):
             args.append("--import-marks=" + path)
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index a10a48d..562edf4 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -81,7 +81,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
-test_expect_failure PYTHON_24 'fetch new branch' '
+test_expect_success PYTHON_24 'fetch new branch' '
 	(cd public &&
 	 git checkout -b new &&
 	 echo content >>file &&
-- 
1.7.5.1.292.g728120

Re: [PATCH 04/19] teach remote-testgit to import non-HEAD refs

[Jonathan Nieder]

Sverre Rabbelier wrote:

>   Unchanged from Peff's series.

Looks good.  What happened to Peff's signoff?

Re: [PATCH 04/19] teach remote-testgit to import non-HEAD refs

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 21:30, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>   Unchanged from Peff's series.
>
> Looks good.  What happened to Peff's signoff?

I pulled the series from his github repo, which didn't have it.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 04/19] teach remote-testgit to import non-HEAD refs

[Jeff King]

On Wed, Jun 08, 2011 at 09:47:38PM +0200, Sverre Rabbelier wrote:

> On Wed, Jun 8, 2011 at 21:30, Jonathan Nieder <jrnieder@gmail.com> wrote:
> >>   Unchanged from Peff's series.
> >
> > Looks good.  What happened to Peff's signoff?
> 
> I pulled the series from his github repo, which didn't have it.

Ah, yeah, I stick it in when I send the email. Now that I am publishing
my topic branches, I should maybe do it at commit time.

-Peff

Re: [PATCH 04/19] teach remote-testgit to import non-HEAD refs

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> 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: Sverre Rabbelier <srabbelier@gmail.com>
> ---
>
>   Unchanged from Peff's series.

This seems to be slightly different from what was sent to the list,
though.  Using refs=None as default and assigning ["HEAD"] is more in line
with the standard Python practice, so the implementation is better.

But "do we still append HEAD after --import-marks?" still stands.

Re: [PATCH 04/19] teach remote-testgit to import non-HEAD refs

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 01:48, Junio C Hamano <gitster@pobox.com> wrote:
> This seems to be slightly different from what was sent to the list,
> though.  Using refs=None as default and assigning ["HEAD"] is more in line
> with the standard Python practice, so the implementation is better.

Ah, yes, I fixed the style nits suggested in my own review.

> But "do we still append HEAD after --import-marks?" still stands.

Fixed locally. Sharp eyes :)

-- 
Cheers,

Sverre Rabbelier

[PATCH 05/19] 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: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged from Peff's series.

 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 562edf4..1abfa5c 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -102,7 +102,7 @@ test_expect_failure PYTHON_24 'fetch multiple branches' '
 	compare_refs server new localclone refs/remotes/origin/new
 '
 
-test_expect_failure PYTHON_24 'push when remote has extra refs' '
+test_expect_success PYTHON_24 '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 06/19] 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: Sverre Rabbelier <srabbelier@gmail.com>
---

  Unchanged from Peff's series.

 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 1abfa5c..a4afe38 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -111,7 +111,7 @@ test_expect_success PYTHON_24 'push when remote has extra refs' '
 	compare_refs clone master server master
 '
 
-test_expect_failure PYTHON_24 'push new branch by name' '
+test_expect_success PYTHON_24 'push new branch by name' '
 	(cd clone &&
 	 git checkout -b new-name  &&
 	 echo content >>file &&
-- 
1.7.5.1.292.g728120

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Jonathan Nieder]

Sverre Rabbelier wrote:

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

Does this deal with forced (non-fast-forward) pushes?  (Not a
complaint, just curious.)

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Jeff King]

On Wed, Jun 08, 2011 at 02:42:05PM -0500, Jonathan Nieder wrote:

> Sverre Rabbelier wrote:
> 
> > --- 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)
> 
> Does this deal with forced (non-fast-forward) pushes?  (Not a
> complaint, just curious.)

No, nor can it. The problem is that any information about the ref
mapping or force-state is lost. All the helper sees is that stuff got
put into the staging repo, and it has to move it over.

If we do a fix that allows "refs/heads/foo:refs/heads/bar", then it
should also properly allow "+refs/heads/foo:refs/heads/bar". Which I
think means changing the single "export" command in the ref-helper to
something like:

  export refs/heads/foo
  export refs/heads/foo:refs/heads/bar
  export +refs/heads/force

-Peff

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 00:19, Jeff King <peff@peff.net> wrote:
> If we do a fix that allows "refs/heads/foo:refs/heads/bar", then it
> should also properly allow "+refs/heads/foo:refs/heads/bar". Which I
> think means changing the single "export" command in the ref-helper to
> something like:
>
>  export refs/heads/foo
>  export refs/heads/foo:refs/heads/bar
>  export +refs/heads/force

Or, since we changed import, to:

export
refs/heads/foo
refs/heads/foo:refs/heads/bar
+refs/heads/force
\n

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Jonathan Nieder]

Jeff King wrote:

> If we do a fix that allows "refs/heads/foo:refs/heads/bar", then it
> should also properly allow "+refs/heads/foo:refs/heads/bar".  Which I
> think means changing the single "export" command in the ref-helper to
> something like:
>
>   export refs/heads/foo
>   export refs/heads/foo:refs/heads/bar
>   export +refs/heads/force

Thanks for explaining.  The spirit of what you're saying seems right,
so I'd like to spell it out a little to make sure.

If we imagine that the remote helper author wants to write as little
code as possible (which seems reasonable), then probably their
"export" command will simply feed its input to a vcs-fast-import
program.  git fast-export doesn't know how to do ref mapping and
neither would vcs-fast-import.  So one is led to wonder which stage in
the pipeline can make the adaptations to make "git push hgrepo
HEAD:refs/heads/bar" work.

To be friendly to remote helper authors, it would be nice to take
care of the ref mapping somewhere on the transport-helper side, unless
fast-import learns a new mode that does not label its result with
refs.  In the latter case, the "export" command could look like[*]

	export :1 refs/heads/foo
	export :2 refs/heads/bar
	export :3 +refs/heads/force

with :1, :2, and :3 being marks in the fast-import stream.

In the former case, the transport-helper could arrange for a stream
that sets up refs/heads/foo, refs/heads/bar, and refs/heads/force
to be written to stdout, but there would need to be a way to mention
in the stream that the update to refs/heads/force is allowed to be
non fast-forward.  fast-import has a --force option for that, which
unfortunately can't be controlled ref by ref.

Which is all a long way to say, I think you're right and that
something like the syntax marked with [*] would be nice.

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 10:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
>        export :1 refs/heads/foo
>        export :2 refs/heads/bar
>        export :3 +refs/heads/force
>
> with :1, :2, and :3 being marks in the fast-import stream.

The only problem there is that we don't know the relevant marks beforehand.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Thu, Jun 9, 2011 at 10:09, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>        export :1 refs/heads/foo
>>        export :2 refs/heads/bar
>>        export :3 +refs/heads/force
>>
>> with :1, :2, and :3 being marks in the fast-import stream.
>
> The only problem there is that we don't know the relevant marks beforehand.

Since this requires changing "git fast-export" anyway, we could
arrange to know.

A related problem, though: it is not friendly to stomp on mark numbers
which an exporter might want to use for some other purpose.  So yeah,
the precise syntax above is not so great.

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 10:43, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Sverre Rabbelier wrote:
>> On Thu, Jun 9, 2011 at 10:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>>        export :1 refs/heads/foo
>>>        export :2 refs/heads/bar
>>>        export :3 +refs/heads/force
>>>
>>> with :1, :2, and :3 being marks in the fast-import stream.
>>
>> The only problem there is that we don't know the relevant marks beforehand.
>
> Since this requires changing "git fast-export" anyway, we could
> arrange to know.

How? Or do you mean like, 'reserve' a certain mark?

> A related problem, though: it is not friendly to stomp on mark numbers
> which an exporter might want to use for some other purpose.  So yeah,
> the precise syntax above is not so great.

We might just want to do it _afterward_. We already have some
communication after the import (the "ok ..." lines), so why not add a
"update :1 /refs/heads/master"?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 06/19] git_remote_helpers: push all refs during a non-local export

[Jeff King]

On Thu, Jun 09, 2011 at 03:09:12AM -0500, Jonathan Nieder wrote:

> If we imagine that the remote helper author wants to write as little
> code as possible (which seems reasonable), then probably their
> "export" command will simply feed its input to a vcs-fast-import
> program.  git fast-export doesn't know how to do ref mapping and
> neither would vcs-fast-import.  So one is led to wonder which stage in
> the pipeline can make the adaptations to make "git push hgrepo
> HEAD:refs/heads/bar" work.

Yeah, when writing the original series, I really had the thought that
the remote helper would be sitting in the middle and could do the
mapping. But really, not necessarily; the data is more likely to go
straight to an importer. And the mapping code may be ugly, which means
we want git itself to do the heavy lifting, not individual helpers.

> To be friendly to remote helper authors, it would be nice to take
> care of the ref mapping somewhere on the transport-helper side, unless
> fast-import learns a new mode that does not label its result with
> refs.  In the latter case, the "export" command could look like[*]
> 
> 	export :1 refs/heads/foo
> 	export :2 refs/heads/bar
> 	export :3 +refs/heads/force
> 
> with :1, :2, and :3 being marks in the fast-import stream.

I think we already have something like that with:

  reset refs/heads/foo
  from :1

in the import stream. And then that matches the concept that the helper
is really just pushing all of the work to that VCS's fast-import stream.
And _if_ we can convince fast-export to write a stream of commits that
are not on any particular ref, then we could just dump the ref-mapping
at the end of the export stream.

There's nowhere to talk about "forced" pushing there, though. We could
add in a "force" flag on that reset command. But it's not even
necessarily a concept that will map to other version control systems. I
wonder if it is simply something that we might have to live without
when moving commits between systems.

-Peff

[PATCH 07/19] 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>
---

  Moved to the front for bisectability.

 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

[PATCH 08/19] 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>
---

  Moved to the front for bisectability.

 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 09/19] git-remote-testgit: fix error handling

[Sverre Rabbelier]

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

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

  Moved to the front for bisectability.

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

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

[PATCH 10/19] 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>
---

  This is actually Jonathan's version that makes done required if
  feature done was specified. See also:

	http://mid.gmane.org/20110213094212.GA25435@elie

 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

Re: [PATCH 10/19] fast-import: introduce 'done' command

[Jonathan Nieder]

Sverre Rabbelier wrote:

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

Hoorah!  Thanks for reviving this.

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

Possible approaches, two ugly and one sane:

 A) use setvbuf() to make input line-buffered.  Unfortunately, as
    POSIX tells us:

	Applications should note that many implementations only provide
	line buffering on input from terminal devices.

    So making it actually work involves pseudo-tty craziness.  Not a
    good idea.

 B) leave some room for buffering after the "done" command and make
    fast-import use setvbuf() to control the size of the stdio buffer.
    This is disgusting but it probably works.

 C) make sure the upstream of the pipe does not write anything after
    "done\n" until the "done\n" command has been read.  In other
    words:

	transport-helper>	done
	remote-helper>   	yep, I'm done
	transport-helper>	ah, now that you mention that, here are
	                 	a few more things you could do...

As long as there is nothing after the "done\n" in the pipe buffer when
fast-import reads "done\n", it won't read too far.

This is all made complicated to debug by stdio's habit of closing
streams at exit time, flushing streams at close time, and
repositioning the file offset of seekable input (but not pipes) at
flush time.

Hope that helps.

Re: [PATCH 10/19] fast-import: introduce 'done' command

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 22:03, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  C) make sure the upstream of the pipe does not write anything after
>    "done\n" until the "done\n" command has been read.  In other
>    words:
>
>        transport-helper>       done
>        remote-helper>          yep, I'm done
>        transport-helper>       ah, now that you mention that, here are
>                                a few more things you could do...
>
> As long as there is nothing after the "done\n" in the pipe buffer when
> fast-import reads "done\n", it won't read too far.

This I think makes the most sense. Although I'm not sure it's
currently possible in remote-testgit, as in, I'm not sure the relevant
pipes are connected to the right programs to make feasible. Also, I'm
not really interested in implementing that, or at least, not right
now, I just shamelessly stole your commit message ;).

-- 
Cheers,

Sverre Rabbelier

[PATCH 11/19] 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 from my original series.

 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 12/19] 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>
---

  Changed from the original addressing comments by Jonathan.

 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 13/19] 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>
---

  Changed from the original only to address style nits.

 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 14/19] 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 from my original series.

 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

[PATCH 15/19] transport-helper: update ref status after push with export

[Sverre Rabbelier]

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

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

 Jonathan added for those who wondered what it should do:

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

  So testgit should be getting this information from the
  result of non_local.push(). Another example is if a ref is a
  non-fast-forward, it should probably detect that before even
  exporting it, and report that here.

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

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index a8e47d9..47a30da 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -153,6 +153,9 @@ def do_export(repo, args):
     if not repo.local:
         repo.non_local.push(repo.gitdir)
 
+    print "ok refs/heads/master"
+    print
+
 
 def do_gitdir(repo, args):
     """Stores the location of the gitdir.
diff --git a/transport-helper.c b/transport-helper.c
index 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 15/19] transport-helper: update ref status after push with export

[Jonathan Nieder]

Sverre Rabbelier wrote:

>  Obviously the testgit helper shouldn't just print 'ok' for master,
>  but it demonstrates the idea.
[... and more interesting commentary ...]

The above probably belongs in the commit message.

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

Now the conversation looks like:

	git>          	done [from fast-export]
	remote helper>	ok refs/heads/master
	remote helper>	[blank line]

and after patch 17:

	git>         	done
	remote helper>	ok refs/heads/master
	remote helper>
	git>

which is to say, this implements the "approach C" mentioned in reply
to patch 10 that ensures that the remote helper gets the blank line
telling it disconnect is imminent.  Good to see.

Re: [PATCH 15/19] transport-helper: update ref status after push with export

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 11:10, Jonathan Nieder <jrnieder@gmail.com> wrote:
> which is to say, this implements the "approach C" mentioned in reply
> to patch 10 that ensures that the remote helper gets the blank line
> telling it disconnect is imminent.  Good to see.

Ah, yes it does :). Happy coincidence.

-- 
Cheers,

Sverre Rabbelier

[PATCH 16/19] 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 from my original series. Replaces Peff's workaround from
  his series.

 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 47a30da..88bcb20 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 a4afe38..682f813 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -94,7 +94,7 @@ test_expect_success PYTHON_24 'fetch new branch' '
 	compare_refs public HEAD localclone FETCH_HEAD
 '
 
-test_expect_failure PYTHON_24 'fetch multiple branches' '
+test_expect_success PYTHON_24 '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 16/19] transport-helper: change import semantics

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Currently the helper must somehow guess how many import statements to
> read before it starts outputting its fast-export stream.

I'm guessing one way is to read until EOF.

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

Seems like a good idea to me.  Documentation/git-remote-helpers.txt
could use a corresponding update ---

Wait, now that I look there, wouldn't it be more consistent with
what "push" does to make the semantics

	... A batch sequence of
	one or more import commands is terminated with a blank line.

?  In other words, what was the rationale for making push work like

	push +src1:dst1
	push +src2:dst2
	push +src3:dst3

	[... command stream continues here ...]

instead of

	push
	+src1:dst1
	+src2:dst2
	+src3:dst3

	[... command stream continues ...]

and does it apply for import, too?

Re: [PATCH 16/19] transport-helper: change import semantics

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 22:47, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Wait, now that I look there, wouldn't it be more consistent with
> what "push" does to make the semantics
>
>        ... A batch sequence of
>        one or more import commands is terminated with a blank line.
>
> ?  In other words, what was the rationale for making push work like
>
>        push +src1:dst1
>        push +src2:dst2
>        push +src3:dst3
>
>        [... command stream continues here ...]
>
> instead of
>
>        push
>        +src1:dst1
>        +src2:dst2
>        +src3:dst3
>
>        [... command stream continues ...]
>
> and does it apply for import, too?

I don't know. Shawn added the push capability in ae4efe19 (Move WebDAV
HTTP push under remote-curl, Fri Oct 30 2009) but there is no
rationale for doing it that way there. Shawn, do you remember?

-- 
Cheers,

Sverre Rabbelier

[PATCH 17/19] transport-helper: export is no longer always the last command

[Sverre Rabbelier]

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

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

  Unchanged from my original series, but with commit message updated
  after review.

 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

Re: [PATCH 17/19] transport-helper: export is no longer always the last command

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Now that the remote helper protocol uses the new done command in its
> fast-import streams, export no longer needs to be the last command in
> the stream.
>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
> ...
> 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);

I've looked at fa8c097 (Support remote helpers implementing smart
transports, 2009-12-09) which introduced this no_disconnect_req field,
73b49a7 (remote-helpers: add support for an export command, 2010-03-29)
which added push_refs_with_export() and made it set the field to 1, and
also have read Documentation/git-remote-helpers.txt, but fail to see the
connection between "this command no longer needs to be the last one" and
"we do not set no-disconnect-req to 1, so that we do not send an empty
line when we disconnect the helper".

Could you clarify the logic, perhaps by commenting a bit more where this
field is examined and code changes its behaviour in disconnect_helper()?

Re: [PATCH 17/19] transport-helper: export is no longer always the last command

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 03:07, Junio C Hamano <gitster@pobox.com> wrote:
> Could you clarify the logic, perhaps by commenting a bit more where this
> field is examined and code changes its behaviour in disconnect_helper()?

Previously this bit had to be set, so that we didn't try to write the
trailing \n on a closed socket. Now, the socket is no longer closed,
so we can send the trailing \n again. Does that make sense?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 17/19] transport-helper: export is no longer always the last command

[Jonathan Nieder]

Sverre Rabbelier wrote:
> On Thu, Jun 9, 2011 at 03:07, Junio C Hamano <gitster@pobox.com> wrote:

>> Could you clarify the logic, perhaps by commenting a bit more where this
>> field is examined and code changes its behaviour in disconnect_helper()?
>
> Previously this bit had to be set, so that we didn't try to write the
> trailing \n on a closed socket. Now, the socket is no longer closed,
> so we can send the trailing \n again. Does that make sense?

Yes, but I'm still missing something.  What does the trailing \n ever
have to be written?

"git log -Sdisconnect_helper -- transport-helper.c" doesn't give many
clues.  I imagine it's a way to check whether the child is still alive
and to warn it not to be alarmed when the output end of its input pipe
closes.

Re: [PATCH 17/19] transport-helper: export is no longer always the last command

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 09:51, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Yes, but I'm still missing something.  What does the trailing \n ever
> have to be written?
>
> "git log -Sdisconnect_helper -- transport-helper.c" doesn't give many
> clues.  I imagine it's a way to check whether the child is still alive
> and to warn it not to be alarmed when the output end of its input pipe
> closes.

Yes, the trailing \n is to signal to the helper that the connection is
about to close, allowing it to do whatever cleanup necessarily. It's
kind of like the "done" command for fast-import.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 17/19] transport-helper: export is no longer always the last command

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Thu, Jun 9, 2011 at 09:51, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Yes, but I'm still missing something.  What does the trailing \n ever
>> have to be written?
>>
>> "git log -Sdisconnect_helper -- transport-helper.c" doesn't give many
>> clues.  I imagine it's a way to check whether the child is still alive
>> and to warn it not to be alarmed when the output end of its input pipe
>> closes.
>
> Yes, the trailing \n is to signal to the helper that the connection is
> about to close, allowing it to do whatever cleanup necessarily. It's
> kind of like the "done" command for fast-import.

... which is the kind of clarification I wanted to see as the comment to
document that no_disconnect_req field, whose semantics was unclear even to
two people who are known to know a bit about git and had to ask you to
clarify.

[PATCH 18/19] transport-helper: Use capname for gitdir capability too

[Sverre Rabbelier]

Also properly use capname in the refspec capability.

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

  Unchanged from my original series.

 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

Re: [PATCH 18/19] transport-helper: Use capname for gitdir capability too

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Also properly use capname in the refspec capability.
>
> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>

Thanks.  The old commit message said

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

which seemed like a somewhat helpful thing to mention. :)  I suspect
the after-the-cut comment

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

would be helpful for future readers, too.

Re: [PATCH 18/19] transport-helper: Use capname for gitdir capability too

[Sverre Rabbelier]

Heya,

On Wed, Jun 8, 2011 at 22:54, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thanks.  The old commit message said [...]
> which seemed like a somewhat helpful thing to mention. :)  I suspect
> the after-the-cut comment would be helpful for future readers, too.

Yeah, not sure what happened, but all my commits had zero-to-none
commit messages. Perhaps I just added them while sending out the patch
series last time? Anyway, can't remember, but that's why, fixed
locally :).

-- 
Cheers,

Sverre Rabbelier

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

  Sanity check added as suggested by Jonathan during review.

 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 88bcb20..d882fa3 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)
     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