[PATCH v3 0/4] port upload-archive to Windows

[PATCH v3 0/4] port upload-archive to Windows

[Erik Faye-Lund]

It's been a while, but here's another updated version of this
series.

The only change since last time is that the series has been made
compatible with Peff's "when remote, disable some features"
changes.

Erik Faye-Lund (4):
  compat/win32/sys/poll.c: upgrade from upstream
  mingw: fix compilation of poll-emulation
  enter_repo: do not modify input
  upload-archive: use start_command instead of fork

 builtin/archive.c        |    6 +++-
 builtin/upload-archive.c |   68 ++++++++++++++-------------------------------
 cache.h                  |    2 +-
 compat/mingw.h           |    2 -
 compat/win32/sys/poll.c  |   17 ++++++++---
 daemon.c                 |    4 +-
 path.c                   |   30 +++++++++-----------
 t/t5000-tar-tree.sh      |   10 +++---
 8 files changed, 60 insertions(+), 79 deletions(-)

-- 
1.7.6.355.g842ba.dirty

[PATCH v3 1/4] compat/win32/sys/poll.c: upgrade from upstream

[Erik Faye-Lund]

poll.c is updated from revision adc3a5b in
git://git.savannah.gnu.org/gnulib.git

The changes are applied with --whitespace=fix to reduce noise.

poll.h is not upgraded, because the most recent version now
contains template-stuff that breaks compilation for us.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/win32/sys/poll.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
index 708a6c9..403eaa7 100644
--- a/compat/win32/sys/poll.c
+++ b/compat/win32/sys/poll.c
@@ -1,7 +1,7 @@
 /* Emulation for poll(2)
    Contributed by Paolo Bonzini.
 
-   Copyright 2001-2003, 2006-2010 Free Software Foundation, Inc.
+   Copyright 2001-2003, 2006-2011 Free Software Foundation, Inc.
 
    This file is part of gnulib.
 
@@ -27,7 +27,10 @@
 #include <malloc.h>
 
 #include <sys/types.h>
-#include "poll.h"
+
+/* Specification.  */
+#include <poll.h>
+
 #include <errno.h>
 #include <limits.h>
 #include <assert.h>
@@ -314,10 +317,7 @@ compute_revents (int fd, int sought, fd_set *rfds, fd_set *wfds, fd_set *efds)
 #endif /* !MinGW */
 
 int
-poll (pfd, nfd, timeout)
-     struct pollfd *pfd;
-     nfds_t nfd;
-     int timeout;
+poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 {
 #ifndef WIN32_NATIVE
   fd_set rfds, wfds, efds;
@@ -454,6 +454,7 @@ poll (pfd, nfd, timeout)
   if (!hEvent)
     hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);
 
+restart:
   handle_array[0] = hEvent;
   nhandles = 1;
   FD_ZERO (&rfds);
@@ -594,6 +595,12 @@ poll (pfd, nfd, timeout)
 	rc++;
     }
 
+  if (!rc && timeout == INFTIM)
+    {
+      SwitchToThread();
+      goto restart;
+    }
+
   return rc;
 #endif
 }
-- 
1.7.6.355.g842ba.dirty

[PATCH v3 2/4] mingw: fix compilation of poll-emulation

[Erik Faye-Lund]

gnulib has changed the inclusion of poll.h from double quotes
to single-quotes. But because compat/win32/sys/ isn't in our
include-path, this breaks compilation. Change it back the way
it was.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/win32/sys/poll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
index 403eaa7..225ddce 100644
--- a/compat/win32/sys/poll.c
+++ b/compat/win32/sys/poll.c
@@ -29,7 +29,7 @@
 #include <sys/types.h>
 
 /* Specification.  */
-#include <poll.h>
+#include "poll.h"
 
 #include <errno.h>
 #include <limits.h>
-- 
1.7.6.355.g842ba.dirty

[PATCH v3 3/4] enter_repo: do not modify input

[Erik Faye-Lund]

