[PATCH] Fixup no-progress for fetch & clone

[PATCH] Fixup no-progress for fetch & clone

[Johannes Schindelin]

The intent of the commit 'fetch & clone: do not output progress when
not on a tty' was to make fetching and cloning less chatty when
output was not redirected (such as in a cron job).

However, there was a serious thinko in that commit. It assumed that
the client _and_ the server got this update at the same time. But
this is obviously not the case, and therefore upload-pack died on
seeing the option "--no-progress".

This patch fixes that issue by making it a protocol option. So, until
your server is updated, you still see the progress, but once the
server has this patch, it will be quiet.

A minor issue was also fixed: when cloning, the checkout did not
heed no_progress.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Do you want me to fold that back into the original patch?

 Documentation/git-upload-pack.txt |    5 +----
 fetch-pack.c                      |   10 +++-------
 git-clone.sh                      |    2 +-
 upload-pack.c                     |   10 ++++------
 4 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index c75c86e..fd65192 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -8,7 +8,7 @@ git-upload-pack - Send objects packed back to git-fetch-pack
 
 SYNOPSIS
 --------
-'git-upload-pack' [--strict] [--timeout=<n>] [--no-progress] <directory>
+'git-upload-pack' [--strict] [--timeout=<n>] <directory>
 
 DESCRIPTION
 -----------
@@ -30,9 +30,6 @@ OPTIONS
 \--timeout=<n>::
 	Interrupt transfer after <n> seconds of inactivity.
 
-\--no-progress::
-	Do not show the progress.
-
 <directory>::
 	The repository to sync from.
 
diff --git a/fetch-pack.c b/fetch-pack.c
index fc6b4e0..8428546 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -174,12 +174,13 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 		}
 
 		if (!fetching)
-			packet_write(fd[1], "want %s%s%s%s%s%s\n",
+			packet_write(fd[1], "want %s%s%s%s%s%s%s\n",
 				     sha1_to_hex(remote),
 				     (multi_ack ? " multi_ack" : ""),
 				     (use_sideband == 2 ? " side-band-64k" : ""),
 				     (use_sideband == 1 ? " side-band" : ""),
 				     (use_thin_pack ? " thin-pack" : ""),
+				     (no_progress ? " no-progress" : ""),
 				     " ofs-delta");
 		else
 			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
@@ -732,12 +733,7 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	if (no_progress) {
-		char buf[256];
-		snprintf(buf, sizeof(buf), "%s --no-progress", uploadpack);
-		pid = git_connect(fd, dest, buf);
-	} else
-		pid = git_connect(fd, dest, uploadpack);
+	pid = git_connect(fd, dest, uploadpack);
 	if (pid < 0)
 		return 1;
 	if (heads && nr_heads)
diff --git a/git-clone.sh b/git-clone.sh
index 86890ea..de51983 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -395,7 +395,7 @@ then
 
 	case "$no_checkout" in
 	'')
-		test "z$quiet" = z && v=-v || v=
+		test "z$quiet" = z -a "z$no_progress" = z && v=-v || v=
 		git-read-tree -m -u $v HEAD HEAD
 	esac
 fi
diff --git a/upload-pack.c b/upload-pack.c
index d1be07f..d9907ca 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -10,7 +10,7 @@
 #include "revision.h"
 #include "list-objects.h"
 
-static const char upload_pack_usage[] = "git-upload-pack [--strict] [--timeout=nn] [--no-progress] <dir>";
+static const char upload_pack_usage[] = "git-upload-pack [--strict] [--timeout=nn] <dir>";
 
 /* bits #0..7 in revision.h, #8..10 in commit.c */
 #define THEY_HAVE	(1u << 11)
@@ -547,6 +547,8 @@ static void receive_needs(void)
 			use_sideband = LARGE_PACKET_MAX;
 		else if (strstr(line+45, "side-band"))
 			use_sideband = DEFAULT_PACKET_MAX;
