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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 8, 2011, 19:28 UTC
Message-ID: <20110608192850.GF27715@elie>
In-Reply-To: <1307558930-16074-4-git-send-email-srabbelier@gmail.com>

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