Hi again,
Ilari Liusvaara wrote:
> This remote helper reflects raw smart remote transport stream back to the
> calling program.
Fundamentally I think remote-fd (unlike remote-ext) suffers from the
"without-a-user" problem: without an example making use of this, it is
hard to know whether we have the interface right.
For example, might the caller want to use the usual in-stream
service identification as in git:// protocol in some cases? Should
it would be possible to do
some-complex-command fd::3
where some-complex-command makes multiple requests?
So personally I would be happier to see remote-fd in contrib/ until
we have at least some experience of what it's like to use.
Anyway, now the documentation is clearer (thanks!). Some nitpicks
follow; patch at end.
> --- /dev/null
> +++ b/Documentation/git-remote-fd.txt
> @@ -0,0 +1,57 @@
> +git-remote-fd(1)
> +=================
> +
> +NAME
> +----
> +git-remote-fd - Reflect smart transport back to caller.
Has an extra period. Might be worth including the word "stream":
git-remote-fd - Reflect a smart transport stream back to caller
This describes what the helper does rather than the URL scheme...
> +SYNOPSIS
> +--------
> +"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
... so maybe the synopsis should describe the remote helper?
'git remote-fd' <name> <infd>[,<outfd>][/<comment>]
> +
> +DESCRIPTION
> +-----------
> +This helper uses specified file descriptors to connect to remote git server.
Maybe it would make sense to clarify that this is not for end-users.
(see patch)
[...]
> +ENVIRONMENT VARIABLES:
> +----------------------
> +
> +$GIT_TRANSLOOP_DEBUG (passed to git)::
> + If set, prints debugging information about various reads/writes.
> +
Nits: other manpages do not use the $ or trailing colon
> +EXAMPLES:
> +---------
> +"fd::17" (as URL)::
> + Connect using socket in file descriptor #17.
[...]
Maybe full commands would be easier to read:
`git fetch fd::17 master`::
Fetch branch master, using a socket with file descriptor 17
to communicate with 'git upload-pack'.
> --- /dev/null
> +++ b/builtin/remote-fd.c
> @@ -0,0 +1,76 @@
[...]
> +static int command_loop()
nit: static int command_loop(void)
Might even be simpler to return void.
> +{
> + char buffer[MAXCOMMAND];
> +
> + while (1) {
> + if (!fgets(buffer, MAXCOMMAND - 1, stdin))
> + exit(0);
This code path is used for errors, no?
if (!fgets(buffer, MAXCOMMAND - 1, stdin)) {
if (ferror(stdin))
die_errno("input error");
exit(0);
}
> + /* Strip end of line characters. */
> + while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
> + buffer[strlen(buffer) - 1] = 0;
Git isspace does not require the cast. Won't the repeated strlen()
be slow?
{
char *p = buffer + strlen(buffer);
while (p > buffer && isspace(*--p))
*p = 0;
}
> +
> + if (!strcmp(buffer, "capabilities")) {
> + printf("*connect\n\n");
> + fflush(stdout);
> + } else if (!strncmp(buffer, "connect ", 8)) {
> + printf("\n");
> + fflush(stdout);
> + return bidirectional_transfer_loop(input_fd,
> + output_fd);
If this returns nonzero, that's a fatal error, right? So
if (bidir...)
exit(128);
exit(0);
> + } else {
> + fprintf(stderr, "Bad command");
> + return 1;
die("bad command %s", buffer);
might be more useful.
> +int cmd_remote_fd(int argc, const char **argv, const char *prefix)
> +{
> + char* end;
> + unsigned long r;
The * should stick to the variable name.
> + r = strtoul(argv[2], &end, 10);
> + input_fd = (int)r;
Why not just
input_fd = (int) strtoul(...
?
> + if ((*end != ',' && *end !='/' && *end) || end == argv[2])
I find
if (end == argv[2] || (*end && ...
more idiomatic, but that is serious nitpicking now.
Anyway, perhaps some of the below can be useful.
---
diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
index c4c0da1..b305c26 100644
--- a/Documentation/git-remote-fd.txt
+++ b/Documentation/git-remote-fd.txt
@@ -3,53 +3,68 @@ git-remote-fd(1)
NAME
----
-git-remote-fd - Reflect smart transport back to caller.
-
+git-remote-fd - Reflect a smart transport stream back to caller
SYNOPSIS
--------
-"fd::<fd>[/<anything>]" or "fd::<infd>,<outfd>[/<anything>]" (as URL)
+'git remote-fd' <remote> (<fd> | <in_fd>,<out_fd>)[/<comment>]
DESCRIPTION
-----------
-This helper uses specified file descriptors to connect to remote git server.
+This is not a command the end user would ever want to run. This
+documentation is meant for people who are writing Porcelain-ish
+programs that need to intercept the data that 'git' transfers.
+
+The 'git remote-fd' helper is used by git to handle requests for a
+repository with a URL such as `fd::3/git.example.com` (see
+linkgit:git-remote-helpers[7]). Such requests are handled by reading
+from and writing to file descriptors inherited from the parent process.
-Data written to <fd> or <outfd> is assumed to make it unmolested to
-git-upload-pack/git-receive-pack/git-upload-archive, and data from that
-program is assumed to be readable unmolested from <fd> or <infd>.
+Data written to '<fd>' or '<out_fd>' is assumed to make it unmolested
+to 'git upload-pack', 'git receive-pack', or 'git upload-archive',
+and data from that program is assumed to be readable unmolested
+from '<fd>' or '<in_fd>'.
-If just <fd> is specified, <fd> is assumed to be socket. Otherwise both
-<infd> and <outfd> are assumed to be pipes.
+If only '<fd>' is specified, '<fd>' is assumed to be a socket.
+Otherwise both '<in_fd>' and '<out_fd>' are assumed to be pipes.
-It is assumed that any handshaking procedures have already been completed
-(such as sending service request for git://) before this helper is started.
+It is assumed that any handshaking procedures (such as sending a
+service request in `git://` protocol) have already been completed
+before this helper is started.
-<anything> can be any string. It is ignored. It is meant for provoding
-information to user in the "URL".
+The '<comment>' following a trailing `/` can be any string.
+It is ignored. It is meant for providing information to the user
+in the URL, in case the URL gets included in an error message
+shown to the end user.
-ENVIRONMENT VARIABLES:
-----------------------
+ENVIRONMENT VARIABLES
+---------------------
-$GIT_TRANSLOOP_DEBUG (passed to git)::
- If set, prints debugging information about various reads/writes.
+'GIT_TRANSLOOP_DEBUG' (passed to git)::
+ If this is set, 'git' will print copious debugging information
+ about the transport data it reads and writes.
-EXAMPLES:
----------
-"fd::17" (as URL)::
- Connect using socket in file descriptor #17.
+EXAMPLES
+--------
+`git fetch fd::17 master`::
+ Fetch branch master, using a socket with file descriptor 17
+ to communicate with 'git upload-pack'.
-"fd::17/foo" (as URL)::
+`git fetch fd::17/foo master`::
Same as above.
-"fd::7,8" (as URL)::
- Connect using pipes in file descriptors #7 and #8. The incoming
- pipe is at fd #7 and the outgoing pipe at fd #8.
+`git push fd::7,8 master`::
+ Push branch master, using two pipes with file descriptors 7
+ and 8 to communicate with 'git receive-pack'.
+ 'git receive-pack'{apostrophe}s output should be readable
+ via the pipe on fd 7 and its input connected to the outgoing
+ pipe on fd 8.
-"fd::7,8/bar" (as URL)::
+`git push fd::7,8/bar master`::
Same as above.
Documentation
---------------
+-------------
Documentation by Ilari Liusvaara and the git list <git@xxxxxxxxxxxxxxx>
GIT
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index 44c174b..7bc62be 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -16,21 +16,28 @@
*
*/
+static const char builtin_remote_fd_usage[] =
+ "git remote-fd <repository> (<fd> | <in_fd>,<out_fd>)[/<comment>]";
+
int input_fd = -1;
int output_fd = -1;
-#define MAXCOMMAND 4096
-
-static int command_loop()
+static NORETURN void command_loop(void)
{
- char buffer[MAXCOMMAND];
+ char buffer[4096];
- while (1) {
- if (!fgets(buffer, MAXCOMMAND - 1, stdin))
+ for (;;) {
+ if (!fgets(buffer, sizeof(buffer) - 1, stdin)) {
+ if (ferror(stdin))
+ die_errno("input error");
exit(0);
+ }
/* Strip end of line characters. */
- while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
- buffer[strlen(buffer) - 1] = 0;
+ {
+ char *p = buffer + strlen(buffer);
+ while (p > buffer && isspace(*--p))
+ *p = 0;
+ }
if (!strcmp(buffer, "capabilities")) {
printf("*connect\n\n");
@@ -38,39 +45,34 @@ static int command_loop()
} else if (!strncmp(buffer, "connect ", 8)) {
printf("\n");
fflush(stdout);
- return bidirectional_transfer_loop(input_fd,
- output_fd);
+ if (bidirectional_transfer_loop(input_fd, output_fd))
+ exit(128);
+ exit(0);
} else {
- fprintf(stderr, "Bad command");
- return 1;
+ die("bad command %s", buffer);
}
}
}
int cmd_remote_fd(int argc, const char **argv, const char *prefix)
{
- char* end;
- unsigned long r;
+ char *end, *end2;
if (argc < 3)
- die("URL missing");
-
- r = strtoul(argv[2], &end, 10);
- input_fd = (int)r;
+ usage(builtin_remote_fd_usage);
- if ((*end != ',' && *end !='/' && *end) || end == argv[2])
+ input_fd = (int) strtoul(argv[2], &end, 10);
+ if (end == argv[2] || (*end && *end != ',' && *end != '/'))
die("Bad URL syntax");
- if (*end == '/' || !*end) {
+ if (*end != ',') { /* fd::fd[/comment] */
output_fd = input_fd;
- } else {
- char* end2;
- r = strtoul(end + 1, &end2, 10);
- output_fd = (int)r;
-
- if ((*end2 !='/' && *end2) || end2 == end + 1)
- die("Bad URL syntax");
+ command_loop();
}
- return command_loop();
+ /* fd::in_fd,out_fd[/comment] */
+ output_fd = (int) strtoul(end + 1, &end2, 10);
+ if (end2 == end + 1 || (*end2 && *end2 != '/'))
+ die("Bad URL syntax");
+ command_loop();
}
--
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html