entr_repo(..., 0) currently modifies the input to strip away
trailing slashes. This means that we some times need to copy the
input to keep the original.

Change it to unconditionally copy it into the used_path buffer so
we can safely use the input without having to copy it.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 cache.h  |    2 +-
 daemon.c |    4 ++--
 path.c   |   30 ++++++++++++++----------------
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/cache.h b/cache.h
index 607c2ea..a7e4de1 100644
--- a/cache.h
+++ b/cache.h
@@ -734,7 +734,7 @@ int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
-char *enter_repo(char *path, int strict);
+const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
 	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
diff --git a/daemon.c b/daemon.c
index 4c8346d..9253192 100644
--- a/daemon.c
+++ b/daemon.c
@@ -108,11 +108,11 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }
 
-static char *path_ok(char *directory)
+static const char *path_ok(char *directory)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
-	char *path;
+	const char *path;
 	char *dir;
 
 	dir = directory;
diff --git a/path.c b/path.c
index 6f3f5d5..f7dfd0b 100644
--- a/path.c
+++ b/path.c
@@ -283,7 +283,7 @@ return_null:
  * links.  User relative paths are also returned as they are given,
  * except DWIM suffixing.
  */
-char *enter_repo(char *path, int strict)
+const char *enter_repo(const char *path, int strict)
 {
 	static char used_path[PATH_MAX];
 	static char validated_path[PATH_MAX];
@@ -297,14 +297,15 @@ char *enter_repo(char *path, int strict)
 		};
 		int len = strlen(path);
 		int i;
-		while ((1 < len) && (path[len-1] == '/')) {
-			path[len-1] = 0;
+		while ((1 < len) && (path[len-1] == '/'))
 			len--;
-		}
+
 		if (PATH_MAX <= len)
 			return NULL;
-		if (path[0] == '~') {
-			char *newpath = expand_user_path(path);
+		strncpy(used_path, path, len);
+
+		if (used_path[0] == '~') {
+			char *newpath = expand_user_path(used_path);
 			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
 				free(newpath);
 				return NULL;
@@ -316,24 +317,21 @@ char *enter_repo(char *path, int strict)
 			 * anyway.
 			 */
 			strcpy(used_path, newpath); free(newpath);
-			strcpy(validated_path, path);
-			path = used_path;
+			strcpy(validated_path, used_path);
 		}
 		else if (PATH_MAX - 10 < len)
 			return NULL;
-		else {
-			path = strcpy(used_path, path);
-			strcpy(validated_path, path);
-		}
-		len = strlen(path);
+		else
+			strcpy(validated_path, used_path);
+		len = strlen(used_path);
 		for (i = 0; suffix[i]; i++) {
-			strcpy(path + len, suffix[i]);
-			if (!access(path, F_OK)) {
+			strcpy(used_path + len, suffix[i]);
+			if (!access(used_path, F_OK)) {
 				strcat(validated_path, suffix[i]);
 				break;
 			}
 		}
-		if (!suffix[i] || chdir(path))
+		if (!suffix[i] || chdir(used_path))
 			return NULL;
 		path = validated_path;
 	}
-- 
1.7.6.355.g842ba.dirty

[PATCH v3 4/4] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

The POSIX-function fork is not supported on Windows. Use our
start_command API instead.

As this is the last call-site that depends on the fork-stub in
compat/mingw.h, remove that as well.

Add an undocumented flag to git-archive that tells it that the
action originated from a remote, so features can be disabled.
Thanks to Jeff King for work on this part.

Remove the NOT_MINGW-prereq for t5000, as git-archive --remote
now works.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Helped-by: Jeff King <peff@peff.net>
---
 builtin/archive.c        |    6 +++-
 builtin/upload-archive.c |   68 ++++++++++++++-------------------------------
 compat/mingw.h           |    2 -
 t/t5000-tar-tree.sh      |   10 +++---
 4 files changed, 31 insertions(+), 55 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 883c009..6668340 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -85,6 +85,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 	const char *exec = "git-upload-archive";
 	const char *output = NULL;
 	const char *remote = NULL;
+	int is_remote = 0;
 	struct option local_opts[] = {
 		OPT_STRING('o', "output", &output, "file",
 			"write the archive to this file"),
@@ -92,6 +93,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 			"retrieve the archive from remote repository <repo>"),
 		OPT_STRING(0, "exec", &exec, "cmd",
 			"path to the remote git-upload-archive command"),
+		{ OPTION_BOOLEAN, 0, "remote-request", &is_remote, NULL,
+			"indicate we are serving a remote request",
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 		OPT_END()
 	};
 
@@ -106,5 +110,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
-	return write_archive(argc, argv, prefix, 1, output, 0);
+	return write_archive(argc, argv, prefix, 1, output, is_remote);
 }
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2d0b383..c57e8bd 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -6,6 +6,7 @@
 #include "archive.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "run-command.h"
 
 static const char upload_archive_usage[] =
 	"git upload-archive <repo>";