+		if (strstr(line+45, "no-progress"))
+			no_progress = 1;
 
 		/* We have sent all our refs already, and the other end
 		 * should have chosen out of them; otherwise they are
@@ -615,7 +617,7 @@ static void receive_needs(void)
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
-		" side-band-64k ofs-delta shallow";
+		" side-band-64k ofs-delta shallow no-progress";
 	struct object *o = parse_object(sha1);
 
 	if (!o)
@@ -670,10 +672,6 @@ int main(int argc, char **argv)
 			timeout = atoi(arg+10);
 			continue;
 		}
-		if (!strcmp(arg, "--no-progress")) {
-			no_progress = 1;
-			continue;
-		}
 		if (!strcmp(arg, "--")) {
 			i++;
 			break;
-- 
1.5.0.1.2248.g9f85a-dirty

Re: [PATCH] Fixup no-progress for fetch & clone

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	Do you want me to fold that back into the original patch?

The js/fetch-progress topic is now part of 'next' so I won't
rewind it.  Fix-up on top as you did is preferable.

Re: [PATCH] Fixup no-progress for fetch & clone

[Johannes Schindelin]

Hi,

Since most servers will not have a no-progress aware upload-pack, how 
about this? (It is slightly ugly, but at least works...)

-- snipsnap --
[PATCH] git-fetch: add --no-progress, work around non-aware upload-pack

This adds the option "--no-progress" to git-fetch, and adds a
workaround for the case when the other side's upload-pack does
not support the "no-progress" extension: it skips the side-band
packets ending in "done\r".

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 Documentation/fetch-options.txt |    5 +++++
 builtin-archive.c               |    2 +-
 fetch-pack.c                    |    2 +-
 git-fetch.sh                    |    3 +++
 sideband.c                      |    6 +++++-
 sideband.h                      |    2 +-
 6 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 5b4d184..3afb28e 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -22,6 +22,11 @@
 	and stores them locally.  This option disables this
 	automatic tag following.
 
+\--no-progress::
+	Tell the other side not to show progress. If the other
+	side's upload-pack is new enough, it will actually
+	heed that request.
+
 -t, \--tags::
 	Most of the tags are fetched automatically as branch
 	heads are downloaded, but tags that do not point at
diff --git a/builtin-archive.c b/builtin-archive.c
index 8ea6cb1..3d3fd1b 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -72,7 +72,7 @@ static int run_remote_archiver(const char *remote, int argc,
 		die("git-archive: expected a flush");
 
 	/* Now, start reading from fd[0] and spit it out to stdout */
-	rv = recv_sideband("archive", fd[0], 1, 2);
+	rv = recv_sideband("archive", fd[0], 1, 2, 0);
 	close(fd[0]);
 	close(fd[1]);
 	rv |= finish_connect(pid);
diff --git a/fetch-pack.c b/fetch-pack.c
index 06f4aec..fd38616 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -482,7 +482,7 @@ static pid_t setup_sideband(int fd[2], int xd[2])
 		close(fd[0]);
 		if (xd[0] != xd[1])
 			close(xd[1]);
-		if (recv_sideband("fetch-pack", xd[0], fd[1], 2))
+		if (recv_sideband("fetch-pack", xd[0], fd[1], 2, no_progress))
 			exit(1);
 		exit(0);
 	}
diff --git a/git-fetch.sh b/git-fetch.sh
index 5ae0d28..e7a2be4 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -51,6 +51,9 @@ do
 	-n|--n|--no|--no-|--no-t|--no-ta|--no-tag|--no-tags)
 		no_tags=t
 		;;
+	--no-progress)
+		no_progress=--no-progress
+		;;
 	-u|--u|--up|--upd|--upda|--updat|--update|--update-|--update-h|\
 	--update-he|--update-hea|--update-head|--update-head-|\
 	--update-head-o|--update-head-ok)
diff --git a/sideband.c b/sideband.c
index 277fa3c..6a1a181 100644
--- a/sideband.c
+++ b/sideband.c
@@ -11,7 +11,8 @@
  * stream, aka "verbose").  A message over band #3 is a signal that
  * the remote died unexpectedly.  A flush() concludes the stream.
  */
