git archive, cygwin, and --git-dir vs --remote

I'm using git on cygwin, and am confused by behavior from git archive.

git --version
git version 1.6.1.2

This command works as expected:
git --git-dir=/cygdrive/w archive --format=tar --verbose --prefix=tmp/
HEAD | tar -xpf -
The latest copy is retrieved and dumped to the tmp subdirectory.

However, I first went down the path of using the --remote option, as
described in the git-archive man page:
git archive --format=tar --verbose --prefix=tmp/ --remote=/cygdrive/w
HEAD | tar -xpf -

When I use the --remote branch it seems to create the same files.  But
then rather than exiting it hangs out until I kill the process, using
about the same CPU % as it did when creating files.

I'm trying to use this for a scripted build process where all I need
is the latest copy, not an archive.  And we're just starting with git,
so while it works fine now I think we're supposed to be switch from
shared drives to something more controlled, at which point I don't
know that the --git-dir option will work.

I've tried searching the mailing list archive, and got overwhelmed by
patches, so I apologize if this has already been discussed and I just
couldn't find it.

Bob Kagy schrieb:

I'm using git on cygwin, and am confused by behavior from git archive.

git --version
git version 1.6.1.2

This command works as expected:
git --git-dir=/cygdrive/w archive --format=tar --verbose --prefix=tmp/
HEAD | tar -xpf -
The latest copy is retrieved and dumped to the tmp subdirectory.

However, I first went down the path of using the --remote option, as
described in the git-archive man page:
git archive --format=tar --verbose --prefix=tmp/ --remote=/cygdrive/w
HEAD | tar -xpf -

When I use the --remote branch it seems to create the same files.  But
then rather than exiting it hangs out until I kill the process, using
about the same CPU % as it did when creating files.

That's strange.  It seems that poll() reports that there is data to read
from the child (which is running git-upload-archive), even though it
already called exit().

The following patch works around this issue by terminating the otherwise
endless loop after read() returned nothing for the thousandth time in a
row.  I'm not sure that there's really no way to get a thousand empty
reads without the child being done, though.

 builtin-upload-archive.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 0206b41..3d7b11b 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c

@@ -17,6 +17,7 @@ static const char lostchild[] =
 "git upload-archive: archiver process was lost";
 
 #define MAX_ARGS (64)
+#define MAX_EMPTY_READS (1000)
 
 static int run_upload_archive(int argc, const char **argv, const char *prefix)
 {

@@ -80,10 +81,11 @@ static void error_clnt(const char *fmt, ...)
 	die("sent error to the client: %s", buf);
 }
 
