In theory it is possible for sideband channel #2 to be delayed if
pack data is quick to come up for sideband channel #1.  And because
data for channel #2 is read only 128 bytes at a time while pack data
is read 8192 bytes at a time, it is possible for many pack blocks to
be sent to the client before the progress message fifo is emptied,
making the situation even worse.  This would result in totally garbled
progress display on the client's console as local progress gets mixed
with partial remote progress lines.

Let's prevent such situations by giving transmission priority to 
progress messages over pack data at all times.

Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---

On Wed, 11 Nov 2009, Petr Baudis wrote:

quoted text
> On Wed, Nov 11, 2009 at 04:19:44PM -0500, Nicolas Pitre wrote:
> > On Wed, 11 Nov 2009, Petr Baudis wrote:
> > 
> > > On Wed, Nov 11, 2009 at 02:50:22PM -0500, Nicolas Pitre wrote:
> > > > According to strace, data from sideband channel #2 (prefixed with 
> > > > "remote: ") pertaining to object compression is printed way after pack 
> > > > data has already started to arrive locally.  This is really weird.
> > > > 
> > > > And this occurs only when fetching from repo.or.cz and not from 
> > > > git.kernel.org for example.  So there is something to investigate on the 
> > > > server side.  Pasky: anything you changed in your git installation 
> > > > lately?
> > > 
> > > Yes, but nothing should have changed in git-daemon, that's the only part
> > > of the infrastructure that uses system-wide git (which it perhaps
> > > shouldn't). I cannot reproduce this problem, though. I have changed
> > > git-daemon to use my local git version (about one week old master), does
> > > this still happen for you?
> > 
> > No, it doesn't happen anymore.
> > 
> > What was the git-daemon version before?
> 
> 1.5.6.5, the default version in debian lenny

I don't see why the issue couldn't happen with latest git as well.
This patch however would plug any possibilities for this to happen again 
though.

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index c4cd1e1..29446e8 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -132,7 +132,6 @@ 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,15 +146,14 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 			}
 			continue;
 		}
-		if (pfd[0].revents & POLLIN)
-			/* Data stream ready */
-			processed[0] = process_input(pfd[0].fd, 1);
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			processed[1] = process_input(pfd[1].fd, 2);
-		/* Always finish to read data when available */
-		if (processed[0] || processed[1])
-			continue;
+			if (process_input(pfd[1].fd, 2))
+				continue;
+		if (pfd[0].revents & POLLIN)
+			/* Data stream ready */
+			if (process_input(pfd[0].fd, 1))
+				continue;
 
 		if (waitpid(writer, &status, 0) < 0)
 			error_clnt("%s", lostchild);
diff --git a/upload-pack.c b/upload-pack.c
index 70badcf..6bfb500 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -310,6 +310,23 @@ static void create_pack_file(void)
 			}
 			continue;
 		}
+		if (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {
+			/* Status ready; we ship that in the side-band
+			 * or dump to the standard error.
+			 */
+			sz = xread(pack_objects.err, progress,
+				  sizeof(progress));
+			if (0 < sz)
+				send_client_data(2, progress, sz);
+			else if (sz == 0) {
+				close(pack_objects.err);
+				pack_objects.err = -1;
+			}
+			else
+				goto fail;
+			/* give priority to status messages */
+			continue;
+		}
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
 			/* Data ready; we keep the last byte to ourselves
 			 * in case we detect broken rev-list, so that we
@@ -347,21 +364,6 @@ static void create_pack_file(void)
 			if (sz < 0)
 				goto fail;
 		}
-		if (0 <= pe && (pfd[pe].revents & (POLLIN|POLLHUP))) {
-			/* Status ready; we ship that in the side-band
-			 * or dump to the standard error.
-			 */
-			sz = xread(pack_objects.err, progress,
-				  sizeof(progress));
-			if (0 < sz)
-				send_client_data(2, progress, sz);
-			else if (sz == 0) {
-				close(pack_objects.err);
-				pack_objects.err = -1;
-			}
-			else
-				goto fail;
-		}
 	}
 
 	if (finish_command(&pack_objects)) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html