-int recv_sideband(const char *me, int in_stream, int out, int err)
+int recv_sideband(const char *me, int in_stream, int out, int err,
+		int no_progress)
 {
 	char buf[7 + LARGE_PACKET_MAX + 1];
 	strcpy(buf, "remote:");
@@ -26,6 +27,9 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 		band = buf[7] & 0xff;
+		if (no_progress && band != 3 && len > 7 + 5 &&
+				!prefixcmp(buf + 7 + len - 5, "done\r"))
+			continue;
 		len--;
 		switch (band) {
 		case 3:
diff --git a/sideband.h b/sideband.h
index a84b691..99a0e96 100644
--- a/sideband.h
+++ b/sideband.h
@@ -7,7 +7,7 @@
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
 
-int recv_sideband(const char *me, int in_stream, int out, int err);
+int recv_sideband(const char *me, int in_stream, int out, int err, int no_progress);
 ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet_max);
 
 #endif

Re: [PATCH] Fixup no-progress for fetch & clone

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Since most servers will not have a no-progress aware upload-pack, how 
> about this? (It is slightly ugly, but at least works...)

Please no.

What's "slight" about this ugliness?

> +		if (no_progress && band != 3 && len > 7 + 5 &&
> +				!prefixcmp(buf + 7 + len - 5, "done\r"))
> +			continue;

Re: [PATCH] Fixup no-progress for fetch & clone

[Johannes Schindelin]

Hi,

On Fri, 23 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Since most servers will not have a no-progress aware upload-pack, how 
> > about this? (It is slightly ugly, but at least works...)
> 
> Please no.
> 
> What's "slight" about this ugliness?
> 
> > +		if (no_progress && band != 3 && len > 7 + 5 &&
> > +				!prefixcmp(buf + 7 + len - 5, "done\r"))
> > +			continue;

This is not the part I found ugly. In sideband, there is a lot of 7+len, 
8+len, buf+7.

The part I found ugly is having to touch builtin-archive.c

But I agree that we should not include that in mainline git. But at least 
this patch is in the open for people who care deeply about no-progress 
_now_...

Ciao,
Dscho

Re: [PATCH] Fixup no-progress for fetch & clone

[Nicolas Pitre]

On Sat, 24 Feb 2007, Johannes Schindelin wrote:

> Hi,
> 
> Since most servers will not have a no-progress aware upload-pack, how 
> about this? (It is slightly ugly, but at least works...)

Just slightly.

Why not simply filtering any string that ends in '\r' ?


Nicolas

Re: [PATCH] Fixup no-progress for fetch & clone

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Sat, 24 Feb 2007, Johannes Schindelin wrote:
>
>> Hi,
>> 
>> Since most servers will not have a no-progress aware upload-pack, how 
>> about this? (It is slightly ugly, but at least works...)
>
> Just slightly.
>
> Why not simply filtering any string that ends in '\r' ?

Yes.  I think that I can live with.

Re: [PATCH] Fixup no-progress for fetch & clone

[Johannes Schindelin]

Hi,

On Fri, 23 Feb 2007, Nicolas Pitre wrote:

> On Sat, 24 Feb 2007, Johannes Schindelin wrote:
> 
> > Since most servers will not have a no-progress aware upload-pack, how 
> > about this? (It is slightly ugly, but at least works...)
> 
> Just slightly.
> 
> Why not simply filtering any string that ends in '\r' ?

As I mentioned in another mail, I find this issue not so ugly as that the 
patch touches a totally unrelated file, builtin-archive.c.

Ciao,
Dscho

Re: [PATCH] Fixup no-progress for fetch & clone

[Nicolas Pitre]

On Sat, 24 Feb 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Fri, 23 Feb 2007, Nicolas Pitre wrote:
> 
> > On Sat, 24 Feb 2007, Johannes Schindelin wrote:
> > 
> > > Since most servers will not have a no-progress aware upload-pack, how 
> > > about this? (It is slightly ugly, but at least works...)
> > 
> > Just slightly.
> > 
> > Why not simply filtering any string that ends in '\r' ?
> 
> As I mentioned in another mail, I find this issue not so ugly as that the 
> patch touches a totally unrelated file, builtin-archive.c.

Actually that is not the part that bothered me at all.

What really bothered me is the implicit knowledge added to the sideband 
stuff about "done\r".

And since all progress strings ar likely to end in '\r' for easy 
redrawing then I think your patch would be so much cleaner if you'd only 
discard any remote string that ends in \r, or only contains a single \n.  
After all those are the strings you don't want to see in your log files.

And I would do that and _only_ that to supress progress.  No messing 
around with protocol extensions.  Just filter out the unwanted messages 
locally and be happy.


Nicolas

Re: [PATCH] Fixup no-progress for fetch & clone

[Johannes Schindelin]

Hi,

On Fri, 23 Feb 2007, Nicolas Pitre wrote:

> And I would do that and _only_ that to supress progress.  No messing 
> around with protocol extensions.  Just filter out the unwanted messages 
> locally and be happy.

This is easier, but I'd feel dirty about it. I mean, you set up a signal 
every so often, have a side band, demultiplex the side band, and for what? 
To just ignore it?

Ciao,
Dscho

Re: [PATCH] Fixup no-progress for fetch & clone

[Nicolas Pitre]

On Sat, 24 Feb 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Fri, 23 Feb 2007, Nicolas Pitre wrote:
> 
> > And I would do that and _only_ that to supress progress.  No messing 
> > around with protocol extensions.  Just filter out the unwanted messages 
> > locally and be happy.
> 
> This is easier, but I'd feel dirty about it. I mean, you set up a signal 
> every so often, have a side band, demultiplex the side band, and for what? 
> To just ignore it?

Yes sir.  But of course this isn't the default.  

And it isn't even the common case either.  So I think a bit of cheating 
to keep the common case simple isn't that bad.

And it is not like if the whole thing was thrown away.  For example I 
think messages like

	remote: Writing 1234 objects.

are good messages to have in a log file even when progress display is 
filtered out.  So the sideband demultiplexing is useful in that case as 
well.


Nicolas

Re: [PATCH] Fixup no-progress for fetch & clone

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> And it is not like if the whole thing was thrown away.  For example I 
> think messages like
>
> 	remote: Writing 1234 objects.
>
> are good messages to have in a log file even when progress display is 
> filtered out.  So the sideband demultiplexing is useful in that case as 
> well.

In that case, maybe we should define a separate sideband for
progress display?  Currently #1 (payload) and #3 (emergency
exit) are distinct but #2 corresponds to stderr which has info
messages and progress noise all mixed up.

