Re: [PATCH v2 3/3] git-remote-ext

[Jonathan Nieder <jrnieder@gmail.com>]

Ilari Liusvaara wrote:

> This remote helper invokes external command and passes raw smart transport
> stream through it. This is useful for instance for invoking ssh with
> one-off odd options, connecting to git services in unix domain
> sockets, in abstract namespace, using TLS or other secure protocols,
> etc...

Tunneling, too (e.g., native git protocol passing through draconian
firewall), right?

>  Documentation/git-remote-ext.txt |   87 ++++++++++++++++++++++++++++++++++++++
>  Makefile                         |    1 +
>  builtin.h                        |    1 +
>  git.c                            |    1 +
>  4 files changed, 90 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/git-remote-ext.txt

Where is the implementation?

> +++ b/Documentation/git-remote-ext.txt
> @@ -0,0 +1,87 @@
> +git-remote-ext(1)
> +=================
> +
> +NAME
> +----
> +git-remote-ext - Bridge smart transport to external command.
> +
> +
> +SYNOPSIS
> +--------
> +"ext::<command>[ <arguments>...]" (as URL)

Maybe:

	git remote add nick "ext::<program>[ <arguments>...]"

as a concrete example.

> +
> +DESCRIPTION
> +-----------
> +This command uses specified command to connect to remote git server.

 - Most users won't invoke remote-ext directly, right?
 - Missing articles ('the' and 'a').
 - Missing formatting ('command' is passed on the command line).

So maybe:

	This remote helper uses the specified 'program' to connect
	to a remote git server.

> +
> +Between <command> and <arguments> (if present) is space. Also space
> +splits different arguments.

	Arguments should be separated by a single unescaped space.

Do I understand correctly?

> +
> +The following sequences have special meaning:

Missing article:

	... have a special meaning:

> +
> +'\ '::
> +	Don't interpret the space as command/argument separator.

'\ '::
	Literal space in 'program' or an argument.

> +
> +'\\'::
> +	Literal backslash

Missing period.

> +'\s' (as argument)::
> +	Replaced by short name (receive-pack, upload-pack, upload-archive)
> +	of service git wants to invoke.

'\s'::
	Name (receive-pack, upload-pack, or upload-archive) of the
	service git wants to invoke.  Can only be used as an entire
	argument (like "ext::foo \s", not "ext::foo BLAH\sBLAH"),

Is that right?

> +'\S' (as argument)::
> +	Replaced by long name (git-receive-pack, git-upload-pack,
> +	git-upload-archive) of service git wants to invoke.

'\S'::
	Long name (git-receive-pack, ...

Does this really mean "name + 'git-'", or does it respect the
fetch-pack --upload-pack et al options?

> +'\G<repository>' (as argument)::
> +	This argument will not be passed to command. Instead, git will send
> +	in-line git:// service request for <repository>. Default is not to
> +	send in-line request.

'\G<repository>'::
	This argument will not be passed to 'program'. Instead, ...

Huh?  What is an in-line git://service request?

> +'\V<host>' (as argument)::
> +	Set the vhost used in in-line git:// service request. Default is
> +	to omit vhost.

Likewise.

> +ENVIRONMENT VARIABLES:
> +----------------------
> +
> +$GIT_EXT_SERVICE (passed to command)::
> +	Initialzed to long name of service git wants to invoke.

The existing manual pages tend to use 'italics' and leave out the $
here.

Maybe the environment passed to the command deserves its own section?
Just nitpicking.

s/Initialzed/Initialized/?  s/long name/the long name/? etc.

> +EXAMPLES:
> +---------

Maybe some introductory text would help.  E.g:

	This remote helper is transparently used by git when
	you use commands such as "git fetch <URL>" where <URL>
	begins with `ext::`.  Examples:

> +"ext::ssh -i /home/foo/.ssh/somekey user@host.example \S \'foo/repo'"::
> +	Use  /home/foo/.ssh/somekey as key when connecting to host.example
> +	and request repo foo/repo.

Probably worth mentioning this avoids adding a nickname and stanza
for this remote in ~/.ssh/config?

An address doesn't really request anything on its own.  Maybe saying
what they point to would be clearer?

	Represents a repository accessible at host.example:foo/repo
	when connecting as user "user" with private key "~foo/.ssh/somekey".

> +"ext::socat -t3600 - ABSTRACT-CONNECT:/git-server \G/somerepo"::
> +	Connect to git:// server named '/git-server' in abstract namespace
> +	and request '/somerepo' from it.

	Represents a repository with path /somerepo accessible over
	git protocol at Unix-domain socket address "/git-server".

> +"ext::git-server-alias foo \G/repo"::
> +	Connect to wherever 'git-server-alias foo' connects to and send
> +	git:// request there for '/repo'.

	Represents a repository with path /repo accessed using the
	helper program "git-server-alias foo".  The path to the
	repository and type of request are not passed on the command
	line but as part of the protocol stream, as usual with git://
	protocol.

> +"ext::git-server-alias foo \G/repo \Vfoo"::
> +	Connect to wherever 'git-server-alias foo' connects to and send
> +	git:// request there for '/repo' using vhost 'foo'.

	Represents a repository with path /repo accessed using the
	helper program "git-server-alias foo".  The hostname for the
	remote server passed in the protocol stream will be "foo"
	(this allows multiple virtual git servers to share a
	link-level address).

> +"ext::git-ssl foo.example /bar"::
> +	Connect to whatever repo 'git-ssl foo.example /bar' goes.

	Represents a repository accessed using the helper program
	"git-ssl foo.example /bar".  The type of request can be
	determined by the helper using environment variables (see
	above).

> --- a/Makefile
> +++ b/Makefile
[...]
> --- a/builtin.h
> +++ b/builtin.h
[...]
This boilerplate looks good, but where's the command?

> --- a/git.c
> +++ b/git.c
> @@ -374,6 +374,7 @@ static void handle_internal_command(int argc, const char **argv)
>  		{ "receive-pack", cmd_receive_pack },
>  		{ "reflog", cmd_reflog, RUN_SETUP },
>  		{ "remote", cmd_remote, RUN_SETUP },
> +		{ "remote-ext", cmd_remote_ext, 0 },
>  		{ "remote-fd", cmd_remote_fd, 0 },

The style of surrounding entries is to leave off the "0" where it can
be inferred like this.

Thanks for a pleasant read.