Ilari Liusvaara wrote:
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@xxxxxxxxxxx>
Just a few random nits.
> +++ b/builtin/remote-ext.c
> @@ -0,0 +1,243 @@
[...]
[...]
> + default:
> + die("Bad remote-ext placeholder '\\%c'.",
> + str[rpos]);
Should be "Bad remote-ext placeholder '%%%c'.", I think.
> +static const char **parse_argv(const char *arg, const char *service)
> +{
> + int arguments = 0;
> + int i;
> + char **ret;
> + char *(temparray[MAXARGUMENTS + 1]);
Would be more idiomatic to leave out the parentheses.
> +
> + while (*arg) {
> + char *ret;
Would be clearer without shadowing. Maybe
char *expanded;
[...]
> + ret = xcalloc(arguments + 1, sizeof(char *));
> + for (i = 0; i < arguments; i++)
> + ret[i] = temparray[i];
> +
> + return (const char **)ret;
Maybe
ret = xmalloc(...
for (...
ret[arguments] = NULL;
return ret;
(to avoid clearing memory that is about to be overwritten) or even
ret = xmalloc(...
memcpy(ret, temparray, arguments * sizeof(*ret));
ret[arguments] = NULL;
return ret;
(to emphasize the copy) would make sense?
Why is ret of type char ** and then cast away rather than being
const char ** in the first place?
> +static int command_loop(const char *child)
> +{
> + char buffer[MAXCOMMAND];
> +
> + while (1) {
> + if (!fgets(buffer, MAXCOMMAND - 1, stdin))
> + exit(0);
Won't this exit(0) for I/O errors?
> + /* Strip end of line characters. */
> + while (isspace((unsigned char)buffer[strlen(buffer) - 1]))
> + buffer[strlen(buffer) - 1] = 0;
This is wasteful: strlen() has to look for the null byte three times
just to strip a \r\n from the end. See remote-fd for an example of
how to avoid that overhead.
--
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