From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 30 Sep 2010 13:14:40 GMT
Subject: Re: [PATCH v2 2/3] git-remote-fd
Message-ID: <20100930131440.GE4850@burratino>
In-Reply-To: <1285847579-21954-3-git-send-email-ilari.liusvaara@elisanet.fi>

Hi Ilari,

Ilari Liusvaara wrote:

> This remote helper reflects raw smart remote transport stream back to the
> calling program. This is useful for example if some UI wants to handle
> ssh itself and not use hacks via GIT_SSH.

Do you ever use this directly, or is it a "because we can" sort of thing?
Whenever I try to come up with an example, ext:: seems to be easier.

An example (ideally a test) would be nice.

> +++ b/Documentation/git-remote-fd.txt
> @@ -0,0 +1,57 @@
[...]
> +"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
[...]
> +<anything> can be any string. It is ignored. It is meant for provoding
> +information to user in form of "URL".

Is this meant for future expansion?  Why not just use a comment in
.git/config?

> @@ -0,0 +1,57 @@
> +ENVIRONMENT VARIABLES:
> +----------------------
> +
> +$GIT_TRANSLOOP_DEBUG (passed to git)::
> +	If set, prints debugging information about various reads/writes.

For the curious: introduced by patch 1.

> +EXAMPLES:
> +---------
> +"fd::17"::
> +	Connect using socket in file descriptor #17.

Maybe with more context?

	socat ... "git push fd::3 HEAD" ...

> +Documentation
> +--------------
> +Documentation by Ilari Liusvaara.

I think one tends to write "Documentation by <person> and the git list
<git@vger.kernel.org>" these days.

Why is this section needed at all?  Doesn't the changelog already
record the relevant information?

> --- /dev/null
> +++ b/builtin/remote-fd.c
> @@ -0,0 +1,88 @@
> +#include "git-compat-util.h"
> +#include "transport.h"
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <unistd.h>

Doesn't git-compat-util.h imply these?

> + * The data output to <outfd>/<inoutfd> should be passed unmolested to
> + * git-receive-pack/git-upload-pack/git-upload-archive and output of
> + * git-receive-pack/git-upload-pack/git-upload-archive should be passed
> + * unmolested to <infd>/<inoutfd>.

Maybe worth mentioning in the manual?

> +static int command_loop()

Simple, I like it. :)

> +int cmd_remote_fd(int argc, const char **argv, const char *prefix)
> +{
> +	char* end;
> +	unsigned long r;
> +
> +	if (argc < 3) {
> +		fprintf(stderr, "Error: URL missing");
> +		exit(1);

Why not use usage() or die()?  (Just curious.)