All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] daemon: send stderr of service programs to the syslog
@ 2009-06-14 20:38 Johannes Sixt
  2009-06-15 14:57 ` Shawn O. Pearce
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Johannes Sixt @ 2009-06-14 20:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, H. Peter Anvin

If git-daemon is run with --detach or --inetd, then stderr is explicitly
redirected to /dev/null. But notice that the service programs were spawned
via execl_git_cmd(), in particular, the stderr channel is inherited from
the daemon. This means that errors that the programs wrote to stderr (for
example, via die()), went to /dev/null.

This patch arranges that the daemon does not merely exec the service
program, but forks it and monitors stderr of the child; it writes the
errors that it produces to the daemons log via logerror().

A consequence is that the daemon process remains in memory for the full
duration of the service program, but this cannot be avoided.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 I don't know whether service programs like upload-archive or upload-pack
 write progress report to stderr or not, for example, if a client does not
 support side-bands. In this case this patch is probably not enough since
 this would fill the log with unneeded progress information. Any hints
 are appreciated.

 I intend to follow-up this patch with another one that integrates
 run_service_command() in execute() in order to streamline how the
 'incoming' fd is inherited to the service programs. But I haven't
 even begun this change, yet, because I'd like to know first how this
 one is received.

 -- Hannes

 daemon.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/daemon.c b/daemon.c
index d7ceca4..3e1a354 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pkt-line.h"
-#include "exec_cmd.h"
+#include "run-command.h"
+#include "strbuf.h"
 
 #include <syslog.h>
 
@@ -343,28 +344,66 @@ static int run_service(char *dir, const struct daemon_service *service)
 	return service->fn();
 }
 
+static void copy_to_log(int fd)
+{
+	struct strbuf line = STRBUF_INIT;
+	FILE *fp;
+
+	fp = fdopen(fd, "r");
+	if (fp == NULL) {
+		logerror("fdopen of error channel failed");
+		close(fd);
+		return;
+	}
+
+	while (strbuf_getline(&line, fp, '\n') != EOF) {
+		logerror("%s", line.buf);
+		strbuf_setlen(&line, 0);
+	}
+	
+	strbuf_release(&line);
+	fclose(fp);
+}
+
+static int run_service_command(const char **argv)
+{
+	struct child_process cld;
+
+	memset(&cld, 0, sizeof(cld));
+	cld.argv = argv;
+	cld.git_cmd = 1;
+	cld.err = -1;
+	if (start_command(&cld))
+		return -1;
+
+	close(0);
+	close(1);
+
+	copy_to_log(cld.err);
+
+	return finish_command(&cld);
+}
+
 static int upload_pack(void)
 {
 	/* Timeout as string */
 	char timeout_buf[64];
+	const char *argv[] = { "upload-pack", "--strict", timeout_buf, ".", NULL };
 
 	snprintf(timeout_buf, sizeof timeout_buf, "--timeout=%u", timeout);
-
-	/* git-upload-pack only ever reads stuff, so this is safe */
-	execl_git_cmd("upload-pack", "--strict", timeout_buf, ".", NULL);
-	return -1;
+	return run_service_command(argv);
 }
 
 static int upload_archive(void)
 {
-	execl_git_cmd("upload-archive", ".", NULL);
-	return -1;
+	static const char *argv[] = { "upload-archive", ".", NULL };
+	return run_service_command(argv);
 }
 
 static int receive_pack(void)
 {
-	execl_git_cmd("receive-pack", ".", NULL);
-	return -1;
+	static const char *argv[] = { "receive-pack", ".", NULL };
+	return run_service_command(argv);
 }
 
 static struct daemon_service daemon_service[] = {
-- 
1.6.3.17.g1665f

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-14 20:38 [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
@ 2009-06-15 14:57 ` Shawn O. Pearce
  2009-06-15 15:34   ` Nicolas Pitre
                     ` (2 more replies)
  2009-06-15 21:39 ` [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
  2009-06-19 20:26 ` Junio C Hamano
  2 siblings, 3 replies; 18+ messages in thread
From: Shawn O. Pearce @ 2009-06-15 14:57 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git, H. Peter Anvin