@@ -18,28 +19,17 @@ static const char lostchild[] =
 
 #define MAX_ARGS (64)
 
-static int run_upload_archive(int argc, const char **argv, const char *prefix)
+static void prepare_argv(const char **sent_argv, const char **argv)
 {
-	const char *sent_argv[MAX_ARGS];
 	const char *arg_cmd = "argument ";
 	char *p, buf[4096];
 	int sent_argc;
 	int len;
 
-	if (argc != 2)
-		usage(upload_archive_usage);
-
-	if (strlen(argv[1]) + 1 > sizeof(buf))
-		die("insanely long repository name");
-
-	strcpy(buf, argv[1]); /* enter-repo smudges its argument */
-
-	if (!enter_repo(buf, 0))
-		die("'%s' does not appear to be a git repository", buf);
-
 	/* put received options in sent_argv[] */
-	sent_argc = 1;
-	sent_argv[0] = "git-upload-archive";
+	sent_argc = 2;
+	sent_argv[0] = "archive";
+	sent_argv[1] = "--remote-request";
 	for (p = buf;;) {
 		/* This will die if not enough free space in buf */
 		len = packet_read_line(0, p, (buf + sizeof buf) - p);
@@ -62,9 +52,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 		*p++ = 0;
 	}
 	sent_argv[sent_argc] = NULL;
-
-	/* parse all options sent by the client */
-	return write_archive(sent_argc, sent_argv, prefix, 0, NULL, 1);
 }
 
 __attribute__((format (printf, 1, 2)))
@@ -96,38 +83,25 @@ static ssize_t process_input(int child_fd, int band)
 
 int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
