Re: [RFC PATCH v6 3/3] git-remote-ext

Ilari Liusvaara wrote:

Signed-off-by: Ilari Liusvaara <redacted>

Just a few random nits.

[...]
[...]

+			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.