Johannes Sixt <j6t@kdbg.org> wrote:
> If git-daemon is run with --detach or --inetd, then stderr is explicitly
> redirected to /dev/null. But notice that the service programs were spawned
> via execl_git_cmd(), in particular, the stderr channel is inherited from
> the daemon. This means that errors that the programs wrote to stderr (for
> example, via die()), went to /dev/null.
> 
> This patch arranges that the daemon does not merely exec the service
> program, but forks it and monitors stderr of the child; it writes the
> errors that it produces to the daemons log via logerror().
> 
> A consequence is that the daemon process remains in memory for the full
> duration of the service program, but this cannot be avoided.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  I don't know whether service programs like upload-archive or upload-pack
>  write progress report to stderr or not, for example, if a client does not
>  support side-bands. In this case this patch is probably not enough since
>  this would fill the log with unneeded progress information. Any hints
>  are appreciated.

They could, if they were broken.  :-)

IIRC only upload-pack produces progress (from pack-objects).
It does so by using a pipe on fd 2, and either copying it down
to the client via side-band, or discarding it.  So progress data
shouldn't ever appear on upload-pack's own fd 2, which means you
won't get it in this syslog thing.

But I have to wonder, why are we doing this?  Why can't we teach the
individual server program to record its error to the syslog before
it aborts?  Are we looking for SIGSEGV or something?  Its only the
daemon program staying around in memory, but that's a lot of little
daemons doing nothing waiting for their children to terminate.
Seems like a waste to me.
 
-- 
Shawn.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-15 14:57 ` Shawn O. Pearce
@ 2009-06-15 15:34   ` Nicolas Pitre
  2009-06-15 19:06   ` H. Peter Anvin
  2009-06-15 21:24   ` [PATCH] upload-pack: squelch progress indicator if client does not request sideband Johannes Sixt
  2 siblings, 0 replies; 18+ messages in thread
From: Nicolas Pitre @ 2009-06-15 15:34 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Johannes Sixt, Junio C Hamano, git, H. Peter Anvin

On Mon, 15 Jun 2009, Shawn O. Pearce wrote:

> But I have to wonder, why are we doing this?  Why can't we teach the
> individual server program to record its error to the syslog before
> it aborts?  Are we looking for SIGSEGV or something?  Its only the
> daemon program staying around in memory, but that's a lot of little
> daemons doing nothing waiting for their children to terminate.

Those daemons certainly have very little in terms of private data memory 
usage, so all of them will share the same memory pages in practice. And 
it is certainly best to have only one location to deal with syslog usage 
than having each server programs to do it, after figuring out if they 
are actually invoked in a context that requires syslog instead of 
stderr.  Having the git daemon deal with syslog, and log any abnormal 
exit code from the programs it spawn is IMHO a pretty logical thing to 
do.


Nicolas

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-15 14:57 ` Shawn O. Pearce
  2009-06-15 15:34   ` Nicolas Pitre
@ 2009-06-15 19:06   ` H. Peter Anvin
  2009-06-15 21:24   ` [PATCH] upload-pack: squelch progress indicator if client does not request sideband Johannes Sixt
  2 siblings, 0 replies; 18+ messages in thread
From: H. Peter Anvin @ 2009-06-15 19:06 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Johannes Sixt, Junio C Hamano, git

Shawn O. Pearce wrote:
> 
> They could, if they were broken.  :-)
> 
> IIRC only upload-pack produces progress (from pack-objects).
> It does so by using a pipe on fd 2, and either copying it down
> to the client via side-band, or discarding it.  So progress data
> shouldn't ever appear on upload-pack's own fd 2, which means you
> won't get it in this syslog thing.
> 
> But I have to wonder, why are we doing this?  Why can't we teach the
> individual server program to record its error to the syslog before
> it aborts?  Are we looking for SIGSEGV or something?  Its only the
> daemon program staying around in memory, but that's a lot of little
> daemons doing nothing waiting for their children to terminate.
> Seems like a waste to me.
>  

