{"thread":{"id":"19813","subject":"[PATCH] daemon: send stderr of service programs to the syslog","startedAt":"2009-06-14T20:38:51Z","lastAt":"2009-06-20T19:25:10Z","messageCount":18,"participants":["Johannes Sixt","Shawn O. Pearce","Nicolas Pitre","H. Peter Anvin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"116292","messageId":"200906142238.51725.j6t@kdbg.org","threadId":"19813","inReplyTo":null,"subject":"[PATCH] daemon: send stderr of service programs to the syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-14T20:38:51Z","receivedAt":"2009-06-14T20:38:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"If git-daemon is run with --detach or --inetd, then stderr is explicitly\nredirected to /dev/null. But notice that the service programs were spawned\nvia execl_git_cmd(), in particular, the stderr channel is inherited from\nthe daemon. This means that errors that the programs wrote to stderr (for\nexample, via die()), went to /dev/null.\n\nThis patch arranges that the daemon does not merely exec the service\nprogram, but forks it and monitors stderr of the child; it writes the\nerrors that it produces to the daemons log via logerror().\n\nA consequence is that the daemon process remains in memory for the full\nduration of the service program, but this cannot be avoided.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n I don't know whether service programs like upload-archive or upload-pack\n write progress report to stderr or not, for example, if a client does not\n support side-bands. In this case this patch is probably not enough since\n this would fill the log with unneeded progress information. Any hints\n are appreciated.\n\n I intend to follow-up this patch with another one that integrates\n run_service_command() in execute() in order to streamline how the\n 'incoming' fd is inherited to the service programs. But I haven't\n even begun this change, yet, because I'd like to know first how this\n one is received.\n\n -- Hannes\n\n daemon.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 48 insertions(+), 9 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex d7ceca4..3e1a354 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"pkt-line.h\"\n-#include \"exec_cmd.h\"\n+#include \"run-command.h\"\n+#include \"strbuf.h\"\n \n #include <syslog.h>\n \n@@ -343,28 +344,66 @@ static int run_service(char *dir, const struct daemon_service *service)\n \treturn service->fn();\n }\n \n+static void copy_to_log(int fd)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tFILE *fp;\n+\n+\tfp = fdopen(fd, \"r\");\n+\tif (fp == NULL) {\n+\t\tlogerror(\"fdopen of error channel failed\");\n+\t\tclose(fd);\n+\t\treturn;\n+\t}\n+\n+\twhile (strbuf_getline(&line, fp, '\\n') != EOF) {\n+\t\tlogerror(\"%s\", line.buf);\n+\t\tstrbuf_setlen(&line, 0);\n+\t}\n+\t\n+\tstrbuf_release(&line);\n+\tfclose(fp);\n+}\n+\n+static int run_service_command(const char **argv)\n+{\n+\tstruct child_process cld;\n+\n+\tmemset(&cld, 0, sizeof(cld));\n+\tcld.argv = argv;\n+\tcld.git_cmd = 1;\n+\tcld.err = -1;\n+\tif (start_command(&cld))\n+\t\treturn -1;\n+\n+\tclose(0);\n+\tclose(1);\n+\n+\tcopy_to_log(cld.err);\n+\n+\treturn finish_command(&cld);\n+}\n+\n static int upload_pack(void)\n {\n \t/* Timeout as string */\n \tchar timeout_buf[64];\n+\tconst char *argv[] = { \"upload-pack\", \"--strict\", timeout_buf, \".\", NULL };\n \n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n-\n-\t/* git-upload-pack only ever reads stuff, so this is safe */\n-\texecl_git_cmd(\"upload-pack\", \"--strict\", timeout_buf, \".\", NULL);\n-\treturn -1;\n+\treturn run_service_command(argv);\n }\n \n static int upload_archive(void)\n {\n-\texecl_git_cmd(\"upload-archive\", \".\", NULL);\n-\treturn -1;\n+\tstatic const char *argv[] = { \"upload-archive\", \".\", NULL };\n+\treturn run_service_command(argv);\n }\n \n static int receive_pack(void)\n {\n-\texecl_git_cmd(\"receive-pack\", \".\", NULL);\n-\treturn -1;\n+\tstatic const char *argv[] = { \"receive-pack\", \".\", NULL };\n+\treturn run_service_command(argv);\n }\n \n static struct daemon_service daemon_service[] = {\n-- \n1.6.3.17.g1665f\n"},{"id":"116341","messageId":"20090615145716.GW16497@spearce.org","threadId":"19813","inReplyTo":"200906142238.51725.j6t@kdbg.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-06-15T14:57:16Z","receivedAt":"2009-06-15T14:57:16Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> wrote:\n> If git-daemon is run with --detach or --inetd, then stderr is explicitly\n> redirected to /dev/null. But notice that the service programs were spawned\n> via execl_git_cmd(), in particular, the stderr channel is inherited from\n> the daemon. This means that errors that the programs wrote to stderr (for\n> example, via die()), went to /dev/null.\n> \n> This patch arranges that the daemon does not merely exec the service\n> program, but forks it and monitors stderr of the child; it writes the\n> errors that it produces to the daemons log via logerror().\n> \n> A consequence is that the daemon process remains in memory for the full\n> duration of the service program, but this cannot be avoided.\n> \n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  I don't know whether service programs like upload-archive or upload-pack\n>  write progress report to stderr or not, for example, if a client does not\n>  support side-bands. In this case this patch is probably not enough since\n>  this would fill the log with unneeded progress information. Any hints\n>  are appreciated.\n\nThey could, if they were broken.  :-)\n\nIIRC only upload-pack produces progress (from pack-objects).\nIt does so by using a pipe on fd 2, and either copying it down\nto the client via side-band, or discarding it.  So progress data\nshouldn't ever appear on upload-pack's own fd 2, which means you\nwon't get it in this syslog thing.\n\nBut I have to wonder, why are we doing this?  Why can't we teach the\nindividual server program to record its error to the syslog before\nit aborts?  Are we looking for SIGSEGV or something?  Its only the\ndaemon program staying around in memory, but that's a lot of little\ndaemons doing nothing waiting for their children to terminate.\nSeems like a waste to me.\n \n-- \nShawn.\n"},{"id":"116345","messageId":"alpine.LFD.2.00.0906151117560.16467@xanadu.home","threadId":"19813","inReplyTo":"20090615145716.GW16497@spearce.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-15T15:34:15Z","receivedAt":"2009-06-15T15:34:15Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 15 Jun 2009, Shawn O. Pearce wrote:\n\n> But I have to wonder, why are we doing this?  Why can't we teach the\n> individual server program to record its error to the syslog before\n> it aborts?  Are we looking for SIGSEGV or something?  Its only the\n> daemon program staying around in memory, but that's a lot of little\n> daemons doing nothing waiting for their children to terminate.\n\nThose daemons certainly have very little in terms of private data memory \nusage, so all of them will share the same memory pages in practice. And \nit is certainly best to have only one location to deal with syslog usage \nthan having each server programs to do it, after figuring out if they \nare actually invoked in a context that requires syslog instead of \nstderr.  Having the git daemon deal with syslog, and log any abnormal \nexit code from the programs it spawn is IMHO a pretty logical thing to \ndo.\n\n\nNicolas\n"},{"id":"116351","messageId":"4A369B99.4030504@zytor.com","threadId":"19813","inReplyTo":"20090615145716.GW16497@spearce.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2009-06-15T19:06:01Z","receivedAt":"2009-06-15T19:06:01Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> \n> They could, if they were broken.  :-)\n> \n> IIRC only upload-pack produces progress (from pack-objects).\n> It does so by using a pipe on fd 2, and either copying it down\n> to the client via side-band, or discarding it.  So progress data\n> shouldn't ever appear on upload-pack's own fd 2, which means you\n> won't get it in this syslog thing.\n> \n> But I have to wonder, why are we doing this?  Why can't we teach the\n> individual server program to record its error to the syslog before\n> it aborts?  Are we looking for SIGSEGV or something?  Its only the\n> daemon program staying around in memory, but that's a lot of little\n> daemons doing nothing waiting for their children to terminate.\n> Seems like a waste to me.\n>  \n\nActually, even logging signals would be useful, so I think this makes\nsense.  The daemon process is pretty trivial compared to the rest of the\nstuff being spawned.\n\n\t-hpa\n"},{"id":"116355","messageId":"200906152324.43435.j6t@kdbg.org","threadId":"19813","inReplyTo":"20090615145716.GW16497@spearce.org","subject":"[PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-15T21:24:43Z","receivedAt":"2009-06-15T21:24:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"upload-pack runs pack-objects, which generates progress indicator output\non its stderr. If the client requests a sideband, this indicator is sent\nto the client; but if it did not, then the progress is written to\nupload-pack's own stderr.\n\nSince the previous patch git-daemon monitors stderr of the service program,\nsuch as upload-pack, and copies it to the syslog. This would now also copy\nthe progress indicator to the syslog. We avoid this by calling pack-objects\nwithout --progress if there is no sideband channel to the client.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nOn Montag, 15. Juni 2009, Shawn O. Pearce wrote:\n> IIRC only upload-pack produces progress (from pack-objects).\n> It does so by using a pipe on fd 2, and either copying it down\n> to the client via side-band, or discarding it.  So progress data\n> shouldn't ever appear on upload-pack's own fd 2, which means you\n> won't get it in this syslog thing.\n\nUnfortunately, upload-pack *does* write the progress to fd 2,\nand this fixes it.\n\n-- Hannes\n\n upload-pack.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex edc7861..fef8be5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -521,6 +521,15 @@ static void receive_needs(void)\n \t}\n \tif (debug_fd)\n \t\twrite_in_full(debug_fd, \"#E\\n\", 3);\n+\n+\t/*\n+\t * If upload-pack is run from the daemon and the client did not\n+\t * request a sideband, the progress output produced by pack-objects\n+\t * would go to the syslog. Squelch it.\n+\t */\n+\tif (!use_sideband)\n+\t\tno_progress = 1;\n+\n \tif (depth == 0 && shallows.nr == 0)\n \t\treturn;\n \tif (depth > 0) {\n-- \n1.6.3.17.g1665f\n"},{"id":"116361","messageId":"200906152339.43607.j6t@kdbg.org","threadId":"19813","inReplyTo":"200906142238.51725.j6t@kdbg.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-15T21:39:43Z","receivedAt":"2009-06-15T21:39:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 14. Juni 2009, Johannes Sixt wrote:\n>  I don't know whether service programs like upload-archive or upload-pack\n>  write progress report to stderr or not, for example, if a client does not\n>  support side-bands. In this case this patch is probably not enough since\n>  this would fill the log with unneeded progress information. Any hints\n>  are appreciated.\n\nThe progress indicator can be helped . But there is now another anoyance: If \nthe client terminates the connection early, this is now logged as:\n\nfatal: unable to run 'git-upload-pack'\n\nThe reason for this is that upload-pack is run as 'git upload-pack', which \nitself spawns the external 'git-upload-pack'. The latter dies from a SIGPIPE, \nand the former, in execv_dashed_external(), dutyfully writes this down.\n\nThe easiest solution is perhaps to make upload-pack a builtin.\n\nBUT... The motivation, of which this patch is actually a fall-out, is to clean \nup the messy error behavor of the start,finish,run_command family. To take \ncare of this error message is just one more (hopefully small) point on my \nagenda.\n\n>  I intend to follow-up this patch with another one that integrates\n>  run_service_command() in execute() in order to streamline how the\n>  'incoming' fd is inherited to the service programs.\n\nI'm not sure anymore whether the change I planned here is worth it. When I \nwrote this announcement, I had mis-remembered how daemon.c's handle() and \nexecute() functions and --inetd mode interact.\n\n-- Hannes\n"},{"id":"116362","messageId":"4A36C086.5010306@zytor.com","threadId":"19813","inReplyTo":"200906152339.43607.j6t@kdbg.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2009-06-15T21:43:34Z","receivedAt":"2009-06-15T21:43:34Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Johannes Sixt wrote:\n> On Sonntag, 14. Juni 2009, Johannes Sixt wrote:\n>>  I don't know whether service programs like upload-archive or upload-pack\n>>  write progress report to stderr or not, for example, if a client does not\n>>  support side-bands. In this case this patch is probably not enough since\n>>  this would fill the log with unneeded progress information. Any hints\n>>  are appreciated.\n> \n> The progress indicator can be helped . But there is now another anoyance: If \n> the client terminates the connection early, this is now logged as:\n> \n> fatal: unable to run 'git-upload-pack'\n> \n> The reason for this is that upload-pack is run as 'git upload-pack', which \n> itself spawns the external 'git-upload-pack'. The latter dies from a SIGPIPE, \n> and the former, in execv_dashed_external(), dutyfully writes this down.\n> \n> The easiest solution is perhaps to make upload-pack a builtin.\n> \n> BUT... The motivation, of which this patch is actually a fall-out, is to clean \n> up the messy error behavor of the start,finish,run_command family. To take \n> care of this error message is just one more (hopefully small) point on my \n> agenda.\n> \n\nWe probably do want to log that the client has disconnected.\n\n\t-hpa\n"},{"id":"116364","messageId":"7vski1i2ov.fsf@alter.siamese.dyndns.org","threadId":"19813","inReplyTo":"200906152324.43435.j6t@kdbg.org","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-15T21:53:36Z","receivedAt":"2009-06-15T21:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Since the previous patch git-daemon monitors stderr of the service program,\n> such as upload-pack, and copies it to the syslog. This would now also copy\n> the progress indicator to the syslog. We avoid this by calling pack-objects\n> without --progress if there is no sideband channel to the client.\n> ...\n> +\n> +\t/*\n> +\t * If upload-pack is run from the daemon and the client did not\n> +\t * request a sideband, the progress output produced by pack-objects\n> +\t * would go to the syslog. Squelch it.\n> +\t */\n> +\tif (!use_sideband)\n> +\t\tno_progress = 1;\n> +\n\nI think it is a very good idea to squelch progress output that will never\ngo to the client (it will be wasted traffic, regardless of the \"syslog\"\nthing), but\n\n (1) Is \"not using sideband\" the same as \"client won't see the progress\n     output\" for all vintages of clients that work with the current\n     server?\n\n     How did we drive upload-pack over native or ssh connection before we\n     introduced sideband?  I vaguely recall that we relied on stderr going\n     to the invoking terminal in the local case.  With this change, does\n     the user suddenly stop seeing progress if the client is older than\n     583b7ea (upload-pack/fetch-pack: support side-band communication,\n     2006-06-21)?  If so, that would be a regression.\n\n (2) The change in _this_ patch may be a good thing independent from the\n     change to the daemon, and I would hate to see it justified in terms\n     of that other change.  This comment applies to the proposed commit\n     log message as well.\n"},{"id":"116373","messageId":"alpine.LFD.2.00.0906152109240.16467@xanadu.home","threadId":"19813","inReplyTo":"7vski1i2ov.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-16T01:16:46Z","receivedAt":"2009-06-16T01:16:46Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 15 Jun 2009, Junio C Hamano wrote:\n\n> I think it is a very good idea to squelch progress output that will never\n> go to the client (it will be wasted traffic, regardless of the \"syslog\"\n> thing), but\n> \n>  (1) Is \"not using sideband\" the same as \"client won't see the progress\n>      output\" for all vintages of clients that work with the current\n>      server?\n> \n>      How did we drive upload-pack over native or ssh connection before we\n>      introduced sideband?  I vaguely recall that we relied on stderr going\n>      to the invoking terminal in the local case.  With this change, does\n>      the user suddenly stop seeing progress if the client is older than\n>      583b7ea (upload-pack/fetch-pack: support side-band communication,\n>      2006-06-21)?  If so, that would be a regression.\n\nNative, or git:// style, certainly never was able to get any kind of \nprogress display without sideband support.\n\nAs to old clients using ssh... well... that could be a regression.  But \ndo we really care?  This is pre v1.4.1 after all, and even the previous \nDebian stable was using a later git version.\n\n\nNicolas\n"},{"id":"116383","messageId":"7vprd4hito.fsf@alter.siamese.dyndns.org","threadId":"19813","inReplyTo":"alpine.LFD.2.00.0906152109240.16467@xanadu.home","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-16T05:02:43Z","receivedAt":"2009-06-16T05:02:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n>>      How did we drive upload-pack over native or ssh connection before we\n>>      introduced sideband?  I vaguely recall that we relied on stderr going\n>>      to the invoking terminal in the local case.  With this change, does\n>>      the user suddenly stop seeing progress if the client is older than\n>>      583b7ea (upload-pack/fetch-pack: support side-band communication,\n>>      2006-06-21)?  If so, that would be a regression.\n>\n> Native, or git:// style, certainly never was able to get any kind of \n> progress display without sideband support.\n>\n> As to old clients using ssh... well... that could be a regression.  But \n> do we really care?  This is pre v1.4.1 after all, and even the previous \n> Debian stable was using a later git version.\n\nI guess both ssh and local would regress, and we do care about regressions\nin general, but I think it is not a grave offense in this case, because\nthe combination between such an old fetch and the updated upload-pack will\nstill transfer the objects and will update the refs as expected; only the\nprogress indication and chatter on the stderr stream will be lost.\n\nSo I would say it is Ok to regress in this particular case, _if_ there is\nno easy way to avoid it.  That was why I asked:\n\n>> I think it is a very good idea to squelch progress output that will never\n>> go to the client (it will be wasted traffic, regardless of the \"syslog\"\n>> thing), but\n>> \n>>  (1) Is \"not using sideband\" the same as \"client won't see the progress\n>>      output\" for all vintages of clients that work with the current\n>>      server?\n\nStated differently, I think \"not using sideband _and_ spawned via daemon\"\nwould be an indication that \"the client won't see the progress anyway even\nif it were sent.\"  So the question becomes \"will it be a small enough\nchange to detect if the upload-pack is driven by the daemon in the\ncodepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do so?\"\n"},{"id":"116427","messageId":"alpine.LFD.2.00.0906161354010.16467@xanadu.home","threadId":"19813","inReplyTo":"7vprd4hito.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-16T18:03:29Z","receivedAt":"2009-06-16T18:03:29Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 15 Jun 2009, Junio C Hamano wrote:\n\n> I guess both ssh and local would regress, and we do care about regressions\n> in general, but I think it is not a grave offense in this case, because\n> the combination between such an old fetch and the updated upload-pack will\n> still transfer the objects and will update the refs as expected; only the\n> progress indication and chatter on the stderr stream will be lost.\n\nMy point exactly, although I realize I didn't make myself clear.\n\n> So I would say it is Ok to regress in this particular case, _if_ \nthere is\n> no easy way to avoid it.  That was why I asked:\n> \n> >> I think it is a very good idea to squelch progress output that will never\n> >> go to the client (it will be wasted traffic, regardless of the \"syslog\"\n> >> thing), but\n> >> \n> >>  (1) Is \"not using sideband\" the same as \"client won't see the progress\n> >>      output\" for all vintages of clients that work with the current\n> >>      server?\n> \n> Stated differently, I think \"not using sideband _and_ spawned via daemon\"\n> would be an indication that \"the client won't see the progress anyway even\n> if it were sent.\"  So the question becomes \"will it be a small enough\n> change to detect if the upload-pack is driven by the daemon in the\n> codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do so?\"\n\nI don't think it is worth it at all.  The regression is purely cosmetic, \nand I suspect you'll have a really hard time finding someone still using \nthose ancient git clients anyway.  Remember that such clients are unable \nto fetch with HTTP from repositories using version 2 of the pack index \nby default already.  That's why we created version 1.4.4.5.\n\n\nNicolas\n"},{"id":"116429","messageId":"200906162041.16608.j6t@kdbg.org","threadId":"19813","inReplyTo":"alpine.LFD.2.00.0906161354010.16467@xanadu.home","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-16T18:41:16Z","receivedAt":"2009-06-16T18:41:16Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:\n> On Mon, 15 Jun 2009, Junio C Hamano wrote:\n> > Stated differently, I think \"not using sideband _and_ spawned via daemon\"\n> > would be an indication that \"the client won't see the progress anyway\n> > even if it were sent.\"  So the question becomes \"will it be a small\n> > enough change to detect if the upload-pack is driven by the daemon in the\n> > codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do\n> > so?\"\n>\n> I don't think it is worth it at all.  The regression is purely cosmetic,\n> and I suspect you'll have a really hard time finding someone still using\n> those ancient git clients anyway.  Remember that such clients are unable\n> to fetch with HTTP from repositories using version 2 of the pack index\n> by default already.  That's why we created version 1.4.4.5.\n\nKeep in mind that there could exist clients outside git.git that use modern\npack-index or unpack-objects, but omit sideband support.\n\nI propose this patch instead, although the assumption that the --timeout\noption is only used by git-daemon may not be quite right. In this case it\nmay be better to add a new option that sets the new flag and that\ngit-daemon has to use.\n\n-- Hannes\n\nSubject: [PATCH] upload-pack: squelch progress indicator if client cannot see it\n\nupload-pack runs pack-objects, which generates progress indicator output\non its stderr. If the client requests a sideband, this indicator is sent\nto the client; but if it did not, then the progress is written to\nupload-pack's own stderr.\n\nIf upload-pack is itself run from git-daemon (and if the client did not\nrequest a sideband) the progress indicator never reaches the client and it\nneed not be generated in the first place. With this patch the progress\nindicator is suppressed in this situation.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n upload-pack.c |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex edc7861..841ebb5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -28,7 +28,7 @@ static unsigned long oldest_have;\n \n static int multi_ack, nr_our_refs;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n-static int no_progress;\n+static int no_progress, daemon_mode;\n static struct object_array have_obj;\n static struct object_array want_obj;\n static unsigned int timeout;\n@@ -521,6 +521,10 @@ static void receive_needs(void)\n \t}\n \tif (debug_fd)\n \t\twrite_in_full(debug_fd, \"#E\\n\", 3);\n+\n+\tif (!use_sideband && daemon_mode)\n+\t\tno_progress = 1;\n+\n \tif (depth == 0 && shallows.nr == 0)\n \t\treturn;\n \tif (depth > 0) {\n@@ -630,6 +634,7 @@ int main(int argc, char **argv)\n \t\t}\n \t\tif (!prefixcmp(arg, \"--timeout=\")) {\n \t\t\ttimeout = atoi(arg+10);\n+\t\t\tdaemon_mode = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n-- \n1.6.3.17.g1665f\n"},{"id":"116432","messageId":"7vski0c8gi.fsf@alter.siamese.dyndns.org","threadId":"19813","inReplyTo":"200906162041.16608.j6t@kdbg.org","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-16T18:57:49Z","receivedAt":"2009-06-16T18:57:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:\n> ...\n>> I don't think it is worth it at all.  The regression is purely cosmetic,\n>> and I suspect you'll have a really hard time finding someone still using\n>> those ancient git clients anyway.  Remember that such clients are unable\n>> to fetch with HTTP from repositories using version 2 of the pack index\n>> by default already.  That's why we created version 1.4.4.5.\n>\n> Keep in mind that there could exist clients outside git.git that use modern\n> pack-index or unpack-objects, but omit sideband support.\n>\n> I propose this patch instead, although the assumption that the --timeout\n> option is only used by git-daemon may not be quite right. In this case it\n> may be better to add a new option that sets the new flag and that\n> git-daemon has to use.\n\nEven though I tend to agree with Nico, this patch strikes a good balance\nwith a very small impact.  I am beginning to like it.\n\nThere _could_ be people who run fetch over git/ssh with custom --exec\noption that has --timeout in it.  We _could_ teach --run-by-daemon option\nto the (daemon, upload-pack) pair in the same release (because it is\nreasonable to assume they come from the same vintage of git) to fix that\ncase.  But I do not think it is worth it.  As Nico says, this is purely\ncosmetic.\n\nThanks.\n"},{"id":"116438","messageId":"alpine.LFD.2.00.0906161542590.16467@xanadu.home","threadId":"19813","inReplyTo":"200906162041.16608.j6t@kdbg.org","subject":"Re: [PATCH] upload-pack: squelch progress indicator if client does not request sideband","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-06-16T19:46:46Z","receivedAt":"2009-06-16T19:46:46Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 16 Jun 2009, Johannes Sixt wrote:\n\n> On Dienstag, 16. Juni 2009, Nicolas Pitre wrote:\n> > On Mon, 15 Jun 2009, Junio C Hamano wrote:\n> > > Stated differently, I think \"not using sideband _and_ spawned via daemon\"\n> > > would be an indication that \"the client won't see the progress anyway\n> > > even if it were sent.\"  So the question becomes \"will it be a small\n> > > enough change to detect if the upload-pack is driven by the daemon in the\n> > > codepath J6t added 'if (!use_sideband)' to, and if so shouldn't we do\n> > > so?\"\n> >\n> > I don't think it is worth it at all.  The regression is purely cosmetic,\n> > and I suspect you'll have a really hard time finding someone still using\n> > those ancient git clients anyway.  Remember that such clients are unable\n> > to fetch with HTTP from repositories using version 2 of the pack index\n> > by default already.  That's why we created version 1.4.4.5.\n> \n> Keep in mind that there could exist clients outside git.git that use modern\n> pack-index or unpack-objects, but omit sideband support.\n\nAnd that would be an abominable thing to make special support for such \nclients.  If they're fancy enough to want progress display, they should \ncomply with what moderately recent native git clients do and get \nsideband support.\n\n\nNicolas\n"},{"id":"116442","messageId":"200906162227.00377.j6t@kdbg.org","threadId":"19813","inReplyTo":"4A36C086.5010306@zytor.com","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-16T20:27:00Z","receivedAt":"2009-06-16T20:27:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Montag, 15. Juni 2009, H. Peter Anvin wrote:\n> Johannes Sixt wrote:\n> > The progress indicator can be helped . But there is now another anoyance:\n> > If the client terminates the connection early, this is now logged as:\n> >\n> > fatal: unable to run 'git-upload-pack'\n>\n> We probably do want to log that the client has disconnected.\n\nAny client, or only clients that disconnect early? Is this useful besides \ndebugging?\n\nI think git-daemon's --verbose is intended to log all client connects and \ndisconnects. This here is about a *new* message that would be logged \nwithout --verbose.\n\n-- Hannes\n"},{"id":"116443","messageId":"4A3800DD.10809@zytor.com","threadId":"19813","inReplyTo":"200906162227.00377.j6t@kdbg.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2009-06-16T20:30:21Z","receivedAt":"2009-06-16T20:30:21Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Johannes Sixt wrote:\n> On Montag, 15. Juni 2009, H. Peter Anvin wrote:\n>> Johannes Sixt wrote:\n>>> The progress indicator can be helped . But there is now another anoyance:\n>>> If the client terminates the connection early, this is now logged as:\n>>>\n>>> fatal: unable to run 'git-upload-pack'\n>> We probably do want to log that the client has disconnected.\n> \n> Any client, or only clients that disconnect early? Is this useful besides \n> debugging?\n> \n\nClients that disconnect without completing the transaction.  This is\nhighly valuable to administrators when tracking down a problem (since it\ngenerally implies a client or network problem, as opposed to a server\nproblem.)\n\n\t-hpa\n"},{"id":"116636","messageId":"7veitgkm0i.fsf@alter.siamese.dyndns.org","threadId":"19813","inReplyTo":"200906142238.51725.j6t@kdbg.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-19T20:26:53Z","receivedAt":"2009-06-19T20:26:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> diff --git a/daemon.c b/daemon.c\n> index d7ceca4..3e1a354 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1,6 +1,7 @@\n>  #include \"cache.h\"\n>  #include \"pkt-line.h\"\n> -#include \"exec_cmd.h\"\n> +#include \"run-command.h\"\n> +#include \"strbuf.h\"\n\n    CC daemon.o\ncc1: warnings being treated as errors\ndaemon.c: In function 'main':\ndaemon.c:981: error: implicit declaration of function 'git_extract_argv0_path'\nmake: *** [daemon.o] Error 1\n\nI'll add the include back in the meantime.\n"},{"id":"116680","messageId":"200906202125.10844.j6t@kdbg.org","threadId":"19813","inReplyTo":"7veitgkm0i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: send stderr of service programs to the syslog","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-06-20T19:25:10Z","receivedAt":"2009-06-20T19:25:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 19. Juni 2009, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> > diff --git a/daemon.c b/daemon.c\n> > index d7ceca4..3e1a354 100644\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -1,6 +1,7 @@\n> >  #include \"cache.h\"\n> >  #include \"pkt-line.h\"\n> > -#include \"exec_cmd.h\"\n> > +#include \"run-command.h\"\n> > +#include \"strbuf.h\"\n>\n>     CC daemon.o\n> cc1: warnings being treated as errors\n> daemon.c: In function 'main':\n> daemon.c:981: error: implicit declaration of function\n> 'git_extract_argv0_path' make: *** [daemon.o] Error 1\n>\n> I'll add the include back in the meantime.\n\nOops, you are right! Thanks for noticing it. Silly me: I didn't have -Wall in \nmy CFLAGS.\n\n-- Hannes\n"}]}