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

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

[Bob Kagy]

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.

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

[René Scharfe]

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)

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

[Tony Finch]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 665 bytes --]

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  <dot@dotat.at>  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.

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

[René Scharfe]

Tony Finch schrieb:
> 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.

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

>> 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)

[PATCH] upload-archive: fix infinite loop on Cygwin

[René Scharfe]

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 <bobkagy@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
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

Re: [PATCH] upload-archive: fix infinite loop on Cygwin

[Tony Finch]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 508 bytes --]

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  <dot@dotat.at>  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.