Actually, even logging signals would be useful, so I think this makes
sense.  The daemon process is pretty trivial compared to the rest of the
stuff being spawned.

	-hpa

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-15 14:57 ` Shawn O. Pearce
  2009-06-15 15:34   ` Nicolas Pitre
  2009-06-15 19:06   ` H. Peter Anvin
@ 2009-06-15 21:24   ` Johannes Sixt
  2009-06-15 21:53     ` Junio C Hamano
  2 siblings, 1 reply; 18+ messages in thread
From: Johannes Sixt @ 2009-06-15 21:24 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git, H. Peter Anvin

upload-pack runs pack-objects, which generates progress indicator output
on its stderr. If the client requests a sideband, this indicator is sent
to the client; but if it did not, then the progress is written to
upload-pack's own stderr.

Since the previous patch git-daemon monitors stderr of the service program,
such as upload-pack, and copies it to the syslog. This would now also copy
the progress indicator to the syslog. We avoid this by calling pack-objects
without --progress if there is no sideband channel to the client.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
On Montag, 15. Juni 2009, Shawn O. Pearce wrote:
> IIRC only upload-pack produces progress (from pack-objects).
> It does so by using a pipe on fd 2, and either copying it down
> to the client via side-band, or discarding it.  So progress data
> shouldn't ever appear on upload-pack's own fd 2, which means you
> won't get it in this syslog thing.

Unfortunately, upload-pack *does* write the progress to fd 2,
and this fixes it.

-- Hannes

 upload-pack.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index edc7861..fef8be5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -521,6 +521,15 @@ static void receive_needs(void)
 	}
 	if (debug_fd)
 		write_in_full(debug_fd, "#E\n", 3);
+
+	/*
+	 * If upload-pack is run from the daemon and the client did not
+	 * request a sideband, the progress output produced by pack-objects
+	 * would go to the syslog. Squelch it.
+	 */
+	if (!use_sideband)
+		no_progress = 1;
+
 	if (depth == 0 && shallows.nr == 0)
 		return;
 	if (depth > 0) {
-- 
1.6.3.17.g1665f

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-14 20:38 [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
  2009-06-15 14:57 ` Shawn O. Pearce