Re: [PATCH] Fixup no-progress for fetch & clone

[Nicolas Pitre]

On Fri, 23 Feb 2007, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> writes:
> 
> > And it is not like if the whole thing was thrown away.  For example I 
> > think messages like
> >
> > 	remote: Writing 1234 objects.
> >
> > are good messages to have in a log file even when progress display is 
> > filtered out.  So the sideband demultiplexing is useful in that case as 
> > well.
> 
> In that case, maybe we should define a separate sideband for
> progress display?  Currently #1 (payload) and #3 (emergency
> exit) are distinct but #2 corresponds to stderr which has info
> messages and progress noise all mixed up.

And how would the remote end distinguish how to split the info from the 
progress noise?  Teaching pack-object about another file descriptor?
And what happen if pack-object is run locally in that case?

How does that solve the issue with servers (currently all of them) that 
don't know about the new sideband?

I think life might just be so much simpler if we just decide right now 
that progress noise is defined as a string ending witha '\r', and that 
we just toss it locally when we don't want progress noise.  As a bonus 
this is even fully backward compatible with all existing servers.


Nicolas

Re: [PATCH] Fixup no-progress for fetch & clone

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> And how would the remote end distinguish how to split the info from the 
> progress noise?  Teaching pack-object about another file descriptor?
> And what happen if pack-object is run locally in that case?

That's true, and your convention to classify line with "\r"
noise and others as info is as good as anything, and does not
break any existing system, so it is very good one.

> How does that solve the issue with servers (currently all of them) that 
> don't know about the new sideband?

I actually happen to consider that a non problem.

It is true that you can only prepare the client and server side
by 1.5.1, and new clients talking to 1.5.0 or older servers
would not be able to squelch the noise.  But that is _not_ a new
problem anyway, and given time the server side will eventually
be updated to 1.5.1 or later.  If anything else, it would give
incentive to people to upgrade their servers to 1.5.1 ;-)

Re: [PATCH] Fixup no-progress for fetch & clone

[Nicolas Pitre]

On Sat, 24 Feb 2007, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> writes:
> 
> > How does that solve the issue with servers (currently all of them) that 
> > don't know about the new sideband?
> 
> I actually happen to consider that a non problem.
> 
> It is true that you can only prepare the client and server side
> by 1.5.1, and new clients talking to 1.5.0 or older servers
> would not be able to squelch the noise.  But that is _not_ a new
> problem anyway, and given time the server side will eventually
> be updated to 1.5.1 or later.  If anything else, it would give
> incentive to people to upgrade their servers to 1.5.1 ;-)

Well... for such a special case feature I doubt this might be such a 
great insentive.  And since locally filtering on '\r' is so simple I 
wonder what the complexity of the alternative solution really buys us.


Nicolas



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


Nicolas

Re: [PATCH] Fixup no-progress for fetch & clone

[Jakub Narebski]

Nicolas Pitre wrote:

> On Sat, 24 Feb 2007, Junio C Hamano wrote:
> 
>> Nicolas Pitre <nico@cam.org> writes:
>> 
>>> How does that solve the issue with servers (currently all of them) that 
>>> don't know about the new sideband?
>> 
>> I actually happen to consider that a non problem.
>> 
>> It is true that you can only prepare the client and server side
>> by 1.5.1, and new clients talking to 1.5.0 or older servers
>> would not be able to squelch the noise.  But that is _not_ a new
>> problem anyway, and given time the server side will eventually
>> be updated to 1.5.1 or later.  If anything else, it would give
>> incentive to people to upgrade their servers to 1.5.1 ;-)
> 
> Well... for such a special case feature I doubt this might be such a 
> great insentive.  And since locally filtering on '\r' is so simple I 
> wonder what the complexity of the alternative solution really buys us.

Slighly less bandwidth used (we don't send progress report)?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git