-	pid_t writer;
-	int fd1[2], fd2[2];
-	/*
-	 * Set up sideband subprocess.
-	 *
-	 * We (parent) monitor and read from child, sending its fd#1 and fd#2
-	 * multiplexed out to our fd#1.  If the child dies, we tell the other
-	 * end over channel #3.
-	 */
-	if (pipe(fd1) < 0 || pipe(fd2) < 0) {
-		int err = errno;
-		packet_write(1, "NACK pipe failed on the remote side\n");
-		die("upload-archive: %s", strerror(err));
-	}
-	writer = fork();
-	if (writer < 0) {
+	const char *sent_argv[MAX_ARGS];
+	struct child_process cld = { sent_argv };
+	cld.out = cld.err = -1;
+	cld.git_cmd = 1;
+
+	if (argc != 2)
+		usage(upload_archive_usage);
+
+	if (!enter_repo(argv[1], 0))
+		die("'%s' does not appear to be a git repository", argv[1]);
+
+	prepare_argv(sent_argv, argv);
+	if (start_command(&cld)) {
 		int err = errno;
 		packet_write(1, "NACK fork failed on the remote side\n");
 		die("upload-archive: %s", strerror(err));
 	}
-	if (!writer) {
-		/* child - connect fd#1 and fd#2 to the pipe */
-		dup2(fd1[1], 1);
-		dup2(fd2[1], 2);
-		close(fd1[1]); close(fd2[1]);
-		close(fd1[0]); close(fd2[0]); /* we do not read from pipe */
-
-		exit(run_upload_archive(argc, argv, prefix));
-	}
 
 	/* parent - read from child, multiplex and send out to fd#1 */
-	close(fd1[1]); close(fd2[1]); /* we do not write to pipe */
 	packet_write(1, "ACK\n");
 	packet_flush(1);
 
@@ -135,9 +109,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		struct pollfd pfd[2];
 		int status;
 
-		pfd[0].fd = fd1[0];
+		pfd[0].fd = cld.out;
 		pfd[0].events = POLLIN;
-		pfd[1].fd = fd2[0];
+		pfd[1].fd = cld.err;
 		pfd[1].events = POLLIN;
 		if (poll(pfd, 2, -1) < 0) {
 			if (errno != EINTR) {
@@ -156,7 +130,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 			if (process_input(pfd[0].fd, 1))
 				continue;
 
-		if (waitpid(writer, &status, 0) < 0)
+		if (waitpid(cld.pid, &status, 0) < 0)
 			error_clnt("%s", lostchild);
 		else if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
 			error_clnt("%s", deadchild);
diff --git a/compat/mingw.h b/compat/mingw.h
index ce9dd98..9cb6eb9 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -85,8 +85,6 @@ static inline int symlink(const char *oldpath, const char *newpath)
 { errno = ENOSYS; return -1; }
 static inline int fchmod(int fildes, mode_t mode)
 { errno = ENOSYS; return -1; }
-static inline pid_t fork(void)
-{ errno = ENOSYS; return -1; }
 static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index d906898..889842e 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -96,7 +96,7 @@ test_expect_success 'git archive with --output' \
     'git archive --output=b4.tar HEAD &&
     test_cmp b.tar b4.tar'
 
-test_expect_success NOT_MINGW 'git archive --remote' \
+test_expect_success 'git archive --remote' \
     'git archive --remote=. HEAD >b5.tar &&
     test_cmp b.tar b5.tar'
 
@@ -266,7 +266,7 @@ test_expect_success 'archive --list mentions user filter' '
 	grep "^bar\$" output
 '
 
-test_expect_success NOT_MINGW 'archive --list shows only enabled remote filters' '
+test_expect_success 'archive --list shows only enabled remote filters' '
 	git archive --list --remote=. >output &&
 	! grep "^tar\.foo\$" output &&
 	grep "^bar\$" output
@@ -298,7 +298,7 @@ test_expect_success 'extension matching requires dot' '
 	test_cmp b.tar config-implicittar.foo
 '
 
-test_expect_success NOT_MINGW 'only enabled filters are available remotely' '
+test_expect_success 'only enabled filters are available remotely' '
 	test_must_fail git archive --remote=. --format=tar.foo HEAD \
 		>remote.tar.foo &&
 	git archive --remote=. --format=bar >remote.bar HEAD &&
@@ -341,12 +341,12 @@ test_expect_success GZIP,GUNZIP 'extract tgz file' '
 	test_cmp b.tar j.tar
 '
 
-test_expect_success GZIP,NOT_MINGW 'remote tar.gz is allowed by default' '
+test_expect_success GZIP 'remote tar.gz is allowed by default' '
 	git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&
 	test_cmp j.tgz remote.tar.gz
 '
 
-test_expect_success GZIP,NOT_MINGW 'remote tar.gz can be disabled' '
+test_expect_success GZIP 'remote tar.gz can be disabled' '
 	git config tar.tar.gz.remote false &&
 	test_must_fail git archive --remote=. --format=tar.gz HEAD \
 		>remote.tar.gz
-- 
1.7.6.355.g842ba.dirty

Re: [PATCH v3 0/4] port upload-archive to Windows

[Erik Faye-Lund]

On Thu, Sep 29, 2011 at 10:59 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> It's been a while, but here's another updated version of this
> series.
>
> The only change since last time is that the series has been made
> compatible with Peff's "when remote, disable some features"
> changes.

Oh, and I forgot to mention here in the cover-letter: I dropped the
NO_MINGW-prereq from the archive --remote tests as well.

Re: [PATCH v3 0/4] port upload-archive to Windows

[Jeff King]

On Thu, Sep 29, 2011 at 10:59:19PM +0200, Erik Faye-Lund wrote:

> It's been a while, but here's another updated version of this
> series.
> 
> The only change since last time is that the series has been made
> compatible with Peff's "when remote, disable some features"
> changes.
> 
> Erik Faye-Lund (4):
>   compat/win32/sys/poll.c: upgrade from upstream
>   mingw: fix compilation of poll-emulation
>   enter_repo: do not modify input
>   upload-archive: use start_command instead of fork

Thanks. I can't comment on the earlier patches, but 4/4 looks sane to
me.

-Peff

Re: [PATCH v3 3/4] enter_repo: do not modify input

[René Scharfe]

Am 29.09.2011 22:59, schrieb Erik Faye-Lund:
> diff --git a/path.c b/path.c
> index 6f3f5d5..f7dfd0b 100644
> --- a/path.c
> +++ b/path.c
> @@ -283,7 +283,7 @@ return_null:
>   * links.  User relative paths are also returned as they are given,
>   * except DWIM suffixing.
>   */
> -char *enter_repo(char *path, int strict)
> +const char *enter_repo(const char *path, int strict)
>  {
>  	static char used_path[PATH_MAX];
>  	static char validated_path[PATH_MAX];
> @@ -297,14 +297,15 @@ char *enter_repo(char *path, int strict)
>  		};
>  		int len = strlen(path);
>  		int i;
> -		while ((1 < len) && (path[len-1] == '/')) {
> -			path[len-1] = 0;
> +		while ((1 < len) && (path[len-1] == '/'))
>  			len--;
> -		}
> +
>  		if (PATH_MAX <= len)
>  			return NULL;
> -		if (path[0] == '~') {
> -			char *newpath = expand_user_path(path);
> +		strncpy(used_path, path, len);
> +
> +		if (used_path[0] == '~') {
> +			char *newpath = expand_user_path(used_path);
>  			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
>  				free(newpath);
>  				return NULL;
> @@ -316,24 +317,21 @@ char *enter_repo(char *path, int strict)
>  			 * anyway.
>  			 */
>  			strcpy(used_path, newpath); free(newpath);
> -			strcpy(validated_path, path);
> -			path = used_path;
> +			strcpy(validated_path, used_path);
>  		}
>  		else if (PATH_MAX - 10 < len)
>  			return NULL;
> -		else {
> -			path = strcpy(used_path, path);
> -			strcpy(validated_path, path);
> -		}
> -		len = strlen(path);
> +		else
> +			strcpy(validated_path, used_path);
> +		len = strlen(used_path);
>  		for (i = 0; suffix[i]; i++) {
> -			strcpy(path + len, suffix[i]);
> -			if (!access(path, F_OK)) {
> +			strcpy(used_path + len, suffix[i]);
> +			if (!access(used_path, F_OK)) {
>  				strcat(validated_path, suffix[i]);
>  				break;
>  			}
>  		}
> -		if (!suffix[i] || chdir(path))
> +		if (!suffix[i] || chdir(used_path))
>  			return NULL;
>  		path = validated_path;
>  	}

The use of strcpy and strncpy makes me nervous, but I can't spot a bug
currently and strcpy and even strcat calls had been already in there
before your patch.

René

Re: [PATCH v3 1/4] compat/win32/sys/poll.c: upgrade from upstream

[René Scharfe]

Am 29.09.2011 22:59, schrieb Erik Faye-Lund:
> poll.c is updated from revision adc3a5b in
> git://git.savannah.gnu.org/gnulib.git
> 
> The changes are applied with --whitespace=fix to reduce noise.
> 
> poll.h is not upgraded, because the most recent version now
> contains template-stuff that breaks compilation for us.

> @@ -27,7 +27,10 @@
>  #include <malloc.h>
>  
>  #include <sys/types.h>
> -#include "poll.h"
> +
> +/* Specification.  */
> +#include <poll.h>
> +

In order to make bisecting easier, I think it's a good idea to squash in
the next patch that undoes this part, i.e. simply skip this hunk and add
a third non-verbatim note to the commit message.

René

Re: [PATCH v3 4/4] upload-archive: use start_command instead of fork

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> Add an undocumented flag to git-archive that tells it that the
> action originated from a remote, so features can be disabled.

> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 2d0b383..c57e8bd 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -6,6 +6,7 @@
>  #include "archive.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "run-command.h"
>  
>  static const char upload_archive_usage[] =
>  	"git upload-archive <repo>";
> @@ -18,28 +19,17 @@ static const char lostchild[] =
>  
>  #define MAX_ARGS (64)
>  
> -static int run_upload_archive(int argc, const char **argv, const char *prefix)
> +static void prepare_argv(const char **sent_argv, const char **argv)
>  {
> -	const char *sent_argv[MAX_ARGS];
>  	const char *arg_cmd = "argument ";
>  	char *p, buf[4096];
>  	int sent_argc;
>  	int len;
>  
> +	sent_argc = 2;
> +	sent_argv[0] = "archive";
> +	sent_argv[1] = "--remote-request";
>  	for (p = buf;;) {
>  		/* This will die if not enough free space in buf */
>  		len = packet_read_line(0, p, (buf + sizeof buf) - p);

Hmm, forgetting the "Windows" for a while, does this client work against
the remote repositories that are running deployed versions of Git?

Re: [PATCH v3 4/4] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

On Mon, Oct 3, 2011 at 8:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> Add an undocumented flag to git-archive that tells it that the
>> action originated from a remote, so features can be disabled.
>
>> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
>> index 2d0b383..c57e8bd 100644
>> --- a/builtin/upload-archive.c
>> +++ b/builtin/upload-archive.c
>> @@ -6,6 +6,7 @@
>>  #include "archive.h"
>>  #include "pkt-line.h"
>>  #include "sideband.h"
>> +#include "run-command.h"
>>
>>  static const char upload_archive_usage[] =
>>       "git upload-archive <repo>";
>> @@ -18,28 +19,17 @@ static const char lostchild[] =
>>
>>  #define MAX_ARGS (64)
>>
>> -static int run_upload_archive(int argc, const char **argv, const char *prefix)
>> +static void prepare_argv(const char **sent_argv, const char **argv)
>>  {
>> -     const char *sent_argv[MAX_ARGS];
>>       const char *arg_cmd = "argument ";
>>       char *p, buf[4096];
>>       int sent_argc;
>>       int len;
>>
>> +     sent_argc = 2;
>> +     sent_argv[0] = "archive";
>> +     sent_argv[1] = "--remote-request";
>>       for (p = buf;;) {
>>               /* This will die if not enough free space in buf */
>>               len = packet_read_line(0, p, (buf + sizeof buf) - p);
>
> Hmm, forgetting the "Windows" for a while, does this client work against
> the remote repositories that are running deployed versions of Git?
>

It should, the change is completely server-side. The first two entries
in sent_argv aren't sent over the network protocol, but the ones that
follow them are.

Or did I misunderstand your concern?

Re: [PATCH v3 4/4] upload-archive: use start_command instead of fork

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> It should, the change is completely server-side. The first two entries
> in sent_argv aren't sent over the network protocol, but the ones that
> follow them are.
>
> Or did I misunderstand your concern?

No you didn't. Thanks.

Re: [PATCH v3 3/4] enter_repo: do not modify input

[Phil Hord]

On Thu, Sep 29, 2011 at 4:59 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> entr_repo(..., 0) currently modifies the input to strip away
> trailing slashes. This means that we some times need to copy the
> input to keep the original.

I'm also modifying enter_repo() so I'm looking a bit closer at this patch now.

> Change it to unconditionally copy it into the used_path buffer so
> we can safely use the input without having to copy it.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
[...]
>  */
> -char *enter_repo(char *path, int strict)
> +const char *enter_repo(const char *path, int strict)
>  {
>        static char used_path[PATH_MAX];
>        static char validated_path[PATH_MAX];
> @@ -297,14 +297,15 @@ char *enter_repo(char *path, int strict)
>                };
>                int len = strlen(path);
>                int i;
> -               while ((1 < len) && (path[len-1] == '/')) {
> -                       path[len-1] = 0;
> +               while ((1 < len) && (path[len-1] == '/'))
>                        len--;
> -               }
> +
>                if (PATH_MAX <= len)
>                        return NULL;
> -               if (path[0] == '~') {
> -                       char *newpath = expand_user_path(path);
> +               strncpy(used_path, path, len);

When len < strlen(path), this will will leave used_path unterminated.

> +
> +               if (used_path[0] == '~') {
> +                       char *newpath = expand_user_path(used_path);
>                        if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
>                                free(newpath);
>                                return NULL;
> @@ -316,24 +317,21 @@ char *enter_repo(char *path, int strict)
>                         * anyway.
>                         */
>                        strcpy(used_path, newpath); free(newpath);
> -                       strcpy(validated_path, path);
> -                       path = used_path;
> +                       strcpy(validated_path, used_path);

The point of 'validated_path' is to keep the original unmolested,
unexpanded path string (plus DWIM suffix), but here you've just
replaced validated_path with a copy of the expanded_user_path.  On the
other hand, we seem always to strcpy(validated_path , path), so we
might as well get that done up-front.

I'll re-roll these changes in in a minute.  Stand by.

Phil

Re: [PATCH v3 3/4] enter_repo: do not modify input

[Phil Hord]

From: Erik Faye-Lund <kusmabite@gmail.com>

entr_repo(..., 0) currently modifies the input to strip away
trailing slashes. This means that we some times need to copy the
input to keep the original.

Change it to unconditionally copy it into the used_path buffer so
we can safely use the input without having to copy it. Also store
a working copy in validated_path up-front before we start
resolving anything.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Phil Hord <hordp@cisco.com>
---
 cache.h  |    2 +-
 daemon.c |    4 ++--
 path.c   |   28 ++++++++++++----------------
 3 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/cache.h b/cache.h
index 607c2ea..a7e4de1 100644
--- a/cache.h
+++ b/cache.h
@@ -734,7 +734,7 @@ int safe_create_leading_directories(char *path);
 int safe_create_leading_directories_const(const char *path);
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
-char *enter_repo(char *path, int strict);
+const char *enter_repo(const char *path, int strict);
 static inline int is_absolute_path(const char *path)
 {
 	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
diff --git a/daemon.c b/daemon.c
index 4c8346d..9253192 100644
--- a/daemon.c
+++ b/daemon.c
@@ -108,11 +108,11 @@ static void NORETURN daemon_die(const char *err,
va_list params)
 	exit(1);
 }

-static char *path_ok(char *directory)
+static const char *path_ok(char *directory)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
-	char *path;
+	const char *path;
 	char *dir;

 	dir = directory;
diff --git a/path.c b/path.c
index 6f3f5d5..01028f2 100644
--- a/path.c
+++ b/path.c
@@ -283,7 +283,7 @@ return_null:
  * links.  User relative paths are also returned as they are given,
  * except DWIM suffixing.
  */
-char *enter_repo(char *path, int strict)
+const char *enter_repo(const char *path, int strict)
 {
 	static char used_path[PATH_MAX];
 	static char validated_path[PATH_MAX];
@@ -297,14 +297,16 @@ char *enter_repo(char *path, int strict)
 		};
 		int len = strlen(path);
 		int i;
-		while ((1 < len) && (path[len-1] == '/')) {
-			path[len-1] = 0;
+		while ((1 < len) && (path[len-1] == '/'))
 			len--;
-		}
+
 		if (PATH_MAX <= len)
 			return NULL;
-		if (path[0] == '~') {
-			char *newpath = expand_user_path(path);
+		strncpy(used_path, path, len); used_path[len] = 0 ;
+		strcpy(validated_path, used_path);
+
+		if (used_path[0] == '~') {
+			char *newpath = expand_user_path(used_path);
 			if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
 				free(newpath);
 				return NULL;
@@ -316,24 +318,18 @@ char *enter_repo(char *path, int strict)
 			 * anyway.
 			 */
 			strcpy(used_path, newpath); free(newpath);
-			strcpy(validated_path, path);
-			path = used_path;
 		}
 		else if (PATH_MAX - 10 < len)
 			return NULL;
-		else {
-			path = strcpy(used_path, path);
-			strcpy(validated_path, path);
-		}
-		len = strlen(path);
+		len = strlen(used_path);
 		for (i = 0; suffix[i]; i++) {
-			strcpy(path + len, suffix[i]);
-			if (!access(path, F_OK)) {
+			strcpy(used_path + len, suffix[i]);
+			if (!access(used_path, F_OK)) {
 				strcat(validated_path, suffix[i]);
 				break;
 			}
 		}
-		if (!suffix[i] || chdir(path))
+		if (!suffix[i] || chdir(used_path))
 			return NULL;
 		path = validated_path;
 	}
-- 
1.7.7.503.g26392.dirty

Re: [PATCH v3 3/4] enter_repo: do not modify input

[Erik Faye-Lund]

On Tue, Oct 4, 2011 at 7:55 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Thu, Sep 29, 2011 at 4:59 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> entr_repo(..., 0) currently modifies the input to strip away
>> trailing slashes. This means that we some times need to copy the
>> input to keep the original.
>
> I'm also modifying enter_repo() so I'm looking a bit closer at this patch now.
>
>> Change it to unconditionally copy it into the used_path buffer so
>> we can safely use the input without having to copy it.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
> [...]
>>  */
>> -char *enter_repo(char *path, int strict)
>> +const char *enter_repo(const char *path, int strict)
>>  {
>>        static char used_path[PATH_MAX];
>>        static char validated_path[PATH_MAX];
>> @@ -297,14 +297,15 @@ char *enter_repo(char *path, int strict)
>>                };
>>                int len = strlen(path);
>>                int i;
>> -               while ((1 < len) && (path[len-1] == '/')) {
>> -                       path[len-1] = 0;
>> +               while ((1 < len) && (path[len-1] == '/'))
>>                        len--;
>> -               }
>> +
>>                if (PATH_MAX <= len)
>>                        return NULL;
>> -               if (path[0] == '~') {
>> -                       char *newpath = expand_user_path(path);
>> +               strncpy(used_path, path, len);
>
> When len < strlen(path), this will will leave used_path unterminated.
>

Good catch, thanks!

>> +
>> +               if (used_path[0] == '~') {
>> +                       char *newpath = expand_user_path(used_path);
>>                        if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
>>                                free(newpath);
>>                                return NULL;
>> @@ -316,24 +317,21 @@ char *enter_repo(char *path, int strict)
>>                         * anyway.
>>                         */
>>                        strcpy(used_path, newpath); free(newpath);
>> -                       strcpy(validated_path, path);
>> -                       path = used_path;
>> +                       strcpy(validated_path, used_path);
>
> The point of 'validated_path' is to keep the original unmolested,
> unexpanded path string (plus DWIM suffix), but here you've just
> replaced validated_path with a copy of the expanded_user_path.  On the
> other hand, we seem always to strcpy(validated_path , path), so we
> might as well get that done up-front.

Yeah, that's probably better.