@ 2009-06-15 21:39 ` Johannes Sixt
  2009-06-15 21:43   ` H. Peter Anvin
  2009-06-19 20:26 ` Junio C Hamano
  2 siblings, 1 reply; 18+ messages in thread
From: Johannes Sixt @ 2009-06-15 21:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, H. Peter Anvin

On Sonntag, 14. Juni 2009, Johannes Sixt wrote:
>  I don't know whether service programs like upload-archive or upload-pack
>  write progress report to stderr or not, for example, if a client does not
>  support side-bands. In this case this patch is probably not enough since
>  this would fill the log with unneeded progress information. Any hints
>  are appreciated.

The progress indicator can be helped . But there is now another anoyance: If 
the client terminates the connection early, this is now logged as:

fatal: unable to run 'git-upload-pack'

The reason for this is that upload-pack is run as 'git upload-pack', which 
itself spawns the external 'git-upload-pack'. The latter dies from a SIGPIPE, 
and the former, in execv_dashed_external(), dutyfully writes this down.

The easiest solution is perhaps to make upload-pack a builtin.

BUT... The motivation, of which this patch is actually a fall-out, is to clean 
up the messy error behavor of the start,finish,run_command family. To take 
care of this error message is just one more (hopefully small) point on my 
agenda.

>  I intend to follow-up this patch with another one that integrates
>  run_service_command() in execute() in order to streamline how the
>  'incoming' fd is inherited to the service programs.

I'm not sure anymore whether the change I planned here is worth it. When I 
wrote this announcement, I had mis-remembered how daemon.c's handle() and 
execute() functions and --inetd mode interact.

-- Hannes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-15 21:39 ` [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
@ 2009-06-15 21:43   ` H. Peter Anvin
  2009-06-16 20:27     ` Johannes Sixt
  0 siblings, 1 reply; 18+ messages in thread
From: H. Peter Anvin @ 2009-06-15 21:43 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git

Johannes Sixt wrote:
> On Sonntag, 14. Juni 2009, Johannes Sixt wrote:
>>  I don't know whether service programs like upload-archive or upload-pack
>>  write progress report to stderr or not, for example, if a client does not
>>  support side-bands. In this case this patch is probably not enough since
>>  this would fill the log with unneeded progress information. Any hints
>>  are appreciated.
> 
> The progress indicator can be helped . But there is now another anoyance: If 
> the client terminates the connection early, this is now logged as:
> 
> fatal: unable to run 'git-upload-pack'
> 
> The reason for this is that upload-pack is run as 'git upload-pack', which 
> itself spawns the external 'git-upload-pack'. The latter dies from a SIGPIPE, 
> and the former, in execv_dashed_external(), dutyfully writes this down.
> 
> The easiest solution is perhaps to make upload-pack a builtin.
> 
> BUT... The motivation, of which this patch is actually a fall-out, is to clean 
> up the messy error behavor of the start,finish,run_command family. To take 
> care of this error message is just one more (hopefully small) point on my 
> agenda.
> 

We probably do want to log that the client has disconnected.

	-hpa

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-15 21:24   ` [PATCH] upload-pack: squelch progress indicator if client does not request sideband Johannes Sixt
@ 2009-06-15 21:53     ` Junio C Hamano
  2009-06-16  1:16       ` Nicolas Pitre
  0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2009-06-15 21:53 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Shawn O. Pearce, git, H. Peter Anvin

Johannes Sixt <j6t@kdbg.org> writes:

> Since the previous patch git-daemon monitors stderr of the service program,
> such as upload-pack, and copies it to the syslog. This would now also copy
> the progress indicator to the syslog. We avoid this by calling pack-objects
> without --progress if there is no sideband channel to the client.
> ...
> +
> +	/*
> +	 * If upload-pack is run from the daemon and the client did not
> +	 * request a sideband, the progress output produced by pack-objects
> +	 * would go to the syslog. Squelch it.
> +	 */
> +	if (!use_sideband)
> +		no_progress = 1;
> +

I think it is a very good idea to squelch progress output that will never
go to the client (it will be wasted traffic, regardless of the "syslog"
thing), but

 (1) Is "not using sideband" the same as "client won't see the progress
     output" for all vintages of clients that work with the current
     server?

     How did we drive upload-pack over native or ssh connection before we
     introduced sideband?  I vaguely recall that we relied on stderr going
     to the invoking terminal in the local case.  With this change, does
     the user suddenly stop seeing progress if the client is older than
     583b7ea (upload-pack/fetch-pack: support side-band communication,
     2006-06-21)?  If so, that would be a regression.

 (2) The change in _this_ patch may be a good thing independent from the
     change to the daemon, and I would hate to see it justified in terms
     of that other change.  This comment applies to the proposed commit
     log message as well.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-15 21:53     ` Junio C Hamano
@ 2009-06-16  1:16       ` Nicolas Pitre
  2009-06-16  5:02         ` Junio C Hamano
  0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Pitre @ 2009-06-16  1:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Shawn O. Pearce, git, H. Peter Anvin

On Mon, 15 Jun 2009, Junio C Hamano wrote:

> I think it is a very good idea to squelch progress output that will never
> go to the client (it will be wasted traffic, regardless of the "syslog"
> thing), but
> 
>  (1) Is "not using sideband" the same as "client won't see the progress
>      output" for all vintages of clients that work with the current
>      server?
> 
>      How did we drive upload-pack over native or ssh connection before we
>      introduced sideband?  I vaguely recall that we relied on stderr going
>      to the invoking terminal in the local case.  With this change, does
>      the user suddenly stop seeing progress if the client is older than
>      583b7ea (upload-pack/fetch-pack: support side-band communication,
>      2006-06-21)?  If so, that would be a regression.

Native, or git:// style, certainly never was able to get any kind of 
progress display without sideband support.

As to old clients using ssh... well... that could be a regression.  But 
do we really care?  This is pre v1.4.1 after all, and even the previous 
Debian stable was using a later git version.


Nicolas

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-16  1:16       ` Nicolas Pitre
@ 2009-06-16  5:02         ` Junio C Hamano
  2009-06-16 18:03           ` Nicolas Pitre
  0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2009-06-16  5:02 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Johannes Sixt, Shawn O. Pearce, git, H. Peter Anvin

Nicolas Pitre <nico@cam.org> writes:

>>      How did we drive upload-pack over native or ssh connection before we
>>      introduced sideband?  I vaguely recall that we relied on stderr going
>>      to the invoking terminal in the local case.  With this change, does
>>      the user suddenly stop seeing progress if the client is older than
>>      583b7ea (upload-pack/fetch-pack: support side-band communication,
>>      2006-06-21)?  If so, that would be a regression.
>
> Native, or git:// style, certainly never was able to get any kind of 
> progress display without sideband support.
>
> As to old clients using ssh... well... that could be a regression.  But 
> do we really care?  This is pre v1.4.1 after all, and even the previous 
> Debian stable was using a later git version.

I guess both ssh and local would regress, and we do care about regressions
in general, but I think it is not a grave offense in this case, because
the combination between such an old fetch and the updated upload-pack will
still transfer the objects and will update the refs as expected; only the
progress indication and chatter on the stderr stream will be lost.

So I would say it is Ok to regress in this particular case, _if_ there is
no easy way to avoid it.  That was why I asked:

>> I think it is a very good idea to squelch progress output that will never
>> go to the client (it will be wasted traffic, regardless of the "syslog"
>> thing), but
>> 
>>  (1) Is "not using sideband" the same as "client won't see the progress
>>      output" for all vintages of clients that work with the current
>>      server?

Stated differently, I think "not using sideband _and_ spawned via daemon"
would be an indication that "the client won't see the progress anyway even
if it were sent."  So the question becomes "will it be a small enough
change to detect if the upload-pack is driven by the daemon in the
codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do so?"

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-16  5:02         ` Junio C Hamano
@ 2009-06-16 18:03           ` Nicolas Pitre
  2009-06-16 18:41             ` Johannes Sixt
  0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Pitre @ 2009-06-16 18:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Shawn O. Pearce, git, H. Peter Anvin

On Mon, 15 Jun 2009, Junio C Hamano wrote:

> I guess both ssh and local would regress, and we do care about regressions
> in general, but I think it is not a grave offense in this case, because
> the combination between such an old fetch and the updated upload-pack will
> still transfer the objects and will update the refs as expected; only the
> progress indication and chatter on the stderr stream will be lost.

My point exactly, although I realize I didn't make myself clear.

> So I would say it is Ok to regress in this particular case, _if_ 
there is
> no easy way to avoid it.  That was why I asked:
> 
> >> I think it is a very good idea to squelch progress output that will never
> >> go to the client (it will be wasted traffic, regardless of the "syslog"
> >> thing), but
> >> 
> >>  (1) Is "not using sideband" the same as "client won't see the progress
> >>      output" for all vintages of clients that work with the current
> >>      server?
> 
> Stated differently, I think "not using sideband _and_ spawned via daemon"
> would be an indication that "the client won't see the progress anyway even
> if it were sent."  So the question becomes "will it be a small enough
> change to detect if the upload-pack is driven by the daemon in the
> codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do so?"

I don't think it is worth it at all.  The regression is purely cosmetic, 
and I suspect you'll have a really hard time finding someone still using 
those ancient git clients anyway.  Remember that such clients are unable 
to fetch with HTTP from repositories using version 2 of the pack index 
by default already.  That's why we created version 1.4.4.5.


Nicolas

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-16 18:03           ` Nicolas Pitre
@ 2009-06-16 18:41             ` Johannes Sixt
  2009-06-16 18:57               ` Junio C Hamano
  2009-06-16 19:46               ` Nicolas Pitre
  0 siblings, 2 replies; 18+ messages in thread
From: Johannes Sixt @ 2009-06-16 18:41 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: Junio C Hamano, Shawn O. Pearce, git, H. Peter Anvin

On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:
> On Mon, 15 Jun 2009, Junio C Hamano wrote:
> > Stated differently, I think "not using sideband _and_ spawned via daemon"
> > would be an indication that "the client won't see the progress anyway
> > even if it were sent."  So the question becomes "will it be a small
> > enough change to detect if the upload-pack is driven by the daemon in the
> > codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do
> > so?"
>
> I don't think it is worth it at all.  The regression is purely cosmetic,
> and I suspect you'll have a really hard time finding someone still using
> those ancient git clients anyway.  Remember that such clients are unable
> to fetch with HTTP from repositories using version 2 of the pack index
> by default already.  That's why we created version 1.4.4.5.

Keep in mind that there could exist clients outside git.git that use modern
pack-index or unpack-objects, but omit sideband support.

I propose this patch instead, although the assumption that the --timeout
option is only used by git-daemon may not be quite right. In this case it
may be better to add a new option that sets the new flag and that
git-daemon has to use.

-- Hannes

Subject: [PATCH] upload-pack: squelch progress indicator if client cannot see it

upload-pack runs pack-objects, which generates progress indicator output
on its stderr. If the client requests a sideband, this indicator is sent
to the client; but if it did not, then the progress is written to
upload-pack's own stderr.

If upload-pack is itself run from git-daemon (and if the client did not
request a sideband) the progress indicator never reaches the client and it
need not be generated in the first place. With this patch the progress
indicator is suppressed in this situation.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 upload-pack.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index edc7861..841ebb5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -28,7 +28,7 @@ static unsigned long oldest_have;
 
 static int multi_ack, nr_our_refs;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
-static int no_progress;
+static int no_progress, daemon_mode;
 static struct object_array have_obj;
 static struct object_array want_obj;
 static unsigned int timeout;
@@ -521,6 +521,10 @@ static void receive_needs(void)
 	}
 	if (debug_fd)
 		write_in_full(debug_fd, "#E\n", 3);
