{"thread":{"id":"27847","subject":"[PATCH v2 2/4] mingw: fix compilation of poll-emulation","startedAt":"2011-07-18T18:08:32Z","lastAt":"2011-09-29T20:20:29Z","messageCount":33,"participants":["Erik Faye-Lund","Junio C Hamano","Jeff King","René Scharfe","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"299185","messageId":"1311012516-4836-1-git-send-email-kusmabite@gmail.com","threadId":"27847","inReplyTo":null,"subject":"[PATCH v2 0/4] port upload-archive to Windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-18T18:08:32Z","receivedAt":"2011-07-18T18:08:32Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Here's the updated version of this series. Changes since last time:\n - Patch 3/4 is new, makes enter_repo take const-input\n - Patch 4/4 (formerly 3/3): added a call to enter_repo to make sure\n the path-argument is taken into account.\n\nErik Faye-Lund (4):\n compat/win32/sys/poll.c: upgrade from upstream\n mingw: fix compilation of poll-emulation\n enter_repo: do not modify input\n upload-archive: use start_command instead of fork\n\n builtin/upload-archive.c | 65 +++++++++++++--------------------------------\n cache.h | 2 +-\n compat/mingw.h | 2 -\n compat/win32/sys/poll.c | 17 ++++++++---\n daemon.c | 4 +-\n path.c | 30 ++++++++++-----------\n 6 files changed, 48 insertions(+), 72 deletions(-)\n\n-- \n1.7.6.135.g378e9\n\n"},{"id":"171594","messageId":"1311012516-4836-2-git-send-email-kusmabite@gmail.com","threadId":"27847","inReplyTo":"1311012516-4836-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 1/4] compat/win32/sys/poll.c: upgrade from upstream","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-18T18:08:33Z","receivedAt":"2011-07-18T18:08:33Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"poll.c is updated from revision adc3a5b in\ngit://git.savannah.gnu.org/gnulib.git\n\nThe changes are applied with --whitespace=fix to reduce noise.\n\npoll.h is not upgraded, because the most recent version now\ncontains template-stuff that breaks compilation for us.\n\nSigned-off-by: Erik Faye-Lund \n---\n compat/win32/sys/poll.c | 19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c\nindex 708a6c9..403eaa7 100644\n--- a/compat/win32/sys/poll.c\n+++ b/compat/win32/sys/poll.c\n@@ -1,7 +1,7 @@\n /* Emulation for poll(2)\n Contributed by Paolo Bonzini.\n \n- Copyright 2001-2003, 2006-2010 Free Software Foundation, Inc.\n+ Copyright 2001-2003, 2006-2011 Free Software Foundation, Inc.\n \n This file is part of gnulib.\n \n@@ -27,7 +27,10 @@\n #include \n \n #include \n-#include \"poll.h\"\n+\n+/* Specification. */\n+#include \n+\n #include \n #include \n #include \n@@ -314,10 +317,7 @@ compute_revents (int fd, int sought, fd_set *rfds, fd_set *wfds, fd_set *efds)\n #endif /* !MinGW */\n \n int\n-poll (pfd, nfd, timeout)\n- struct pollfd *pfd;\n- nfds_t nfd;\n- int timeout;\n+poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n {\n #ifndef WIN32_NATIVE\n fd_set rfds, wfds, efds;\n@@ -454,6 +454,7 @@ poll (pfd, nfd, timeout)\n if (!hEvent)\n hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);\n \n+restart:\n handle_array[0] = hEvent;\n nhandles = 1;\n FD_ZERO (&rfds);\n@@ -594,6 +595,12 @@ poll (pfd, nfd, timeout)\n \trc++;\n }\n \n+ if (!rc && timeout == INFTIM)\n+ {\n+ SwitchToThread();\n+ goto restart;\n+ }\n+\n return rc;\n #endif\n }\n-- \n1.7.6.135.g378e9\n"},{"id":"171592","messageId":"1311012516-4836-3-git-send-email-kusmabite@gmail.com","threadId":"27847","inReplyTo":"1311012516-4836-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 2/4] mingw: fix compilation of poll-emulation","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-18T18:08:34Z","receivedAt":"2011-07-18T18:08:34Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"gnulib has changed the inclusion of poll.h from double quotes\nto single-quotes. But because compat/win32/sys/ isn't in our\ninclude-path, this breaks compilation. Change it back the way\nit was.\n\nSigned-off-by: Erik Faye-Lund \n---\n compat/win32/sys/poll.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c\nindex 403eaa7..225ddce 100644\n--- a/compat/win32/sys/poll.c\n+++ b/compat/win32/sys/poll.c\n@@ -29,7 +29,7 @@\n #include \n \n /* Specification. */\n-#include \n+#include \"poll.h\"\n \n #include \n #include \n-- \n1.7.6.135.g378e9\n"},{"id":"171595","messageId":"1311012516-4836-4-git-send-email-kusmabite@gmail.com","threadId":"27847","inReplyTo":"1311012516-4836-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 3/4] enter_repo: do not modify input","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-18T18:08:35Z","receivedAt":"2011-07-18T18:08:35Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"entr_repo(..., 0) currently modifies the input to strip away\ntrailing slashes. This means that we some times need to copy the\ninput to keep the original.\n\nChange it to unconditionally copy it into the used_path buffer so\nwe can safely use the input without having to copy it.\n\nSigned-off-by: Erik Faye-Lund \n---\n cache.h | 2 +-\n daemon.c | 4 ++--\n path.c | 30 ++++++++++++++----------------\n 3 files changed, 17 insertions(+), 19 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex bc9e5eb..02386f1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -716,7 +716,7 @@ int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\n int mkdir_in_gitdir(const char *path);\n extern char *expand_user_path(const char *path);\n-char *enter_repo(char *path, int strict);\n+const char *enter_repo(const char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\n \treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..9253192 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -108,11 +108,11 @@ static void NORETURN daemon_die(const char *err, va_list params)\n \texit(1);\n }\n \n-static char *path_ok(char *directory)\n+static const char *path_ok(char *directory)\n {\n \tstatic char rpath[PATH_MAX];\n \tstatic char interp_path[PATH_MAX];\n-\tchar *path;\n+\tconst char *path;\n \tchar *dir;\n \n \tdir = directory;\ndiff --git a/path.c b/path.c\nindex 4d73cc9..2757d8b 100644\n--- a/path.c\n+++ b/path.c\n@@ -283,7 +283,7 @@ return_null:\n * links. User relative paths are also returned as they are given,\n * except DWIM suffixing.\n */\n-char *enter_repo(char *path, int strict)\n+const char *enter_repo(const char *path, int strict)\n {\n \tstatic char used_path[PATH_MAX];\n \tstatic char validated_path[PATH_MAX];\n@@ -297,14 +297,15 @@ char *enter_repo(char *path, int strict)\n \t\t};\n \t\tint len = strlen(path);\n \t\tint i;\n-\t\twhile ((1 < len) && (path[len-1] == '/')) {\n-\t\t\tpath[len-1] = 0;\n+\t\twhile ((1 < len) && (path[len-1] == '/'))\n \t\t\tlen--;\n-\t\t}\n+\n \t\tif (PATH_MAX <= len)\n \t\t\treturn NULL;\n-\t\tif (path[0] == '~') {\n-\t\t\tchar *newpath = expand_user_path(path);\n+\t\tstrncpy(used_path, path, len);\n+\n+\t\tif (used_path[0] == '~') {\n+\t\t\tchar *newpath = expand_user_path(used_path);\n \t\t\tif (!newpath || (PATH_MAX - 10 < strlen(newpath))) {\n \t\t\t\tfree(newpath);\n \t\t\t\treturn NULL;\n@@ -316,24 +317,21 @@ char *enter_repo(char *path, int strict)\n \t\t\t * anyway.\n \t\t\t */\n \t\t\tstrcpy(used_path, newpath); free(newpath);\n-\t\t\tstrcpy(validated_path, path);\n-\t\t\tpath = used_path;\n+\t\t\tstrcpy(validated_path, used_path);\n \t\t}\n \t\telse if (PATH_MAX - 10 < len)\n \t\t\treturn NULL;\n-\t\telse {\n-\t\t\tpath = strcpy(used_path, path);\n-\t\t\tstrcpy(validated_path, path);\n-\t\t}\n-\t\tlen = strlen(path);\n+\t\telse\n+\t\t\tstrcpy(validated_path, used_path);\n+\t\tlen = strlen(used_path);\n \t\tfor (i = 0; suffix[i]; i++) {\n-\t\t\tstrcpy(path + len, suffix[i]);\n-\t\t\tif (!access(path, F_OK)) {\n+\t\t\tstrcpy(used_path + len, suffix[i]);\n+\t\t\tif (!access(used_path, F_OK)) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (!suffix[i] || chdir(path))\n+\t\tif (!suffix[i] || chdir(used_path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n-- \n1.7.6.135.g378e9\n"},{"id":"171593","messageId":"1311012516-4836-5-git-send-email-kusmabite@gmail.com","threadId":"27847","inReplyTo":"1311012516-4836-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-18T18:08:36Z","receivedAt":"2011-07-18T18:08:36Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"The POSIX-function fork is not supported on Windows. Use our\nstart_command API instead.\n\nAs this is the last call-site that depends on the fork-stub in\ncompat/mingw.h, remove that as well.\n\nSigned-off-by: Erik Faye-Lund \n---\n builtin/upload-archive.c | 65 +++++++++++++--------------------------------\n compat/mingw.h | 2 -\n 2 files changed, 19 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 73f788e..0c192b5 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -6,6 +6,7 @@\n #include \"archive.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n static const char upload_archive_usage[] =\n \t\"git upload-archive \";\n@@ -18,28 +19,16 @@ static const char lostchild[] =\n \n #define MAX_ARGS (64)\n \n-static int run_upload_archive(int argc, const char **argv, const char *prefix)\n+static void prepare_argv(const char **sent_argv, const char **argv)\n {\n-\tconst char *sent_argv[MAX_ARGS];\n \tconst char *arg_cmd = \"argument \";\n \tchar *p, buf[4096];\n \tint sent_argc;\n \tint len;\n \n-\tif (argc != 2)\n-\t\tusage(upload_archive_usage);\n-\n-\tif (strlen(argv[1]) + 1 > sizeof(buf))\n-\t\tdie(\"insanely long repository name\");\n-\n-\tstrcpy(buf, argv[1]); /* enter-repo smudges its argument */\n-\n-\tif (!enter_repo(buf, 0))\n-\t\tdie(\"'%s' does not appear to be a git repository\", buf);\n-\n \t/* put received options in sent_argv[] */\n \tsent_argc = 1;\n-\tsent_argv[0] = \"git-upload-archive\";\n+\tsent_argv[0] = \"archive\";\n \tfor (p = buf;;) {\n \t\t/* This will die if not enough free space in buf */\n \t\tlen = packet_read_line(0, p, (buf + sizeof buf) - p);\n@@ -62,9 +51,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)\n \t\t*p++ = 0;\n \t}\n \tsent_argv[sent_argc] = NULL;\n-\n-\t/* parse all options sent by the client */\n-\treturn write_archive(sent_argc, sent_argv, prefix, 0);\n }\n \n __attribute__((format (printf, 1, 2)))\n@@ -96,38 +82,25 @@ static ssize_t process_input(int child_fd, int band)\n \n int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n {\n-\tpid_t writer;\n-\tint fd1[2], fd2[2];\n-\t/*\n-\t * Set up sideband subprocess.\n-\t *\n-\t * We (parent) monitor and read from child, sending its fd#1 and fd#2\n-\t * multiplexed out to our fd#1. If the child dies, we tell the other\n-\t * end over channel #3.\n-\t */\n-\tif (pipe(fd1) < 0 || pipe(fd2) < 0) {\n-\t\tint err = errno;\n-\t\tpacket_write(1, \"NACK pipe failed on the remote side\\n\");\n-\t\tdie(\"upload-archive: %s\", strerror(err));\n-\t}\n-\twriter = fork();\n-\tif (writer < 0) {\n+\tconst char *sent_argv[MAX_ARGS];\n+\tstruct child_process cld = { sent_argv };\n+\tcld.out = cld.err = -1;\n+\tcld.git_cmd = 1;\n+\n+\tif (argc != 2)\n+\t\tusage(upload_archive_usage);\n+\n+\tif (!enter_repo(argv[1], 0))\n+\t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n+\n+\tprepare_argv(sent_argv, argv);\n+\tif (start_command(&cld)) {\n \t\tint err = errno;\n \t\tpacket_write(1, \"NACK fork failed on the remote side\\n\");\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n-\tif (!writer) {\n-\t\t/* child - connect fd#1 and fd#2 to the pipe */\n-\t\tdup2(fd1[1], 1);\n-\t\tdup2(fd2[1], 2);\n-\t\tclose(fd1[1]); close(fd2[1]);\n-\t\tclose(fd1[0]); close(fd2[0]); /* we do not read from pipe */\n-\n-\t\texit(run_upload_archive(argc, argv, prefix));\n-\t}\n \n \t/* parent - read from child, multiplex and send out to fd#1 */\n-\tclose(fd1[1]); close(fd2[1]); /* we do not write to pipe */\n \tpacket_write(1, \"ACK\\n\");\n \tpacket_flush(1);\n \n@@ -135,9 +108,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \t\tstruct pollfd pfd[2];\n \t\tint status;\n \n-\t\tpfd[0].fd = fd1[0];\n+\t\tpfd[0].fd = cld.out;\n \t\tpfd[0].events = POLLIN;\n-\t\tpfd[1].fd = fd2[0];\n+\t\tpfd[1].fd = cld.err;\n \t\tpfd[1].events = POLLIN;\n \t\tif (poll(pfd, 2, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n@@ -156,7 +129,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \t\t\tif (process_input(pfd[0].fd, 1))\n \t\t\t\tcontinue;\n \n-\t\tif (waitpid(writer, &status, 0) < 0)\n+\t\tif (waitpid(cld.pid, &status, 0) < 0)\n \t\t\terror_clnt(\"%s\", lostchild);\n \t\telse if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n \t\t\terror_clnt(\"%s\", deadchild);\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex ce9dd98..9cb6eb9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -85,8 +85,6 @@ static inline int symlink(const char *oldpath, const char *newpath)\n { errno = ENOSYS; return -1; }\n static inline int fchmod(int fildes, mode_t mode)\n { errno = ENOSYS; return -1; }\n-static inline pid_t fork(void)\n-{ errno = ENOSYS; return -1; }\n static inline unsigned int alarm(unsigned int seconds)\n { return 0; }\n static inline int fsync(int fd)\n-- \n1.7.6.135.g378e9\n"},{"id":"171704","messageId":"7vmxga7zb9.fsf@alter.siamese.dyndns.org","threadId":"27847","inReplyTo":"1311012516-4836-5-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-07-19T21:09:14Z","receivedAt":"2011-07-19T21:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> @@ -18,28 +19,16 @@ static const char lostchild[] =\n> \n> #define MAX_ARGS (64)\n> \n> -static int run_upload_archive(int argc, const char **argv, const char *prefix)\n> +static void prepare_argv(const char **sent_argv, const char **argv)\n> {\n> -\tconst char *sent_argv[MAX_ARGS];\n> \tconst char *arg_cmd = \"argument \";\n> \tchar *p, buf[4096];\n> \tint sent_argc;\n> \tint len;\n> \n> -\tif (argc != 2)\n> -\t\tusage(upload_archive_usage);\n> -\n> -\tif (strlen(argv[1]) + 1 > sizeof(buf))\n> -\t\tdie(\"insanely long repository name\");\n> -\n> -\tstrcpy(buf, argv[1]); /* enter-repo smudges its argument */\n> -\n> -\tif (!enter_repo(buf, 0))\n> -\t\tdie(\"'%s' does not appear to be a git repository\", buf);\n> -\n> \t/* put received options in sent_argv[] */\n> \tsent_argc = 1;\n> -\tsent_argv[0] = \"git-upload-archive\";\n> +\tsent_argv[0] = \"archive\";\n> \tfor (p = buf;;) {\n> \t\t/* This will die if not enough free space in buf */\n> \t\tlen = packet_read_line(0, p, (buf + sizeof buf) - p);\n> @@ -62,9 +51,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)\n> \t\t*p++ = 0;\n> \t}\n> \tsent_argv[sent_argc] = NULL;\n> -\n> -\t/* parse all options sent by the client */\n> -\treturn write_archive(sent_argc, sent_argv, prefix, 0);\n> }\n\nHmm, how well would this change work with recent archive-filter work by\nPeff, especially with the \"when remote, disable some features\" bits?\n"},{"id":"172242","messageId":"CABPQNSYtpRpNQVW=QvyfFhmWge5-C+k85bs2EJeK8k0jdfHzrw@mail.gmail.com","threadId":"27847","inReplyTo":"7vmxga7zb9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-28T08:32:54Z","receivedAt":"2011-07-28T08:32:54Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Sorry for the late reply. Things have been a bit hectic here in Oslo\nover the last week.\n\nOn Tue, Jul 19, 2011 at 11:09 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> @@ -18,28 +19,16 @@ static const char lostchild[] =\n>>\n>>  #define MAX_ARGS (64)\n>>\n>> -static int run_upload_archive(int argc, const char **argv, const char *prefix)\n>> +static void prepare_argv(const char **sent_argv, const char **argv)\n>>  {\n>> -     const char *sent_argv[MAX_ARGS];\n>>       const char *arg_cmd = \"argument \";\n>>       char *p, buf[4096];\n>>       int sent_argc;\n>>       int len;\n>>\n>> -     if (argc != 2)\n>> -             usage(upload_archive_usage);\n>> -\n>> -     if (strlen(argv[1]) + 1 > sizeof(buf))\n>> -             die(\"insanely long repository name\");\n>> -\n>> -     strcpy(buf, argv[1]); /* enter-repo smudges its argument */\n>> -\n>> -     if (!enter_repo(buf, 0))\n>> -             die(\"'%s' does not appear to be a git repository\", buf);\n>> -\n>>       /* put received options in sent_argv[] */\n>>       sent_argc = 1;\n>> -     sent_argv[0] = \"git-upload-archive\";\n>> +     sent_argv[0] = \"archive\";\n>>       for (p = buf;;) {\n>>               /* This will die if not enough free space in buf */\n>>               len = packet_read_line(0, p, (buf + sizeof buf) - p);\n>> @@ -62,9 +51,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)\n>>               *p++ = 0;\n>>       }\n>>       sent_argv[sent_argc] = NULL;\n>> -\n>> -     /* parse all options sent by the client */\n>> -     return write_archive(sent_argc, sent_argv, prefix, 0);\n>>  }\n>\n> Hmm, how well would this change work with recent archive-filter work by\n> Peff, especially with the \"when remote, disable some features\" bits?\n>\n\nThanks for the notice. And no, not at all :(\n\nOK then. I'll probably have to rewrite this to use start_async +\ndup-bonanza instead of start_command...\n"},{"id":"172267","messageId":"20110728160845.GA14337@sigill.intra.peff.net","threadId":"27847","inReplyTo":"CABPQNSYtpRpNQVW=QvyfFhmWge5-C+k85bs2EJeK8k0jdfHzrw@mail.gmail.com","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-28T16:08:45Z","receivedAt":"2011-07-28T16:08:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2011 at 10:32:54AM +0200, Erik Faye-Lund wrote:\n\n> > Hmm, how well would this change work with recent archive-filter work by\n> > Peff, especially with the \"when remote, disable some features\" bits?\n> >\n> \n> Thanks for the notice. And no, not at all :(\n> \n> OK then. I'll probably have to rewrite this to use start_async +\n> dup-bonanza instead of start_command...\n\nWhat's the issue? In my final version, I didn't think I changed\nrun_upload_archive much at all. I would think there would be a trivial\ntext conflict at the end of run_upload_archive, but the semantics should\notherwise be the same.\n\nLet me know if you need help untangling it.\n\n-Peff\n"},{"id":"172272","messageId":"20110728164758.GA15931@sigill.intra.peff.net","threadId":"27847","inReplyTo":"20110728160845.GA14337@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-28T16:47:58Z","receivedAt":"2011-07-28T16:47:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2011 at 10:08:45AM -0600, Jeff King wrote:\n\n> > Thanks for the notice. And no, not at all :(\n> > \n> > OK then. I'll probably have to rewrite this to use start_async +\n> > dup-bonanza instead of start_command...\n> \n> What's the issue? In my final version, I didn't think I changed\n> run_upload_archive much at all. I would think there would be a trivial\n> text conflict at the end of run_upload_archive, but the semantics should\n> otherwise be the same.\n\nAh, nevermind. I see now; the command-line option interface to \"git\narchive\" is not as rich as what you can pass via write_archive.\n\nI think you can get around it by adding an option to \"git archive\" to\nindicate that we are filling a remote request, which is the only\nextra information that my series adds.\n\nI do think the start_async approach is a little more natural, though. If\nthe dup() bits get too ugly, it would not be that hard to make the\narchive code write to a specific descriptor.\n\n-Peff\n"},{"id":"172291","messageId":"20110728170222.GB15931@sigill.intra.peff.net","threadId":"27847","inReplyTo":"20110728164758.GA15931@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-28T17:02:27Z","receivedAt":"2011-07-28T17:02:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 28, 2011 at 10:47:58AM -0600, Jeff King wrote:\n\n> Ah, nevermind. I see now; the command-line option interface to \"git\n> archive\" is not as rich as what you can pass via write_archive.\n> \n> I think you can get around it by adding an option to \"git archive\" to\n> indicate that we are filling a remote request, which is the only\n> extra information that my series adds.\n\nAnd that patch would look something like the one below. You can also now\ndrop the \"remote\" parameter to write_archive entirely, but I didn't do\nso here. Applied on top of the merge of your series into next, this\npasses t5000.\n\n-Peff\n\n---\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 883c009..6668340 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -85,6 +85,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \tconst char *exec = \"git-upload-archive\";\n \tconst char *output = NULL;\n \tconst char *remote = NULL;\n+\tint is_remote = 0;\n \tstruct option local_opts[] = {\n \t\tOPT_STRING('o', \"output\", &output, \"file\",\n \t\t\t\"write the archive to this file\"),\n@@ -92,6 +93,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \t\t\t\"retrieve the archive from remote repository \"),\n \t\tOPT_STRING(0, \"exec\", &exec, \"cmd\",\n \t\t\t\"path to the remote git-upload-archive command\"),\n+\t\t{ OPTION_BOOLEAN, 0, \"remote-request\", &is_remote, NULL,\n+\t\t\t\"indicate we are serving a remote request\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_HIDDEN },\n \t\tOPT_END()\n \t};\n \n@@ -106,5 +110,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n \n \tsetvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n \n-\treturn write_archive(argc, argv, prefix, 1, output, 0);\n+\treturn write_archive(argc, argv, prefix, 1, output, is_remote);\n }\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 0c192b5..c57e8bd 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -27,8 +27,9 @@ static void prepare_argv(const char **sent_argv, const char **argv)\n \tint len;\n \n \t/* put received options in sent_argv[] */\n-\tsent_argc = 1;\n+\tsent_argc = 2;\n \tsent_argv[0] = \"archive\";\n+\tsent_argv[1] = \"--remote-request\";\n \tfor (p = buf;;) {\n \t\t/* This will die if not enough free space in buf */\n \t\tlen = packet_read_line(0, p, (buf + sizeof buf) - p);\n"},{"id":"172534","messageId":"CABPQNSaqyD+rhWPRbtVdnkweuXSycBahKEsasGZkEg3mi4SaxQ@mail.gmail.com","threadId":"27847","inReplyTo":"20110728170222.GB15931@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-08-01T14:45:03Z","receivedAt":"2011-08-01T14:45:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jul 28, 2011 at 7:02 PM, Jeff King wrote:\n> On Thu, Jul 28, 2011 at 10:47:58AM -0600, Jeff King wrote:\n>\n>> Ah, nevermind. I see now; the command-line option interface to \"git\n>> archive\" is not as rich as what you can pass via write_archive.\n>>\n>> I think you can get around it by adding an option to \"git archive\" to\n>> indicate that we are filling a remote request, which is the only\n>> extra information that my series adds.\n>\n> And that patch would look something like the one below.\n\nThis is the most straight forward way of doing this, thanks. I'll post\na new version with this squashed in soon.\n\nWhat's the proper way of attributing you for the work?\n\n> You can also now\n> drop the \"remote\" parameter to write_archive entirely, but I didn't do\n> so here.\n\nI'm not entirely sure how you propose we do this... do you mean by\nhoisting the relevant logic from write_archive up to cmd_archive or\nsomething?\n\n> Applied on top of the merge of your series into next, this\n> passes t5000.\n\nUnfortunately, it doesn't pass on Windows, but this doesn't seem to be\ndirectly related to the remote-stuff: Here's the tests that fail:\n\n\n$ make t5000-tar-tree\n*** t5000-tar-tree.sh ***\nok 1 - populate workdir\n\nok 53 - infer tgz from .tar.gz filename\nnot ok - 54 extract tgz file\n#\n# $GUNZIP -c j.tar &&\n# test_cmp b.tar j.tar\n#\nnot ok - 55 remote tar.gz is allowed by default\n#\n# git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n# test_cmp j.tgz remote.tar.gz\n#\nok 56 - remote tar.gz can be disabled\n# failed 2 among 56 test(s)\n1..56\nmake: *** [t5000-tar-tree.sh] Error 1\n\nIt seems \"git archive --format=tgz HEAD\" is broken on Windows:\n$ git archive --format=tgz HEAD > j.tgz\n$ gunzip -c j.tgz > j.tar && echo ok\n\ngzip: j.tgz: invalid compressed data--format violated\n\nBut if I perform the compression manually, the archive is fine:\n$ git archive --format=tar HEAD | gzip -cn > j.tgz\n$ gunzip -c j.tgz > j.tar && echo ok\nok\n\nSo, the big question is, are all tar-filters broken on Windows? It\nseems not; the tests for the foo-filter works fine... any clues? Could\nit somehow be newline-related, perhaps?\n\n>\n> -Peff\n>\n> ---\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 883c009..6668340 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -85,6 +85,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n>        const char *exec = \"git-upload-archive\";\n>        const char *output = NULL;\n>        const char *remote = NULL;\n> +       int is_remote = 0;\n>        struct option local_opts[] = {\n>                OPT_STRING('o', \"output\", &output, \"file\",\n>                        \"write the archive to this file\"),\n> @@ -92,6 +93,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n>                        \"retrieve the archive from remote repository \"),\n>                OPT_STRING(0, \"exec\", &exec, \"cmd\",\n>                        \"path to the remote git-upload-archive command\"),\n> +               { OPTION_BOOLEAN, 0, \"remote-request\", &is_remote, NULL,\n> +                       \"indicate we are serving a remote request\",\n> +                       PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },\n>                OPT_END()\n>        };\n>\n> @@ -106,5 +110,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)\n>\n>        setvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n>\n> -       return write_archive(argc, argv, prefix, 1, output, 0);\n> +       return write_archive(argc, argv, prefix, 1, output, is_remote);\n>  }\n> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> index 0c192b5..c57e8bd 100644\n> --- a/builtin/upload-archive.c\n> +++ b/builtin/upload-archive.c\n> @@ -27,8 +27,9 @@ static void prepare_argv(const char **sent_argv, const char **argv)\n>        int len;\n>\n>        /* put received options in sent_argv[] */\n> -       sent_argc = 1;\n> +       sent_argc = 2;\n>        sent_argv[0] = \"archive\";\n> +       sent_argv[1] = \"--remote-request\";\n>        for (p = buf;;) {\n>                /* This will die if not enough free space in buf */\n>                len = packet_read_line(0, p, (buf + sizeof buf) - p);\n>\n"},{"id":"172550","messageId":"20110801174603.GB10302@sigill.intra.peff.net","threadId":"27847","inReplyTo":"CABPQNSaqyD+rhWPRbtVdnkweuXSycBahKEsasGZkEg3mi4SaxQ@mail.gmail.com","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-01T17:46:03Z","receivedAt":"2011-08-01T17:46:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2011 at 04:45:03PM +0200, Erik Faye-Lund wrote:\n\n> > And that patch would look something like the one below.\n> \n> This is the most straight forward way of doing this, thanks. I'll post\n> a new version with this squashed in soon.\n> \n> What's the proper way of attributing you for the work?\n\nIf you're squashing, just make a note in the text, or add a Helped-by\nheader at the end.\n\n> > You can also now\n> > drop the \"remote\" parameter to write_archive entirely, but I didn't do\n> > so here.\n> \n> I'm not entirely sure how you propose we do this... do you mean by\n> hoisting the relevant logic from write_archive up to cmd_archive or\n> something?\n\nSorry, I wrote that comment, then totally changed how my patch was\nimplemented and failed to update the comment. :)\n\nThe arguments to git-archive actually get parsed in two places:\n\n 1. cmd_archive passes them to decide on meta stuff like remote vs\n local\n\n 2. write_archive has a parse_archive_args function to actually parse\n the arguments that both upload-archive and archive share\n\nMy patch puts the new argument in (1). We could put it in (2) and do\naway with passing the flag all the way down the call stack. But it seems\nconceptually cleaner to me to have it in (1) (and the diff is much\nshorter, too).\n\n> $ make t5000-tar-tree\n> *** t5000-tar-tree.sh ***\n> ok 1 - populate workdir\n> \n> ok 53 - infer tgz from .tar.gz filename\n> not ok - 54 extract tgz file\n> #\n> # $GUNZIP -c j.tar &&\n> # test_cmp b.tar j.tar\n> #\n> not ok - 55 remote tar.gz is allowed by default\n> #\n> # git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n> # test_cmp j.tgz remote.tar.gz\n> #\n> ok 56 - remote tar.gz can be disabled\n> # failed 2 among 56 test(s)\n> 1..56\n> make: *** [t5000-tar-tree.sh] Error 1\n> \n> It seems \"git archive --format=tgz HEAD\" is broken on Windows:\n> $ git archive --format=tgz HEAD > j.tgz\n> $ gunzip -c j.tgz > j.tar && echo ok\n> \n> gzip: j.tgz: invalid compressed data--format violated\n> \n> But if I perform the compression manually, the archive is fine:\n> $ git archive --format=tar HEAD | gzip -cn > j.tgz\n> $ gunzip -c j.tgz > j.tar && echo ok\n> ok\n\nWeird. What does j.tgz end up looking like? Is it empty, or does it have\nbogus data in it? Does gzip actually get invoked at all? Try running\nwith GIT_TRACE=1. I don't suppose you guys have something like strace,\nwhich might be helpful.\n\n> So, the big question is, are all tar-filters broken on Windows? It\n> seems not; the tests for the foo-filter works fine... any clues? Could\n> it somehow be newline-related, perhaps?\n\nI'm not sure that newlines would make a difference. We use straight\nwrite() everywhere in the archive code, which should be binary safe.\n\n-Peff\n"},{"id":"172557","messageId":"CABPQNSbyp2hEgXvzo3bMHHxrbc9fQLqaABweqfjP7xQzBUdj6A@mail.gmail.com","threadId":"27847","inReplyTo":"20110801174603.GB10302@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-08-01T18:02:22Z","receivedAt":"2011-08-01T18:02:22Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Aug 1, 2011 at 7:46 PM, Jeff King wrote:\n> On Mon, Aug 01, 2011 at 04:45:03PM +0200, Erik Faye-Lund wrote:\n>\n>> > And that patch would look something like the one below.\n>>\n>> This is the most straight forward way of doing this, thanks. I'll post\n>> a new version with this squashed in soon.\n>>\n>> What's the proper way of attributing you for the work?\n>\n> If you're squashing, just make a note in the text, or add a Helped-by\n> header at the end.\n>\n\nOK, that's basically what I did already ;)\n\n>> > You can also now\n>> > drop the \"remote\" parameter to write_archive entirely, but I didn't do\n>> > so here.\n>>\n>> I'm not entirely sure how you propose we do this... do you mean by\n>> hoisting the relevant logic from write_archive up to cmd_archive or\n>> something?\n>\n> Sorry, I wrote that comment, then totally changed how my patch was\n> implemented and failed to update the comment. :)\n>\n> The arguments to git-archive actually get parsed in two places:\n>\n>  1. cmd_archive passes them to decide on meta stuff like remote vs\n>     local\n>\n>  2. write_archive has a parse_archive_args function to actually parse\n>     the arguments that both upload-archive and archive share\n>\n> My patch puts the new argument in (1). We could put it in (2) and do\n> away with passing the flag all the way down the call stack. But it seems\n> conceptually cleaner to me to have it in (1) (and the diff is much\n> shorter, too).\n>\n\nMakes sense. Thanks for the explanation, I agree.\n\n>> $ make t5000-tar-tree\n>> *** t5000-tar-tree.sh ***\n>> ok 1 - populate workdir\n>> \n>> ok 53 - infer tgz from .tar.gz filename\n>> not ok - 54 extract tgz file\n>> #\n>> #               $GUNZIP -c j.tar &&\n>> #               test_cmp b.tar j.tar\n>> #\n>> not ok - 55 remote tar.gz is allowed by default\n>> #\n>> #               git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n>> #               test_cmp j.tgz remote.tar.gz\n>> #\n>> ok 56 - remote tar.gz can be disabled\n>> # failed 2 among 56 test(s)\n>> 1..56\n>> make: *** [t5000-tar-tree.sh] Error 1\n>>\n>> It seems \"git archive --format=tgz HEAD\" is broken on Windows:\n>> $ git archive --format=tgz HEAD > j.tgz\n>> $ gunzip -c j.tgz > j.tar && echo ok\n>>\n>> gzip: j.tgz: invalid compressed data--format violated\n>>\n>> But if I perform the compression manually, the archive is fine:\n>> $ git archive --format=tar HEAD | gzip -cn > j.tgz\n>> $ gunzip -c j.tgz > j.tar && echo ok\n>> ok\n>\n> Weird. What does j.tgz end up looking like? Is it empty, or does it have\n> bogus data in it? Does gzip actually get invoked at all? Try running\n> with GIT_TRACE=1. I don't suppose you guys have something like strace,\n> which might be helpful.\n>\n\nIt does have data, and gzip does get invoked:\n$ gunzip -c j.tgz | wc -c\n\ngzip: j.tgz: invalid compressed data--format violated\n 131072\n\nSo it seems there are around 130k of valid data before it barfs.\n\nHmm, but when I try the same after re-running the test, I get a\ndifferent amount of valid data (491520 bytes this time)... Is this a\ntiming-related issue?\n\nYeah, strace would have been useful, but we don't have that on\nWindows. There are probably other alternatives, but I'm not aware...\nperhaps procmon can be of help?\n\n>> So, the big question is, are all tar-filters broken on Windows? It\n>> seems not; the tests for the foo-filter works fine... any clues? Could\n>> it somehow be newline-related, perhaps?\n>\n> I'm not sure that newlines would make a difference. We use straight\n> write() everywhere in the archive code, which should be binary safe.\n>\n\nOK, so that hunch was a bad one ;)\n"},{"id":"172582","messageId":"20110801182516.GC10302@sigill.intra.peff.net","threadId":"27847","inReplyTo":"CABPQNSbyp2hEgXvzo3bMHHxrbc9fQLqaABweqfjP7xQzBUdj6A@mail.gmail.com","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-01T18:25:16Z","receivedAt":"2011-08-01T18:25:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2011 at 08:02:22PM +0200, Erik Faye-Lund wrote:\n\n> > Weird. What does j.tgz end up looking like? Is it empty, or does it have\n> > bogus data in it? Does gzip actually get invoked at all? Try running\n> > with GIT_TRACE=1. I don't suppose you guys have something like strace,\n> > which might be helpful.\n> >\n> \n> It does have data, and gzip does get invoked:\n> $ gunzip -c j.tgz | wc -c\n> \n> gzip: j.tgz: invalid compressed data--format violated\n> 131072\n> \n> So it seems there are around 130k of valid data before it barfs.\n> \n> Hmm, but when I try the same after re-running the test, I get a\n> different amount of valid data (491520 bytes this time)... Is this a\n> timing-related issue?\n\nHmm. Non-deterministic output is not good. Could be timing, or we could\nbe touching memory we're not supposed to. I just ran t5000 through\nvalgrind, and nothing turned up. And of course I can't replicate the\ntest failure on Linux. So I assume it really is Windows-specific.\n\nThat makes me suspect some difference in how the run-command API works\non the two platforms, since that is the code here that will be vastly\ndifference. I notice the start_command code uses dup() on Windows\ninstead of dup2() (I guess you guys don't have dup2?). I wonder if there\ncould be some issue with another descriptor accidentally pointing to the\nsame spot. But that's just a wild guess.\n\nI think at this point, I'd probably start stepping through the archive\ncode with gdb.\n\n-Peff\n"},{"id":"172598","messageId":"4E371109.7050500@lsrfire.ath.cx","threadId":"27847","inReplyTo":"20110801182516.GC10302@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-01T20:48:09Z","receivedAt":"2011-08-01T20:48:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.08.2011 20:25, schrieb Jeff King:\n> On Mon, Aug 01, 2011 at 08:02:22PM +0200, Erik Faye-Lund wrote:\n> \n>>> Weird. What does j.tgz end up looking like? Is it empty, or does it have\n>>> bogus data in it? Does gzip actually get invoked at all? Try running\n>>> with GIT_TRACE=1. I don't suppose you guys have something like strace,\n>>> which might be helpful.\n>>>\n>>\n>> It does have data, and gzip does get invoked:\n>> $ gunzip -c j.tgz | wc -c\n>>\n>> gzip: j.tgz: invalid compressed data--format violated\n>> 131072\n>>\n>> So it seems there are around 130k of valid data before it barfs.\n>>\n>> Hmm, but when I try the same after re-running the test, I get a\n>> different amount of valid data (491520 bytes this time)... Is this a\n>> timing-related issue?\n> \n> Hmm. Non-deterministic output is not good. Could be timing, or we could\n> be touching memory we're not supposed to. I just ran t5000 through\n> valgrind, and nothing turned up. And of course I can't replicate the\n> test failure on Linux. So I assume it really is Windows-specific.\n> \n> That makes me suspect some difference in how the run-command API works\n> on the two platforms, since that is the code here that will be vastly\n> difference. I notice the start_command code uses dup() on Windows\n> instead of dup2() (I guess you guys don't have dup2?). I wonder if there\n> could be some issue with another descriptor accidentally pointing to the\n> same spot. But that's just a wild guess.\n> \n> I think at this point, I'd probably start stepping through the archive\n> code with gdb.\n\nHrm, here is another data point, for what it's worth (Windows 7 x64,\nMinGW):\n\n\t$ git archive v1.7.6 | gzip -cn | md5sum\n\ta0ca1c873a533a5fcd41d248fb325a5b *-\n\n\t$ git archive --format=tar.gz v1.7.6 | md5sum\n\ta0ca1c873a533a5fcd41d248fb325a5b *-\n\n\t$ git archive v1.7.6 | gzip -cn >a.tgz && md5sum a.tgz && md5sum a.tgz && md5sum a.tgz && md5sum a.tgz && md5sum a.tgz && md5sum a.tar && md5sum So git archive gives the right results when writing to a pipe, but\n> always the same wrong result when writing directly to a file.\n\nThis could indeed be a CRLF issue. archive-tar.c runs gzip to let it\nwrite to the original fd 1 (stdout). gzip is an MSYS program, and MSYS\nis \"clever\" and sets up the channel in text mode (CRLF conversion) if it\nis a regular file, but in binary mode if it is a pipe.\n\nWithout the gzip filter, git-archive writes to stdout itself. Since we\nhave set up all our channels in binary mode, we do not suffer from the\nsame problem for plain tar format.\n\nSo, I don't think we can do a lot about it, short of patching MSYS again...\n\n-- Hannes\n"},{"id":"172606","messageId":"4E371DE3.3000008@lsrfire.ath.cx","threadId":"27847","inReplyTo":"4E3718B4.6090803@kdbg.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-01T21:42:59Z","receivedAt":"2011-08-01T21:42:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.08.2011 23:20, schrieb Johannes Sixt:\n> Am 01.08.2011 22:48, schrieb René Scharfe:\n>> So git archive gives the right results when writing to a pipe, but\n>> always the same wrong result when writing directly to a file.\n> \n> This could indeed be a CRLF issue. archive-tar.c runs gzip to let it\n> write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS\n> is \"clever\" and sets up the channel in text mode (CRLF conversion) if it\n> is a regular file, but in binary mode if it is a pipe.\n> \n> Without the gzip filter, git-archive writes to stdout itself. Since we\n> have set up all our channels in binary mode, we do not suffer from the\n> same problem for plain tar format.\n\nThat seems to be it indeed:\n\n\t$ git config tar.t.command 'tar tf -'\n\t$ git archive --format=t v1.7.6 >a.t\n\t$ git archive --format=t v1.7.6 | cat >b.t\n\n\t$ diff -b a.t b.t | wc -l\n\t 0\n\n\t$ md5sum Am 01.08.2011 22:48, schrieb René Scharfe:\n>> So git archive gives the right results when writing to a pipe, but\n>> always the same wrong result when writing directly to a file.\n> \n> This could indeed be a CRLF issue. archive-tar.c runs gzip to let it\n> write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS\n> is \"clever\" and sets up the channel in text mode (CRLF conversion) if it\n> is a regular file, but in binary mode if it is a pipe.\n> \n> Without the gzip filter, git-archive writes to stdout itself. Since we\n> have set up all our channels in binary mode, we do not suffer from the\n> same problem for plain tar format.\n> \n> So, I don't think we can do a lot about it, short of patching MSYS again...\n\nOr we could pipe the output through us, i.e. attach a builtin version of\ncat at the output end of the called command. Only on Windows, of\ncourse. Better ugly and limping then wrong, right?\n\nRené\n"},{"id":"172626","messageId":"20110802040004.GC17494@sigill.intra.peff.net","threadId":"27847","inReplyTo":"4E37202B.80208@lsrfire.ath.cx","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-02T04:00:04Z","receivedAt":"2011-08-02T04:00:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 01, 2011 at 11:52:43PM +0200, René Scharfe wrote:\n\n> Am 01.08.2011 23:20, schrieb Johannes Sixt:\n> > Am 01.08.2011 22:48, schrieb René Scharfe:\n> >> So git archive gives the right results when writing to a pipe, but\n> >> always the same wrong result when writing directly to a file.\n> > \n> > This could indeed be a CRLF issue. archive-tar.c runs gzip to let it\n> > write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS\n> > is \"clever\" and sets up the channel in text mode (CRLF conversion) if it\n> > is a regular file, but in binary mode if it is a pipe.\n> > \n> > Without the gzip filter, git-archive writes to stdout itself. Since we\n> > have set up all our channels in binary mode, we do not suffer from the\n> > same problem for plain tar format.\n> > \n> > So, I don't think we can do a lot about it, short of patching MSYS again...\n> \n> Or we could pipe the output through us, i.e. attach a builtin version of\n> cat at the output end of the called command. Only on Windows, of\n> course. Better ugly and limping then wrong, right?\n\nYeah, that would work. But I am confused. If what Johannes says is true,\nisn't MSYS gzip totally broken for:\n\n # works\n echo foo | gzip -c | cat >foo.gz\n\n # broken; introduces CR\n echo foo | gzip -c >foo.gz\n\n? (The \"works\" and \"broken\" there are my guesses; I don't have a Windows\nbox to test on). IOW, is it simply gzip that is broken, and any fix we\ndo is simply working around a bug in gzip? And therefore the right\nsolution is for MSYS people to fix gzip?\n\n-Peff\n"},{"id":"172669","messageId":"4E3829DC.8070802@lsrfire.ath.cx","threadId":"27847","inReplyTo":"20110802040004.GC17494@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-02T16:46:20Z","receivedAt":"2011-08-02T16:46:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.08.2011 06:00, schrieb Jeff King:\n> On Mon, Aug 01, 2011 at 11:52:43PM +0200, René Scharfe wrote:\n> \n>> Am 01.08.2011 23:20, schrieb Johannes Sixt:\n>>> Am 01.08.2011 22:48, schrieb René Scharfe:\n>>>> So git archive gives the right results when writing to a pipe, but\n>>>> always the same wrong result when writing directly to a file.\n>>>\n>>> This could indeed be a CRLF issue. archive-tar.c runs gzip to let it\n>>> write to the original fd 1 (stdout). gzip is an MSYS program, and MSYS\n>>> is \"clever\" and sets up the channel in text mode (CRLF conversion) if it\n>>> is a regular file, but in binary mode if it is a pipe.\n>>>\n>>> Without the gzip filter, git-archive writes to stdout itself. Since we\n>>> have set up all our channels in binary mode, we do not suffer from the\n>>> same problem for plain tar format.\n>>>\n>>> So, I don't think we can do a lot about it, short of patching MSYS again...\n>>\n>> Or we could pipe the output through us, i.e. attach a builtin version of\n>> cat at the output end of the called command. Only on Windows, of\n>> course. Better ugly and limping then wrong, right?\n> \n> Yeah, that would work. But I am confused. If what Johannes says is true,\n> isn't MSYS gzip totally broken for:\n> \n> # works\n> echo foo | gzip -c | cat >foo.gz\n> \n> # broken; introduces CR\n> echo foo | gzip -c >foo.gz\n> \n> ? (The \"works\" and \"broken\" there are my guesses; I don't have a Windows\n> box to test on). IOW, is it simply gzip that is broken, and any fix we\n> do is simply working around a bug in gzip? And therefore the right\n> solution is for MSYS people to fix gzip?\n\n\"foo\" may be too short to trigger the issue as the small resulting gz\nfile has a low probability of containing LFs.\n\nThe output of gzip is not simply always mangled, though (taken from my\nearlier email, all three are working):\n\n\t$ git archive v1.7.6 | gzip -cn | md5sum\n\ta0ca1c873a533a5fcd41d248fb325a5b *-\n\n\t$ git archive --format=tar.gz v1.7.6 | md5sum\n\ta0ca1c873a533a5fcd41d248fb325a5b *-\n\n\t$ git archive v1.7.6 | gzip -cn >a.tgz && md5sum a.tgz && md5sum a.tgz && md5sum The output of gzip is not simply always mangled, though (taken from my\n> earlier email, all three are working):\n> \n> \t$ git archive v1.7.6 | gzip -cn | md5sum\n> \ta0ca1c873a533a5fcd41d248fb325a5b *-\n> \n> \t$ git archive --format=tar.gz v1.7.6 | md5sum\n> \ta0ca1c873a533a5fcd41d248fb325a5b *-\n> \n> \t$ git archive v1.7.6 | gzip -cn >a.tgz && md5sum \ta0ca1c873a533a5fcd41d248fb325a5b *-\n> \n> It's only broken if we call it from git archive:\n> \n> \t$ git archive --format=tar.gz v1.7.6 >a.tgz && md5sum \t30886283af1aed05ae6a36fc5aeda077 *-\n> \n> \t$ git archive -o a.tgz v1.7.6 && md5sum \t30886283af1aed05ae6a36fc5aeda077 *-\n> \n> But not if we stuff the result into a pipe instead of a file:\n> \n> \t$ git archive --format=tar.gz v1.7.6 | cat >a.tgz && md5sum \ta0ca1c873a533a5fcd41d248fb325a5b *-\n> \n> It _is_ confusing.\n\nHmm. So it's not _just_ the pipe vs file thing. What's different about\ncalling it from the shell, versus the way we call it from git-archive?\n\nIOW, the pipe trick you mentioned is one way to fix it; but it may be\nmore elegant to do whatever it is the shell is doing to get those\nresults (unless the shell is secretly putting a \"cat\" into the pipeline\n:) ).\n\n-Peff\n"},{"id":"172721","messageId":"4E388A55.6080606@kdbg.org","threadId":"27847","inReplyTo":"20110802181357.GA1861@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-08-02T23:37:57Z","receivedAt":"2011-08-02T23:37:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.08.2011 20:13, schrieb Jeff King:\n> Hmm. So it's not _just_ the pipe vs file thing. What's different about\n> calling it from the shell, versus the way we call it from git-archive?\n\nWhen the parent process of an MSYS process is itself an MSYS process,\nsuch as bash, then the child does not do its own\nbinary-mode-vs.-text-mode detection, but just uses whatever it is told\nby the parent. This is achieved by MSYS's fork emulation.\n\nBut if the parent is a regular Windows program, such as git(-archive),\nthen the autodection happens and file descriptors pointing to files are\nput into text mode.\n\n-- Hannes\n"},{"id":"172732","messageId":"20110803054958.GA7547@sigill.intra.peff.net","threadId":"27847","inReplyTo":"4E388A55.6080606@kdbg.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-03T05:49:58Z","receivedAt":"2011-08-03T05:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 03, 2011 at 01:37:57AM +0200, Johannes Sixt wrote:\n\n> Am 02.08.2011 20:13, schrieb Jeff King:\n> > Hmm. So it's not _just_ the pipe vs file thing. What's different about\n> > calling it from the shell, versus the way we call it from git-archive?\n> \n> When the parent process of an MSYS process is itself an MSYS process,\n> such as bash, then the child does not do its own\n> binary-mode-vs.-text-mode detection, but just uses whatever it is told\n> by the parent. This is achieved by MSYS's fork emulation.\n> \n> But if the parent is a regular Windows program, such as git(-archive),\n> then the autodection happens and file descriptors pointing to files are\n> put into text mode.\n\nYuck. Well, I guess that's not really an option, then. The pipe trick\nsounds like the sanest option (it would even be trivial to switch the\ndefault value to \"gzip -cn | cat\" on Windows, but I assume \"cat\" has the\nsame problem).\n\nThanks for the explanation.\n\n-Peff\n"},{"id":"173058","messageId":"4E3D0C1D.9000807@lsrfire.ath.cx","threadId":"27847","inReplyTo":"4E388A55.6080606@kdbg.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-06T09:40:45Z","receivedAt":"2011-08-06T09:40:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.08.2011 01:37, schrieb Johannes Sixt:\n> Am 02.08.2011 20:13, schrieb Jeff King:\n>> Hmm. So it's not _just_ the pipe vs file thing. What's different about\n>> calling it from the shell, versus the way we call it from git-archive?\n> \n> When the parent process of an MSYS process is itself an MSYS process,\n> such as bash, then the child does not do its own\n> binary-mode-vs.-text-mode detection, but just uses whatever it is told\n> by the parent. This is achieved by MSYS's fork emulation.\n> \n> But if the parent is a regular Windows program, such as git(-archive),\n> then the autodection happens and file descriptors pointing to files are\n> put into text mode.\n\nSo here's an ugly patch to implement an internal passthrough filter to\navoid newline conversions. It makes the tar filter command (gzip etc.)\nwrite to a pipe instead of directly to a file.\n\nThe patch does this unconditionally, which is a waste on all unaffected\nsystems, of course. We could #ifdef it out, but is there perhaps a nice\nway to integrate this functionality into run_command/finish_command?\n\nRené\n\n---\n archive-tar.c | 31 +++++++++++++++++++++++++++++++\n 1 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 20af005..22d52c5 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -314,6 +314,25 @@ static int write_tar_archive(const struct archiver *ar,\n \treturn err;\n }\n \n+static int cat_fn(int in, int out, void *data)\n+{\n+\tfor (;;) {\n+\t\tchar buf[16 * 1024];\n+\t\tssize_t got = xread(in, buf, sizeof(buf));\n+\t\tif (got == 0)\n+\t\t\tbreak;\n+\t\tif (got < 0) {\n+\t\t\tif (errno == EPIPE)\n+\t\t\t\tbreak;\n+\t\t\tdie_errno(\"read errror\");\n+\t\t}\n+\t\twrite_or_die(out, buf, got);\n+\t}\n+\tclose(in);\n+\tclose(out);\n+\treturn 0;\n+}\n+\n static int write_tar_filter_archive(const struct archiver *ar,\n \t\t\t\t struct archiver_args *args)\n {\n@@ -321,6 +340,14 @@ static int write_tar_filter_archive(const struct archiver *ar,\n \tstruct child_process filter;\n \tconst char *argv[2];\n \tint r;\n+\tstruct async cat;\n+\n+\tmemset(&cat, 0, sizeof(cat));\n+\tcat.proc = cat_fn;\n+\tcat.in = -1;\n+\tcat.out = dup(1);\n+\tif (start_async(&cat))\n+\t\tdie(\"unable to start passthrough filter\");\n \n \tif (!ar->data)\n \t\tdie(\"BUG: tar-filter archiver called with no filter defined\");\n@@ -335,6 +362,7 @@ static int write_tar_filter_archive(const struct archiver *ar,\n \tfilter.argv = argv;\n \tfilter.use_shell = 1;\n \tfilter.in = -1;\n+\tfilter.out = cat.in;\n \n \tif (start_command(&filter) < 0)\n \t\tdie_errno(\"unable to start '%s' filter\", argv[0]);\n@@ -349,6 +377,9 @@ static int write_tar_filter_archive(const struct archiver *ar,\n \tif (finish_command(&filter) != 0)\n \t\tdie(\"'%s' filter reported error\", argv[0]);\n \n+\tif (finish_async(&cat))\n+\t\tdie(\"passthrough filter reported error\");\n+\n \tstrbuf_release(&cmd);\n \treturn r;\n }\n"},{"id":"173102","messageId":"4E3EEF3B.80908@kdbg.org","threadId":"27847","inReplyTo":"4E3D0C1D.9000807@lsrfire.ath.cx","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-08-07T20:02:03Z","receivedAt":"2011-08-07T20:02:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.08.2011 11:40, schrieb René Scharfe:\n> So here's an ugly patch to implement an internal passthrough filter to\n> avoid newline conversions. It makes the tar filter command (gzip etc.)\n> write to a pipe instead of directly to a file.\n\n*If* we are working around the CRLF conversion issue in git itself,\nwouldn't it be much simpler to just:\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 20af005..da3d98a 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -364,9 +364,9 @@ void init_tar_archiver(void)\n \tint i;\n \tregister_archiver(&tar_archiver);\n\n-\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn\", NULL);\n+\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn | cat\", NULL);\n \ttar_filter_config(\"tar.tgz.remote\", \"true\", NULL);\n-\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn\", NULL);\n+\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn | cat\", NULL);\n \ttar_filter_config(\"tar.tar.gz.remote\", \"true\", NULL);\n \tgit_config(git_tar_config, NULL);\n \tfor (i = 0; i < nr_tar_filters; i++) {\n\n(provided that 'cat' magically does not suffer from the same problem,\nand I do think that it does not.)\n\nAnyway, I think it would be better to address the problem on the msysgit\nside. Perhaps by providing a gzip of a different vintage (e.g. a\nself-compiled one) that does not suffer from the CRLF conversion issue.\n\n-- Hannes\n"},{"id":"173104","messageId":"20110807210633.GA2336@sigill.intra.peff.net","threadId":"27847","inReplyTo":"4E3EEF3B.80908@kdbg.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-07T21:06:33Z","receivedAt":"2011-08-07T21:06:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 07, 2011 at 10:02:03PM +0200, Johannes Sixt wrote:\n\n> Am 06.08.2011 11:40, schrieb René Scharfe:\n> > So here's an ugly patch to implement an internal passthrough filter to\n> > avoid newline conversions. It makes the tar filter command (gzip etc.)\n> > write to a pipe instead of directly to a file.\n> \n> *If* we are working around the CRLF conversion issue in git itself,\n> wouldn't it be much simpler to just:\n> \n> diff --git a/archive-tar.c b/archive-tar.c\n> index 20af005..da3d98a 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -364,9 +364,9 @@ void init_tar_archiver(void)\n> \tint i;\n> \tregister_archiver(&tar_archiver);\n> \n> -\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn\", NULL);\n> +\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn | cat\", NULL);\n> \ttar_filter_config(\"tar.tgz.remote\", \"true\", NULL);\n> -\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn\", NULL);\n> +\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn | cat\", NULL);\n> \ttar_filter_config(\"tar.tar.gz.remote\", \"true\", NULL);\n> \tgit_config(git_tar_config, NULL);\n> \tfor (i = 0; i < nr_tar_filters; i++) {\n> \n> (provided that 'cat' magically does not suffer from the same problem,\n> and I do think that it does not.)\n\nI like that much better, but assumed cat was broken. It might be better\nstill to have a GZIP_FILTER variable in the Makefile. That would let\nmsysgit do this, but would also let people with a funny path to gzip\ndefine it at build time (they could also just reconfigure it, of course,\nbut it is nice to work out of the box on odd platforms).\n\n> Anyway, I think it would be better to address the problem on the msysgit\n> side. Perhaps by providing a gzip of a different vintage (e.g. a\n> self-compiled one) that does not suffer from the CRLF conversion issue.\n\nYeah, that would make me happy, too. :)\n\n-Peff\n"},{"id":"173138","messageId":"4E401869.8060702@lsrfire.ath.cx","threadId":"27847","inReplyTo":"4E3EEF3B.80908@kdbg.org","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-08T17:10:01Z","receivedAt":"2011-08-08T17:10:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.08.2011 22:02, schrieb Johannes Sixt:\n> Am 06.08.2011 11:40, schrieb René Scharfe:\n>> So here's an ugly patch to implement an internal passthrough filter to\n>> avoid newline conversions. It makes the tar filter command (gzip etc.)\n>> write to a pipe instead of directly to a file.\n> \n> *If* we are working around the CRLF conversion issue in git itself,\n> wouldn't it be much simpler to just:\n> \n> diff --git a/archive-tar.c b/archive-tar.c\n> index 20af005..da3d98a 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -364,9 +364,9 @@ void init_tar_archiver(void)\n> \tint i;\n> \tregister_archiver(&tar_archiver);\n> \n> -\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn\", NULL);\n> +\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn | cat\", NULL);\n> \ttar_filter_config(\"tar.tgz.remote\", \"true\", NULL);\n> -\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn\", NULL);\n> +\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn | cat\", NULL);\n> \ttar_filter_config(\"tar.tar.gz.remote\", \"true\", NULL);\n> \tgit_config(git_tar_config, NULL);\n> \tfor (i = 0; i < nr_tar_filters; i++) {\n> \n> (provided that 'cat' magically does not suffer from the same problem,\n> and I do think that it does not.)\n\nThe external cat can indeed be used. We'd need to do that for user\nsupplied commands as well, though, like this (ugh):\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 20af005..eaa9a1c 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -326,6 +326,9 @@ static int write_tar_filter_archive(const struct archiver *ar,\n \t\tdie(\"BUG: tar-filter archiver called with no filter defined\");\n \n \tstrbuf_addstr(&cmd, ar->data);\n+#ifdef WIN32\n+\tstrbuf_addstr(&cmd, \" | cat\");\n+#endif\n \tif (args->compression_level >= 0)\n \t\tstrbuf_addf(&cmd, \" -%d\", args->compression_level);\n \n\n> Anyway, I think it would be better to address the problem on the msysgit\n> side. Perhaps by providing a gzip of a different vintage (e.g. a\n> self-compiled one) that does not suffer from the CRLF conversion issue.\n\nUsers would probably also need sanitized versions of bzip2 and xz, and\nperhaps more.\n\nIf MSYS could be asked to refrain from switching file descriptors to text\nmode, e.g. by setting an environment variable, we could solve the issue\nin a generic way instead.\n\nRené\n"},{"id":"173195","messageId":"20110809050211.GA3588@sigill.intra.peff.net","threadId":"27847","inReplyTo":"4E401869.8060702@lsrfire.ath.cx","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-09T05:02:11Z","receivedAt":"2011-08-09T05:02:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2011 at 07:10:01PM +0200, René Scharfe wrote:\n\n> > -\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn\", NULL);\n> > +\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn | cat\", NULL);\n> > \ttar_filter_config(\"tar.tgz.remote\", \"true\", NULL);\n> > -\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn\", NULL);\n> > +\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn | cat\", NULL);\n> > \ttar_filter_config(\"tar.tar.gz.remote\", \"true\", NULL);\n> > \tgit_config(git_tar_config, NULL);\n> > \tfor (i = 0; i < nr_tar_filters; i++) {\n> > \n> > (provided that 'cat' magically does not suffer from the same problem,\n> > and I do think that it does not.)\n> \n> The external cat can indeed be used. We'd need to do that for user\n> supplied commands as well, though, like this (ugh):\n> \n> diff --git a/archive-tar.c b/archive-tar.c\n> index 20af005..eaa9a1c 100644\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -326,6 +326,9 @@ static int write_tar_filter_archive(const struct archiver *ar,\n> \t\tdie(\"BUG: tar-filter archiver called with no filter defined\");\n> \n> \tstrbuf_addstr(&cmd, ar->data);\n> +#ifdef WIN32\n> +\tstrbuf_addstr(&cmd, \" | cat\");\n> +#endif\n> \tif (args->compression_level >= 0)\n> \t\tstrbuf_addf(&cmd, \" -%d\", args->compression_level);\n\nDo we need to? It seems to me that defaulting to \"gzip -cn | cat\" is not\n\"we are on Windows, a platform that needs a special workaround in git\",\nbut rather \"this gzip is horribly broken, but at build-time you can\nset a gzip that works\".\n\nSo if the user wants to specify some broken filter, it is up to them to\nadd \"| cat\" if their filter merits it.\n\nBut that is somewhat a matter of perception, and it won't make a user on\nWindows who does \"git config archive.bz2 bzip2 -c\" any happier when they\nare told it is their responsibility to deal with it.\n\nBTW, as nice as this \"gzip -cn | cat\" idea is, I think it needs to be\nwrapped in a shell script. With the code above, we will generate \"gzip\n-cn | cat -9\". So we really need:\n\n $ cat `which gzip`\n #!/bin/sh\n gzip.real -cn \"$@\" | cat\n\nand then no hacks need to go into git at all. The fix is about providing\na sane gzip, not fixing git.\n\nBTW, from what Johannes said, the issue is about a non-msys program\ncalling an msys one. Does that mean that having git run:\n\n sh -c 'gzip -cn'\n\nwould work? If so, then could the solution be as simple as turning off\nthe \"don't bother with the shell\" optimization that run-command uses?\nSomething like \"gzip -cn\" gets split by git and run via spawn now\n(because it has no metacharacters). But we could easily make it always\nrun through the shell.\n\n-Peff\n"},{"id":"173204","messageId":"4E410B26.1080407@lsrfire.ath.cx","threadId":"27847","inReplyTo":"20110809050211.GA3588@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-08-09T10:25:42Z","receivedAt":"2011-08-09T10:25:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.08.2011 07:02, schrieb Jeff King:\n> On Mon, Aug 08, 2011 at 07:10:01PM +0200, René Scharfe wrote:\n> \n>>> -\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn\", NULL);\n>>> +\ttar_filter_config(\"tar.tgz.command\", \"gzip -cn | cat\", NULL);\n>>> \ttar_filter_config(\"tar.tgz.remote\", \"true\", NULL);\n>>> -\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn\", NULL);\n>>> +\ttar_filter_config(\"tar.tar.gz.command\", \"gzip -cn | cat\", NULL);\n>>> \ttar_filter_config(\"tar.tar.gz.remote\", \"true\", NULL);\n>>> \tgit_config(git_tar_config, NULL);\n>>> \tfor (i = 0; i < nr_tar_filters; i++) {\n>>>\n>>> (provided that 'cat' magically does not suffer from the same problem,\n>>> and I do think that it does not.)\n>>\n>> The external cat can indeed be used. We'd need to do that for user\n>> supplied commands as well, though, like this (ugh):\n>>\n>> diff --git a/archive-tar.c b/archive-tar.c\n>> index 20af005..eaa9a1c 100644\n>> --- a/archive-tar.c\n>> +++ b/archive-tar.c\n>> @@ -326,6 +326,9 @@ static int write_tar_filter_archive(const struct archiver *ar,\n>> \t\tdie(\"BUG: tar-filter archiver called with no filter defined\");\n>> \n>> \tstrbuf_addstr(&cmd, ar->data);\n>> +#ifdef WIN32\n>> +\tstrbuf_addstr(&cmd, \" | cat\");\n>> +#endif\n>> \tif (args->compression_level >= 0)\n>> \t\tstrbuf_addf(&cmd, \" -%d\", args->compression_level);\n> \n> Do we need to? It seems to me that defaulting to \"gzip -cn | cat\" is not\n> \"we are on Windows, a platform that needs a special workaround in git\",\n> but rather \"this gzip is horribly broken, but at build-time you can\n> set a gzip that works\".\n\nIt's an MSYS platform problem IMHO, and we can work around it. That\nextra cat is nothing to be proud of, sure. And msysgit avoiding the\nissue by distibuting a gzip that never converts line endings would solve\nit without any runtime overhead.\n\n> So if the user wants to specify some broken filter, it is up to them to\n> add \"| cat\" if their filter merits it.\n\nWell, I'd rather take this responsibility from the user, who might\nstruggle a while before finding out what is going on. However...\n\n> But that is somewhat a matter of perception, and it won't make a user on\n> Windows who does \"git config archive.bz2 bzip2 -c\" any happier when they\n> are told it is their responsibility to deal with it.\n\nIf all packers shipped with msysgit are usable then we're probably good.\n\n> BTW, as nice as this \"gzip -cn | cat\" idea is, I think it needs to be\n> wrapped in a shell script. With the code above, we will generate \"gzip\n> -cn | cat -9\".\n\nYes, the three added lines in the patch above would have to be moved\ndown two lines, after the compression level is added. D'oh!\n\n> So we really need:\n> \n> $ cat `which gzip`\n> #!/bin/sh\n> gzip.real -cn \"$@\" | cat\n>\n> and then no hacks need to go into git at all. The fix is about providing\n> a sane gzip, not fixing git.\n\nOK, that's one way to do it; another would be let gzip (and bzip2 etc.)\ndo whatever cat does to avoid end of line conversions. And yet another\nis to take them from http://unxutils.sourceforge.net/.\n\n> BTW, from what Johannes said, the issue is about a non-msys program\n> calling an msys one. Does that mean that having git run:\n> \n> sh -c 'gzip -cn'\n> \n> would work? If so, then could the solution be as simple as turning off\n> the \"don't bother with the shell\" optimization that run-command uses?\n> Something like \"gzip -cn\" gets split by git and run via spawn now\n> (because it has no metacharacters). But we could easily make it always\n> run through the shell.\n\nJust checked -- it doesn't work. I assume that's because the shell is\nalso an MSYS program.\n\nRené\n"},{"id":"173221","messageId":"20110809200504.GA31944@sigill.intra.peff.net","threadId":"27847","inReplyTo":"4E410B26.1080407@lsrfire.ath.cx","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-09T20:05:04Z","receivedAt":"2011-08-09T20:05:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2011 at 12:25:42PM +0200, René Scharfe wrote:\n\n> > BTW, as nice as this \"gzip -cn | cat\" idea is, I think it needs to be\n> > wrapped in a shell script. With the code above, we will generate \"gzip\n> > -cn | cat -9\".\n> \n> Yes, the three added lines in the patch above would have to be moved\n> down two lines, after the compression level is added. D'oh!\n\nAlso, is adding \"| cat\" also sufficient for arbitrary shell code (i.e.,\nwhatever the user hands us via the config)? I couldn't think of an\nexample that wouldn't work.\n\n> OK, that's one way to do it; another would be let gzip (and bzip2 etc.)\n> do whatever cat does to avoid end of line conversions. And yet another\n> is to take them from http://unxutils.sourceforge.net/.\n\nYeah, I like all of those solutions better than hacking an extra pipe\ninto git. I don't know enough to say how painful they are in practice,\nthough.\n\n> > BTW, from what Johannes said, the issue is about a non-msys program\n> > calling an msys one. Does that mean that having git run:\n> > \n> > sh -c 'gzip -cn'\n> > \n> > would work? If so, then could the solution be as simple as turning off\n> > the \"don't bother with the shell\" optimization that run-command uses?\n> > Something like \"gzip -cn\" gets split by git and run via spawn now\n> > (because it has no metacharacters). But we could easily make it always\n> > run through the shell.\n> \n> Just checked -- it doesn't work. I assume that's because the shell is\n> also an MSYS program.\n\nToo bad. I guess the first msys program to be run sets the mode of the\noutput descriptor, and then everybody else inherits that. If there were\na helper or shell builtin to switch to binary mode, we could put it at\nthe start of the shell pipeline.\n\nIf you write a C program that does:\n\n setmode(1, O_BINARY);\n\nand call it like:\n\n ./setmode-helper; gzip -cn\n\ndoes that work? I suspect not, as from my brief reading of the msys gzip\nsource, it actually calls setmode() itself.\n\n-Peff\n"},{"id":"176515","messageId":"CABPQNSba+nQ+AOmVEVNWhJi2e65EiuhRxfOWZsW03Pkc6zXdHg@mail.gmail.com","threadId":"27847","inReplyTo":"20110809200504.GA31944@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-09-29T19:54:04Z","receivedAt":"2011-09-29T19:54:04Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"OK, this is quite late, but I've got some news on the subject.\n\nOn Tue, Aug 9, 2011 at 10:05 PM, Jeff King wrote:\n> On Tue, Aug 09, 2011 at 12:25:42PM +0200, René Scharfe wrote:\n>\n>> > BTW, as nice as this \"gzip -cn | cat\" idea is, I think it needs to be\n>> > wrapped in a shell script. With the code above, we will generate \"gzip\n>> > -cn | cat -9\".\n>>\n>> Yes, the three added lines in the patch above would have to be moved\n>> down two lines, after the compression level is added.  D'oh!\n>\n> Also, is adding \"| cat\" also sufficient for arbitrary shell code (i.e.,\n> whatever the user hands us via the config)? I couldn't think of an\n> example that wouldn't work.\n>\n>> OK, that's one way to do it; another would be let gzip (and bzip2 etc.)\n>> do whatever cat does to avoid end of line conversions.  And yet another\n>> is to take them from http://unxutils.sourceforge.net/.\n>\n> Yeah, I like all of those solutions better than hacking an extra pipe\n> into git. I don't know enough to say how painful they are in practice,\n> though.\n>\n\nIt turns out, if I compile gzip myself from source the test pass just\nfine. So the problem seems indeed to be MSYS vs non-MSYS applications.\n\nSo the way forward is probably to just change to a self-compiled gzip\nin msysGit (and Git for Windows by proxy).\n\nI'll post the patch to build a native-gzip to the msysGit mailing list\n(I only got 1.2.4 to compile on Windows, but it's the same version as\nwe have in msysGit so it's probably fine), and post the latest version\nof this series soon...\n"},{"id":"176518","messageId":"4E84D29F.6020305@lsrfire.ath.cx","threadId":"27847","inReplyTo":"CABPQNSba+nQ+AOmVEVNWhJi2e65EiuhRxfOWZsW03Pkc6zXdHg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-09-29T20:18:39Z","receivedAt":"2011-09-29T20:18:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.09.2011 21:54, schrieb Erik Faye-Lund:\n> So the way forward is probably to just change to a self-compiled gzip\n> in msysGit (and Git for Windows by proxy).\n> \n> I'll post the patch to build a native-gzip to the msysGit mailing list\n> (I only got 1.2.4 to compile on Windows, but it's the same version as\n> we have in msysGit so it's probably fine), and post the latest version\n> of this series soon...\n\nGood to hear that.\n\nBy the way, are you aware of http://unxutils.sourceforge.net/ (includes\na native gzip 1.2.4 binary)?\n\nOr http://zlib.net/pigz/ (parallel gzip, no binaries)?\n\nRené\n"},{"id":"176519","messageId":"CABPQNSZaEdBMRq9wMbGV1E=EzZgpcKhT=_JW-3hSzF4GqviHHQ@mail.gmail.com","threadId":"27847","inReplyTo":"4E84D29F.6020305@lsrfire.ath.cx","subject":"Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-09-29T20:20:29Z","receivedAt":"2011-09-29T20:20:29Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Sep 29, 2011 at 10:18 PM, René Scharfe\n wrote:\n> Am 29.09.2011 21:54, schrieb Erik Faye-Lund:\n>> So the way forward is probably to just change to a self-compiled gzip\n>> in msysGit (and Git for Windows by proxy).\n>>\n>> I'll post the patch to build a native-gzip to the msysGit mailing list\n>> (I only got 1.2.4 to compile on Windows, but it's the same version as\n>> we have in msysGit so it's probably fine), and post the latest version\n>> of this series soon...\n>\n> Good to hear that.\n>\n> By the way, are you aware of http://unxutils.sourceforge.net/ (includes\n> a native gzip 1.2.4 binary)?\n\nYes, but we prefer to build our binaries ourselves in the msysGit project.\n\n> Or http://zlib.net/pigz/ (parallel gzip, no binaries)?\n\nI'm aware of pigz, but changing the gzip implementation isn't really\nthe scope of this change IMO.\n"}]}