-static void process_input(int child_fd, int band)
+static void process_input(int child_fd, int band, int *empty_reads)
 {
 	char buf[16384];
 	ssize_t sz = read(child_fd, buf, sizeof(buf));
+	*empty_reads = sz ? 0 : *empty_reads + 1;
 	if (sz < 0) {
 		if (errno != EAGAIN && errno != EINTR)
 			error_clnt("read error: %s\n", strerror(errno));

@@ -96,6 +98,8 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	pid_t writer;
 	int fd1[2], fd2[2];
+	int empty_reads_1 = 0;
+	int empty_reads_2 = 0;
 	/*
 	 * Set up sideband subprocess.
 	 *

@@ -147,12 +151,14 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		}
 		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			process_input(pfd[0].fd, 1);
+			process_input(pfd[0].fd, 1, &empty_reads_1);
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			process_input(pfd[1].fd, 2);
+			process_input(pfd[1].fd, 2, &empty_reads_2);
 		/* Always finish to read data when available */
-		if ((pfd[0].revents | pfd[1].revents) & POLLIN)
+		if (((pfd[0].revents | pfd[1].revents) & POLLIN) &&
+		    empty_reads_1 <= MAX_EMPTY_READS &&
+		    empty_reads_2 <= MAX_EMPTY_READS)
 			continue;
 
 		if (waitpid(writer, &status, 0) < 0)

On Thu, 21 May 2009, René Scharfe wrote:

That's strange.  It seems that poll() reports that there is data to read
from the child (which is running git-upload-archive), even though it
already called exit().

Poll reports an FD is readable when it reaches EOF.

The following patch works around this issue by terminating the otherwise
endless loop after read() returned nothing for the thousandth time in a
row.

You should stop reading the first time read() returns 0 i.e. EOF.

Tony.
-- 
f.anthony.n.finch  [off-list ref]  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.

Tony Finch schrieb:

On Thu, 21 May 2009, René Scharfe wrote:

quoted

That's strange.  It seems that poll() reports that there is data to read
from the child (which is running git-upload-archive), even though it
already called exit().

Poll reports an FD is readable when it reaches EOF.

OK, makes sense.  I still don't understand why upload-archive doesn't get
into an infinite loop on Linux (Fedora 10), though.

quoted

The following patch works around this issue by terminating the otherwise
endless loop after read() returned nothing for the thousandth time in a
row.

You should stop reading the first time read() returns 0 i.e. EOF.

Thanks.  In that case the following patch is better.


 builtin-upload-archive.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 0206b41..a3fa5b3 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c

@@ -80,16 +80,19 @@ static void error_clnt(const char *fmt, ...)
 	die("sent error to the client: %s", buf);
 }
 
-static void process_input(int child_fd, int band)
+static int process_input(int child_fd, int band)
 {
 	char buf[16384];
 	ssize_t sz = read(child_fd, buf, sizeof(buf));
+	if (sz == 0)
+		return EOF;
 	if (sz < 0) {
 		if (errno != EAGAIN && errno != EINTR)
 			error_clnt("read error: %s\n", strerror(errno));
-		return;
+		return 0;
 	}
 	send_sideband(1, band, buf, sz, LARGE_PACKET_MAX);
+	return 0;
 }
 
 int cmd_upload_archive(int argc, const char **argv, const char *prefix)

@@ -131,7 +134,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 
 	while (1) {
 		struct pollfd pfd[2];
-		int status;
+		int status, both_at_eof = EOF;
 
 		pfd[0].fd = fd1[0];
 		pfd[0].events = POLLIN;

@@ -147,12 +150,12 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		}
 		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			process_input(pfd[0].fd, 1);
+			both_at_eof &= process_input(pfd[0].fd, 1);
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			process_input(pfd[1].fd, 2);
+			both_at_eof &= process_input(pfd[1].fd, 2);
 		/* Always finish to read data when available */
-		if ((pfd[0].revents | pfd[1].revents) & POLLIN)
+		if (!both_at_eof)
 			continue;
 
 		if (waitpid(writer, &status, 0) < 0)

On Cygwin, poll() reports POLLIN even for file descriptors that have
reached their end.  This caused git upload-archive to be stuck in an
infinite loop, as it only looked at the POLLIN flag.

In addition to POLLIN, check if read() returned 0, which indicates
end-of-file, and keep looping only as long as at least one of the file
descriptors has input.  This lets the following command finish on its
own when run in a git repository on Cygwin, instead of it getting stuck
after printing all file names:

	$ git archive -v --remote . HEAD >/dev/null

Reported-by: Bob Kagy <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
This version of the patch has been cleaned up a bit compared to the
previous one and is slightly shorter, but does the same.

 builtin-upload-archive.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 0206b41..c4cd1e1 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c

@@ -80,16 +80,17 @@ static void error_clnt(const char *fmt, ...)
 	die("sent error to the client: %s", buf);
 }
 
-static void process_input(int child_fd, int band)
+static ssize_t process_input(int child_fd, int band)
 {
 	char buf[16384];
 	ssize_t sz = read(child_fd, buf, sizeof(buf));
 	if (sz < 0) {
 		if (errno != EAGAIN && errno != EINTR)
 			error_clnt("read error: %s\n", strerror(errno));
-		return;
+		return sz;
 	}
 	send_sideband(1, band, buf, sz, LARGE_PACKET_MAX);
+	return sz;
 }
 
 int cmd_upload_archive(int argc, const char **argv, const char *prefix)

@@ -131,6 +132,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 
 	while (1) {
 		struct pollfd pfd[2];
+		ssize_t processed[2] = { 0, 0 };
 		int status;
 
 		pfd[0].fd = fd1[0];

@@ -147,12 +149,12 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		}
 		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			process_input(pfd[0].fd, 1);
+			processed[0] = process_input(pfd[0].fd, 1);
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			process_input(pfd[1].fd, 2);
+			processed[1] = process_input(pfd[1].fd, 2);
 		/* Always finish to read data when available */
-		if ((pfd[0].revents | pfd[1].revents) & POLLIN)
+		if (processed[0] || processed[1])
 			continue;
 
 		if (waitpid(writer, &status, 0) < 0)

-- 
1.6.3.2.307.g21cd0

On Wed, 17 Jun 2009, René Scharfe wrote:

On Cygwin, poll() reports POLLIN even for file descriptors that have
reached their end.  This caused git upload-archive to be stuck in an
infinite loop, as it only looked at the POLLIN flag.

I think it isn't just Cygwin that does this - see
http://www.greenend.org.uk/rjk/2001/06/poll.html

Tony.
-- 
f.anthony.n.finch  [off-list ref]  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.