+
+	if (!use_sideband && daemon_mode)
+		no_progress = 1;
+
 	if (depth == 0 && shallows.nr == 0)
 		return;
 	if (depth > 0) {
@@ -630,6 +634,7 @@ int main(int argc, char **argv)
 		}
 		if (!prefixcmp(arg, "--timeout=")) {
 			timeout = atoi(arg+10);
+			daemon_mode = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--")) {
-- 
1.6.3.17.g1665f

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-16 18:41             ` Johannes Sixt
@ 2009-06-16 18:57               ` Junio C Hamano
  2009-06-16 19:46               ` Nicolas Pitre
  1 sibling, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2009-06-16 18:57 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Nicolas Pitre, Shawn O. Pearce, git, H. Peter Anvin

Johannes Sixt <j6t@kdbg.org> writes:

> On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:
> ...
>> I don't think it is worth it at all.  The regression is purely cosmetic,
>> and I suspect you'll have a really hard time finding someone still using
>> those ancient git clients anyway.  Remember that such clients are unable
>> to fetch with HTTP from repositories using version 2 of the pack index
>> by default already.  That's why we created version 1.4.4.5.
>
> Keep in mind that there could exist clients outside git.git that use modern
> pack-index or unpack-objects, but omit sideband support.
>
> I propose this patch instead, although the assumption that the --timeout
> option is only used by git-daemon may not be quite right. In this case it
> may be better to add a new option that sets the new flag and that
> git-daemon has to use.

Even though I tend to agree with Nico, this patch strikes a good balance
with a very small impact.  I am beginning to like it.

There _could_ be people who run fetch over git/ssh with custom --exec
option that has --timeout in it.  We _could_ teach --run-by-daemon option
to the (daemon, upload-pack) pair in the same release (because it is
reasonable to assume they come from the same vintage of git) to fix that
case.  But I do not think it is worth it.  As Nico says, this is purely
cosmetic.

Thanks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband
  2009-06-16 18:41             ` Johannes Sixt
  2009-06-16 18:57               ` Junio C Hamano
@ 2009-06-16 19:46               ` Nicolas Pitre
  1 sibling, 0 replies; 18+ messages in thread
From: Nicolas Pitre @ 2009-06-16 19:46 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, Shawn O. Pearce, git, H. Peter Anvin

On Tue, 16 Jun 2009, Johannes Sixt wrote:

> On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:
> > On Mon, 15 Jun 2009, Junio C Hamano wrote:
> > > Stated differently, I think "not using sideband _and_ spawned via daemon"
> > > would be an indication that "the client won't see the progress anyway
> > > even if it were sent."  So the question becomes "will it be a small
> > > enough change to detect if the upload-pack is driven by the daemon in the
> > > codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do
> > > so?"
> >
> > I don't think it is worth it at all.  The regression is purely cosmetic,
> > and I suspect you'll have a really hard time finding someone still using
> > those ancient git clients anyway.  Remember that such clients are unable
> > to fetch with HTTP from repositories using version 2 of the pack index
> > by default already.  That's why we created version 1.4.4.5.
> 
> Keep in mind that there could exist clients outside git.git that use modern
> pack-index or unpack-objects, but omit sideband support.

And that would be an abominable thing to make special support for such 
clients.  If they're fancy enough to want progress display, they should 
comply with what moderately recent native git clients do and get 
sideband support.


Nicolas

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-15 21:43   ` H. Peter Anvin
@ 2009-06-16 20:27     ` Johannes Sixt
  2009-06-16 20:30       ` H. Peter Anvin
  0 siblings, 1 reply; 18+ messages in thread
From: Johannes Sixt @ 2009-06-16 20:27 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Junio C Hamano, git

On Montag, 15. Juni 2009, H. Peter Anvin wrote:
> Johannes Sixt wrote:
> > The progress indicator can be helped . But there is now another anoyance:
> > If the client terminates the connection early, this is now logged as:
> >
> > fatal: unable to run 'git-upload-pack'
>
> We probably do want to log that the client has disconnected.

Any client, or only clients that disconnect early? Is this useful besides 
debugging?

I think git-daemon's --verbose is intended to log all client connects and 
disconnects. This here is about a *new* message that would be logged 
without --verbose.

-- Hannes

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-16 20:27     ` Johannes Sixt
@ 2009-06-16 20:30       ` H. Peter Anvin
  0 siblings, 0 replies; 18+ messages in thread
From: H. Peter Anvin @ 2009-06-16 20:30 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git

Johannes Sixt wrote:
> On Montag, 15. Juni 2009, H. Peter Anvin wrote:
>> Johannes Sixt wrote:
>>> The progress indicator can be helped . But there is now another anoyance:
>>> If the client terminates the connection early, this is now logged as:
>>>
>>> fatal: unable to run 'git-upload-pack'
>> We probably do want to log that the client has disconnected.
> 
> Any client, or only clients that disconnect early? Is this useful besides 
> debugging?
> 

Clients that disconnect without completing the transaction.  This is
highly valuable to administrators when tracking down a problem (since it
generally implies a client or network problem, as opposed to a server
problem.)

	-hpa

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-14 20:38 [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
  2009-06-15 14:57 ` Shawn O. Pearce
  2009-06-15 21:39 ` [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
@ 2009-06-19 20:26 ` Junio C Hamano
  2009-06-20 19:25   ` Johannes Sixt
  2 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2009-06-19 20:26 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, git, H. Peter Anvin

Johannes Sixt <j6t@kdbg.org> writes:

> diff --git a/daemon.c b/daemon.c
> index d7ceca4..3e1a354 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "pkt-line.h"
> -#include "exec_cmd.h"
> +#include "run-command.h"
> +#include "strbuf.h"

    CC daemon.o
cc1: warnings being treated as errors
daemon.c: In function 'main':
daemon.c:981: error: implicit declaration of function 'git_extract_argv0_path'
make: *** [daemon.o] Error 1

I'll add the include back in the meantime.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] daemon: send stderr of service programs to the syslog
  2009-06-19 20:26 ` Junio C Hamano
@ 2009-06-20 19:25   ` Johannes Sixt
  0 siblings, 0 replies; 18+ messages in thread
From: Johannes Sixt @ 2009-06-20 19:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, H. Peter Anvin

On Freitag, 19. Juni 2009, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> > diff --git a/daemon.c b/daemon.c
> > index d7ceca4..3e1a354 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1,6 +1,7 @@
> >  #include "cache.h"
> >  #include "pkt-line.h"
> > -#include "exec_cmd.h"
> > +#include "run-command.h"
> > +#include "strbuf.h"
>
>     CC daemon.o
> cc1: warnings being treated as errors
> daemon.c: In function 'main':
> daemon.c:981: error: implicit declaration of function
> 'git_extract_argv0_path' make: *** [daemon.o] Error 1
>
> I'll add the include back in the meantime.

Oops, you are right! Thanks for noticing it. Silly me: I didn't have -Wall in 
my CFLAGS.

-- Hannes

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2009-06-20 19:25 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-14 20:38 [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
2009-06-15 14:57 ` Shawn O. Pearce
2009-06-15 15:34   ` Nicolas Pitre
2009-06-15 19:06   ` H. Peter Anvin
2009-06-15 21:24   ` [PATCH] upload-pack: squelch progress indicator if client does not request sideband Johannes Sixt
2009-06-15 21:53     ` Junio C Hamano
2009-06-16  1:16       ` Nicolas Pitre
2009-06-16  5:02         ` Junio C Hamano
2009-06-16 18:03           ` Nicolas Pitre
2009-06-16 18:41             ` Johannes Sixt
2009-06-16 18:57               ` Junio C Hamano
2009-06-16 19:46               ` Nicolas Pitre
2009-06-15 21:39 ` [PATCH] daemon: send stderr of service programs to the syslog Johannes Sixt
2009-06-15 21:43   ` H. Peter Anvin
2009-06-16 20:27     ` Johannes Sixt
2009-06-16 20:30       ` H. Peter Anvin
2009-06-19 20:26 ` Junio C Hamano
2009-06-20 19:25   ` Johannes Sixt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.