threads / patch / 27583

patch, 19 partsremote-helper improvements

Subject: [PATCH 00/19] remote-helper improvements

## tl;dr

54 messages between Jun 8, 2011 and Jun 13, 2011. Diffs are folded; open one to read it.

replies: 53people: 4as markdown or json

Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · lore

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(-)
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to transport-helper.c +1 −0
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
Jeff King· Jun 8, 2011, 21:57 UTC · re: Sverre Rabbelier · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 22:08 UTC · re: Jeff King · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to t/t5800-remote-helpers.sh +8 −4
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to t/t5800-remote-helpers.sh +47 −0
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
Jonathan Nieder· Jun 8, 2011, 19:28 UTC · re: Sverre Rabbelier · lore

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

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

[...]
Show 7 quoted lines
> 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. :)

Show 7 quoted lines
> --- 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?
[...]
Show 6 quoted lines
> +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
Jonathan Nieder· Jun 8, 2011, 19:36 UTC · re: Jonathan Nieder · lore

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

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.

Show 6 quoted lines
> 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.
Sverre Rabbelier· Jun 8, 2011, 19:51 UTC · re: Jonathan Nieder · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 21:13 UTC · re: Jonathan Nieder · lore

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

Heya,
On Wed, Jun 8, 2011 at 21:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
>> +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
Sverre Rabbelier· Jun 9, 2011, 12:45 UTC · re: Sverre Rabbelier · lore

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

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
Jeff King· Jun 10, 2011, 01:18 UTC · re: Jonathan Nieder · lore

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

On Wed, Jun 08, 2011 at 02:28:51PM -0500, Jonathan Nieder wrote:
Show 10 quoted lines
> > 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).

Show 7 quoted lines
> 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. :)

Show 17 quoted lines
> > --- 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.

Show 10 quoted lines
> > +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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to 3 files +9 −3

git-remote-testgit.py, git_remote_helpers/git/exporter.py, t/t5800-remote-helpers.sh

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
Sverre Rabbelier· Jun 8, 2011, 19:47 UTC · re: Jonathan Nieder · lore

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

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
Jeff King· Jun 8, 2011, 22:15 UTC · re: Sverre Rabbelier · lore

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

On Wed, Jun 08, 2011 at 09:47:38PM +0200, Sverre Rabbelier wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Jun 8, 2011, 23:48 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 17 quoted lines
> 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.
Sverre Rabbelier· Jun 9, 2011, 06:23 UTC · re: Junio C Hamano · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 05/19] transport-helper: don't feed bogus refs to export push

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(-)
Show changes to 2 files +3 −2

t/t5800-remote-helpers.sh, transport-helper.c

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to 2 files +2 −2

git_remote_helpers/git/non_local.py, t/t5800-remote-helpers.sh

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
Jonathan Nieder· Jun 8, 2011, 19:42 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
Show 9 quoted lines
> --- 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.)

Jeff King· Jun 8, 2011, 22:19 UTC · re: Jonathan Nieder · lore

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

On Wed, Jun 08, 2011 at 02:42:05PM -0500, Jonathan Nieder wrote:
Show 14 quoted lines
> 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
Sverre Rabbelier· Jun 8, 2011, 22:21 UTC · re: Jeff King · lore

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

Heya,
On Thu, Jun 9, 2011 at 00:19, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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
Jonathan Nieder· Jun 9, 2011, 08:09 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 8 quoted lines
> 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.

Sverre Rabbelier· Jun 9, 2011, 08:29 UTC · re: Jonathan Nieder · lore

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

Heya,
On Thu, Jun 9, 2011 at 10:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
>        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
Jonathan Nieder· Jun 9, 2011, 08:43 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
> On Thu, Jun 9, 2011 at 10:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
>>        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.

Sverre Rabbelier· Jun 9, 2011, 10:26 UTC · re: Jonathan Nieder · lore

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

Heya,
On Thu, Jun 9, 2011 at 10:43, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 13 quoted lines
> 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
Jeff King· Jun 10, 2011, 01:40 UTC · re: Jonathan Nieder · lore

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

On Thu, Jun 09, 2011 at 03:09:12AM -0500, Jonathan Nieder wrote:
Show 7 quoted lines
> 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.

Show 10 quoted lines
> 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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 07/19] remote-curl: accept empty line as terminator

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(-)
Show changes to remote-curl.c +3 −0
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 08/19] git-remote-testgit: only push for non-local repositories

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(-)
Show changes to git-remote-testgit.py +3 −1
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to git_remote_helpers/git/importer.py +3 −2
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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

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

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

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
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(-)
Show changes to 3 files +75 −0

Documentation/git-fast-import.txt, fast-import.c, t/t9300-fast-import.sh

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
Jonathan Nieder· Jun 8, 2011, 20:03 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
Show 6 quoted lines
> 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.
Show 6 quoted lines
> 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.
Sverre Rabbelier· Jun 8, 2011, 20:07 UTC · re: Jonathan Nieder · lore

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

Heya,
On Wed, Jun 8, 2011 at 22:03, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
>  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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 11/19] fast-export: support done feature

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(-)
Show changes to 2 files +13 −0

Documentation/git-fast-export.txt, builtin/fast-export.c

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 12/19] transport-helper: factor out push_update_refs_status

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(-)
Show changes to transport-helper.c +84 −69
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to transport-helper.c +15 −8
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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

[PATCH 14/19] transport-helper: use the new done feature where possible

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(-)
Show changes to 2 files +4 −6

git-remote-testgit.py, transport-helper.c

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to 2 files +4 −0

git-remote-testgit.py, transport-helper.c

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
Jonathan Nieder· Jun 9, 2011, 09:10 UTC · re: Sverre Rabbelier · lore

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

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.
[...]
Show 7 quoted lines
> --- 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.

Sverre Rabbelier· Jun 9, 2011, 10:23 UTC · re: Jonathan Nieder · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to 3 files +18 −5

git-remote-testgit.py, t/t5800-remote-helpers.sh, transport-helper.c

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
Jonathan Nieder· Jun 8, 2011, 20:47 UTC · re: Sverre Rabbelier · lore

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

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.
Show 9 quoted lines
> 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?
Sverre Rabbelier· Jun 8, 2011, 20:52 UTC · re: Jonathan Nieder · lore

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

Heya,
On Wed, Jun 8, 2011 at 22:47, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 24 quoted lines
> 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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to transport-helper.c +0 −1
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
Junio C Hamano· Jun 9, 2011, 01:07 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 19 quoted lines
> 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()?

Sverre Rabbelier· Jun 9, 2011, 06:48 UTC · re: Junio C Hamano · lore

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

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
Jonathan Nieder· Jun 9, 2011, 07:51 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
> On Thu, Jun 9, 2011 at 03:07, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>> 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.

Sverre Rabbelier· Jun 9, 2011, 08:28 UTC · re: Jonathan Nieder · lore

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

Heya,
On Thu, Jun 9, 2011 at 09:51, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Jun 13, 2011, 15:24 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 12 quoted lines
> 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.

Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to transport-helper.c +2 −2
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
Jonathan Nieder· Jun 8, 2011, 20:54 UTC · re: Sverre Rabbelier · lore

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

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.
Sverre Rabbelier· Jun 8, 2011, 20:57 UTC · re: Jonathan Nieder · lore

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

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
Sverre Rabbelier· Jun 8, 2011, 18:48 UTC · re: Sverre Rabbelier · lore

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

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

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(-)
Show changes to 2 files +36 −42

git-remote-testgit.py, transport-helper.c

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

← back to recent threads