Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Jeff King]

On Tue, Jul 19, 2016 at 09:05:14AM -0700, Stefan Beller wrote:

> > I know that writing the function myself and then critiquing is the very
> > definition of a strawman. :) But it's the best I could think of.  Maybe
> > you had something more clever in mind?
> 
> Actually no, I had not. I was hoping you could come up with a clever thing.
> My original point was the perceived added complexity to a simple
> seemingly simple function (copy_to_sideband in your original patch),
> so my gut reaction was to shove the complexity away into a helper function.
> But no matter how it is done, there is always this one function that looks
> complex for this problem. So I think your original approach is fine then?

Yeah, I agree the original was nicer, but IMHO none of the "like this"
alternatives I showed in this thread are real improvements. So I'm
inclined to go with what was originally posted (though I'm open to
more input or suggestions).

The only avenue I didn't look at is actually ditching the async muxer
completely for the index-pack call, and replacing it with the main
thread doing a poll() on stdout/stderr from index-pack. That's probably
going to end up as _more_ code, but it potentially makes the "report
magic NUL" a little less gross (we could signal with something more sane
on stdout). I can explore that if people want me to.

If not, then the only change to the original series is adding "static"
to the rev-list progress variables that Junio noticed. A replacement for
patch 2 is below in case that makes things easier.

-- >8 --
Subject: [PATCH] rev-list: add optional progress reporting

It's easy to ask rev-list to do a traversal that may takes
many seconds (e.g., by calling "--objects --all"). In theory
you can monitor its progress by the output you get to
stdout, but this isn't always easy.

Some operations, like "--count", don't make any output until
the end.

And some callers, like check_everything_connected(), are
using it just for the error-checking of the traversal, and
throw away stdout entirely.

This patch adds a "--progress" option which can be used to
give some eye-candy for a user waiting for a long traversal.
This is just a rev-list option and not a regular traversal
option, because it needs cooperation from the callbacks in
builtin/rev-list.c to do the actual count.

Signed-off-by: Jeff King 
---
 Documentation/rev-list-options.txt |  4 
 builtin/rev-list.c | 17 +
 2 files changed, 21 insertions(+)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index c5bd218..f39cb6d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -274,6 +274,10 @@ ifdef::git-rev-list[]
Try to speed up the traversal using the pack bitmap index (if
one is available). Note that when traversing with `--objects`,
trees and blobs will not have their associated path printed.
+
+--progress=::
+   Show progress reports on stderr as objects are considered. The
+   `` text will be printed with each progress update.
 endif::git-rev-list[]
 
 --
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index b82bcc3..0ba82b1 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -9,6 +9,7 @@
 #include "log-tree.h"
 #include "graph.h"
 #include "bisect.h"
+#include "progress.h"
 
 static const char rev_list_usage[] =
 "git rev-list [OPTION] ... [ -- paths... ]\n"
@@ -49,12 +50,17 @@ static const char rev_list_usage[] =
 "--bisect-all"
 ;
 
+static struct progress *progress;
+static unsigned progress_counter;
+
 static void finish_commit(struct commit *commit, void *data);
 static void show_commit(struct commit *commit, void *data)
 {
struct rev_list_info *info = data;
struct rev_info *revs = info->revs;
 
+   display_progress(progress, ++progress_counter);
+
if (info->flags & REV_LIST_QUIET) {
finish_commit(commit, data);
return;
@@ -190,6 +196,7 @@ static void show_object(struct object *obj, const char 
*name, void *cb_data)
 {
struct rev_list_info *info = cb_data;
finish_object(obj, name, cb_data);
+   display_progress(progress, ++progress_counter);
if (info->flags & REV_LIST_QUIET)
return;
show_object_with_name(stdout, obj, name);
@@ -276,6 +283,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
int bisect_show_vars = 0;
int bisect_find_all = 0;
int use_bitmap_index = 0;
+   const char *show_progress = NULL;
 
git_config(git_default_config, NULL);
init_revisions(, prefix);
@@ -325,6 +333,10 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
test_bitmap_walk();
return 0;
}
+   if (skip_prefix(arg, "--progress=", )) {
+   show_progress = arg;
+   continue;
+ 

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Stefan Beller]

On Tue, Jul 19, 2016 at 3:07 AM, Jeff King  wrote:
> On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:
>
>> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King  wrote:
>> >> > +   if (use_keepalive == KEEPALIVE_AFTER_NUL && 
>> >> > !keepalive_active) {
>> >> > +   const char *p = memchr(data, '\0', sz);
>> >> > +   if (p) {
>> >> > +   /*
>> >> > +* The NUL tells us to start sending 
>> >> > keepalives. Make
>> >> > +* sure we send any other data we read 
>> >> > along
>> >> > +* with it.
>> >> > +*/
>> >> > +   keepalive_active = 1;
>> >> > +   send_sideband(1, 2, data, p - data, 
>> >> > use_sideband);
>> >> > +   send_sideband(1, 2, p + 1, sz - (p - 
>> >> > data + 1), use_sideband);
>> >> > +   continue;
>> >>
>> >> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I 
>> >> thought.
>> >> I wonder if we can use a better read function, that would stop reading at 
>> >> a NUL,
>> >> and return early instead?
>> >
>> > How would you do that, if not by read()ing a byte at a time (which is
>> > inefficient)? Otherwise you have to deal with the leftovers (after the
>> > NUL) in your buffer. It's one of the reasons I went with a single-byte
>> > signal, because otherwise it gets rather complicated to do robustly.
>>
>> I do not question the concept of a single NUL byte, but rather the
>> implementation, i.e. if we had an xread_until_nul you would not need
>> to have a double send_sideband here?
>
> What would xread_until_nul() look like?
>
> The only reasonable implementation I can think of is:
>
>   ssize_t xread_until_nul(int fd, char *out, size_t len)
>   {
> ssize_t total = 0;
> while (total < len) {
> ssize_t ret = xread(fd, out + total, 1);
> if (ret < 0) {
> /* Oops, anything in out[0..total] is lost, but
>  * we have no way of signaling both a partial
>  * read and an error at the end; fixable by
>  * changing the interface, but doesn't really
>  * matter in practice for this application. */
> return -1;
> }
> if (ret == 0)
> break; /* EOF, stop reading */
> if (out[total] == '\0')
> break; /* got our NUL, stop reading */
> total++;
> }
> return total;
>   }
>
> There are two problems with this function:
>
>   1. Until we see the NUL, we're making an excessive number of read()
>  syscalls looking for it. You could make larger reads, but then what
>  do you do with the residual bytes (i.e., the ones after the NUL in
>  the buffer you read)? You'd have to somehow save it to return at
>  the next xread_until_nul(). Which implie some kind of internal
>  buffering.
>
>  The reason there are two send_sidebands is to cover the case where
>  we have some real data, then the signal byte, then some more data.
>  So instead of buffering, we just handle the data immediately.
>
>   2. How does it know when to return?
>
>  We want to send the data as soon as it is available, in as large a
>  chunk as possible. Using a single xread() as we do now, we get
>  whatever the OS has for us, up to our buffer size.
>
>  But after each 1-byte read above, we would not want to return;
>  there might be more data. So it keeps reading until NUL or we fill
>  our buffer. But that may mean the xread() blocks, even though we
>  have data that _could_ be shipped over the sideband.
>
>  To fix that, you'd have to poll() for each xread(), and return when
>  it says nothing's ready.
>
> I know that writing the function myself and then critiquing is the very
> definition of a strawman. :) But it's the best I could think of.  Maybe
> you had something more clever in mind?

Actually no, I had not. I was hoping you could come up with a clever thing.
My original point was the perceived added complexity to a simple
seemingly simple function (copy_to_sideband in your original patch),
so my gut reaction was to shove the complexity away into a helper function.
But no matter how it is done, there is always this one function that looks
complex for this problem. So I think your original approach is fine then?

Thanks,
Stefan


>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Jeff King]

On Mon, Jul 18, 2016 at 10:28:25PM -0700, Stefan Beller wrote:

> On Sat, Jul 16, 2016 at 12:56 AM, Jeff King  wrote:
> >> > +   if (use_keepalive == KEEPALIVE_AFTER_NUL && 
> >> > !keepalive_active) {
> >> > +   const char *p = memchr(data, '\0', sz);
> >> > +   if (p) {
> >> > +   /*
> >> > +* The NUL tells us to start sending 
> >> > keepalives. Make
> >> > +* sure we send any other data we read 
> >> > along
> >> > +* with it.
> >> > +*/
> >> > +   keepalive_active = 1;
> >> > +   send_sideband(1, 2, data, p - data, 
> >> > use_sideband);
> >> > +   send_sideband(1, 2, p + 1, sz - (p - 
> >> > data + 1), use_sideband);
> >> > +   continue;
> >>
> >> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I 
> >> thought.
> >> I wonder if we can use a better read function, that would stop reading at 
> >> a NUL,
> >> and return early instead?
> >
> > How would you do that, if not by read()ing a byte at a time (which is
> > inefficient)? Otherwise you have to deal with the leftovers (after the
> > NUL) in your buffer. It's one of the reasons I went with a single-byte
> > signal, because otherwise it gets rather complicated to do robustly.
> 
> I do not question the concept of a single NUL byte, but rather the
> implementation, i.e. if we had an xread_until_nul you would not need
> to have a double send_sideband here?

What would xread_until_nul() look like?

The only reasonable implementation I can think of is:

  ssize_t xread_until_nul(int fd, char *out, size_t len)
  {
ssize_t total = 0;
while (total < len) {
ssize_t ret = xread(fd, out + total, 1);
if (ret < 0) {
/* Oops, anything in out[0..total] is lost, but
 * we have no way of signaling both a partial
 * read and an error at the end; fixable by
 * changing the interface, but doesn't really
 * matter in practice for this application. */
return -1;
}
if (ret == 0)
break; /* EOF, stop reading */
if (out[total] == '\0')
break; /* got our NUL, stop reading */
total++;
}
return total;
  }

There are two problems with this function:

  1. Until we see the NUL, we're making an excessive number of read()
 syscalls looking for it. You could make larger reads, but then what
 do you do with the residual bytes (i.e., the ones after the NUL in
 the buffer you read)? You'd have to somehow save it to return at
 the next xread_until_nul(). Which implie some kind of internal
 buffering.

 The reason there are two send_sidebands is to cover the case where
 we have some real data, then the signal byte, then some more data.
 So instead of buffering, we just handle the data immediately.

  2. How does it know when to return?

 We want to send the data as soon as it is available, in as large a
 chunk as possible. Using a single xread() as we do now, we get
 whatever the OS has for us, up to our buffer size.

 But after each 1-byte read above, we would not want to return;
 there might be more data. So it keeps reading until NUL or we fill
 our buffer. But that may mean the xread() blocks, even though we
 have data that _could_ be shipped over the sideband.

 To fix that, you'd have to poll() for each xread(), and return when
 it says nothing's ready.

I know that writing the function myself and then critiquing is the very
definition of a strawman. :) But it's the best I could think of.  Maybe
you had something more clever in mind?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Stefan Beller]

On Sat, Jul 16, 2016 at 12:56 AM, Jeff King  wrote:
>> > +   if (use_keepalive == KEEPALIVE_AFTER_NUL && 
>> > !keepalive_active) {
>> > +   const char *p = memchr(data, '\0', sz);
>> > +   if (p) {
>> > +   /*
>> > +* The NUL tells us to start sending 
>> > keepalives. Make
>> > +* sure we send any other data we read 
>> > along
>> > +* with it.
>> > +*/
>> > +   keepalive_active = 1;
>> > +   send_sideband(1, 2, data, p - data, 
>> > use_sideband);
>> > +   send_sideband(1, 2, p + 1, sz - (p - data 
>> > + 1), use_sideband);
>> > +   continue;
>>
>> Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
>> I wonder if we can use a better read function, that would stop reading at a 
>> NUL,
>> and return early instead?
>
> How would you do that, if not by read()ing a byte at a time (which is
> inefficient)? Otherwise you have to deal with the leftovers (after the
> NUL) in your buffer. It's one of the reasons I went with a single-byte
> signal, because otherwise it gets rather complicated to do robustly.

I do not question the concept of a single NUL byte, but rather the
implementation,
i.e. if we had an xread_until_nul you would not need to have a double
send_sideband
here?

>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Jeff King]

On Fri, Jul 15, 2016 at 10:24:16AM -0700, Stefan Beller wrote:

> > @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)
> >  static int copy_to_sideband(int in, int out, void *arg)
> >  {
> > char data[128];
> 
> While looking at this code, do you think it is feasible to increase the
> size of data[] to 1024 ? (The largest that is possible when
> side-band, but no side-band-64k is given).

I don't see any reason it could not be increased. On the other hand, I
find it unlikely to accomplish anything. We are relaying stderr over
sideband 2 here, so assuming that typically consists of human-readable
lines, they're unlikely to go over 128 (and if they do, they simply get
split into two packets).

I suppose if your hooks dump large ascii art to the user, we could save
a few framing bytes.

So I don't mind it, but it's definitely orthogonal to this series.

> The method was short and concise, this adds a lot of lines.
> Remembering d751dd11 (2016-07-10, hoist out handle_nonblock
> function for xread and xwrite), do you think it would be reasonable to
> put the whole poll handling into a dedicated function, maybe even reuse the
> that function?
> 
> if (keepalive_active) {
> if (wrapper_around_poll(_in) < 0) // handles EINTR internally
> break;
> if (!data_in)
> send_keep_alive();
> }
> 
> I am not sure if that makes this function more legible, just food for thought.

That would skip the EINTR continue, though since we're looping anyway
here, I'm not sure it's a big deal. The biggest simplification is more
like this, I think:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e41f55f..8d792a2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -328,6 +328,32 @@ static void rp_error(const char *err, ...)
va_end(params);
 }
 
+/* returns 1 if data is available, 0 otherwise */
+static int keepalive_poll(int in, int out)
+{
+   struct pollfd pfd;
+   int ret;
+
+   pfd.fd = fd;
+   pfd.events = POLLIN;
+   ret = poll(, 1, 1000 * keepalive_in_sec);
+
+   if (ret < 0) {
+   if (errno == EINTR)
+   return 0; /* no data, we should try again */
+   else
+   return 1; /* no data, but read should return real error 
*/
+   }
+   if (ret == 0) {
+   /* no data; send a keepalive packet */
+   static const char buf[] = "0005\1";
+   write_or_die(out, buf, sizeof(buf) - 1);
+   return 0;
+   }
+   /* else there is actual data to read */
+   return 1;
+}
+
 static int copy_to_sideband(int in, int out, void *arg)
 {
char data[128];
@@ -341,26 +367,8 @@ static int copy_to_sideband(int in, int out, void *arg)
while (1) {
ssize_t sz;
 
-   if (keepalive_active) {
-   struct pollfd pfd;
-   int ret;
-
-   pfd.fd = in;
-   pfd.events = POLLIN;
-   ret = poll(, 1, 1000 * keepalive_in_sec);
-
-   if (ret < 0) {
-   if (errno == EINTR)
-   continue;
-   else
-   break;
-   } else if (ret == 0) {
-   /* no data; send a keepalive packet */
-   static const char buf[] = "0005\1";
-   write_or_die(1, buf, sizeof(buf) - 1);
-   continue;
-   } /* else there is actual data to read */
-   }
+   if (keepalive_active && !keepalive_poll(in, 1))
+   continue;
 
sz = xread(in, data, sizeof(data));
if (sz <= 0)

Note that there are actually three outcomes to poll (error, timeout, or
data), and this simplifies away the "error" case to just "try to read
some data" and assumes that the read() will cover (which is similar to
what xread() does).

I dunno if it really helps much, though. There's still a bunch of
keepalive logic in the function to handle the NUL-signal case.

I guess your proposal is more to have an "xpoll" that handles the EINTR
looping, and leave that logic in place. I think that still leaves most
of the complexity in the function.

> > +   } else if (ret == 0) {
> > +   /* no data; send a keepalive packet */
> > +   static const char buf[] = "0005\1";
> 
> and the \1 is the first sideband. Why do we choose that sideband?

What other sideband do you propose? :)

More seriously, this is modeled on the similar keepalive in upload-pack.
The only other option is sideband 2, and IIRC, clients do bad things
with empty band-2 packets (like printing "remote: " but never finishing
the line if we never 

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Junio C Hamano]

Jeff King  writes:

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 1cba120..54f2cfb 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1626,6 +1626,7 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   struct pack_idx_option opts;
>   unsigned char pack_sha1[20];
>   unsigned foreign_nr = 1;/* zero is a "good" value, assume bad */
> + int report_end_of_input = 0;
>  
>   if (argc == 2 && !strcmp(argv[1], "-h"))
>   usage(index_pack_usage);
> @@ -1697,6 +1698,8 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   verbose = 1;
>   } else if (!strcmp(arg, "--show-resolving-progress")) {
>   show_resolving_progress = 1;
> + } else if (!strcmp(arg, "--report-end-of-input")) {
> + report_end_of_input = 1;
>   } else if (!strcmp(arg, "-o")) {
>   if (index_name || (i+1) >= argc)
>   usage(index_pack_usage);
> @@ -1754,6 +1757,8 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   obj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct 
> object_stat));
>   ofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));
>   parse_pack_objects(pack_sha1);
> + if (report_end_of_input)
> + write_in_full(2, "\0", 1);
>   resolve_deltas();
>   conclude_pack(fix_thin_pack, curr_pack, pack_sha1);
>   free(ofs_deltas);

I naively thought "well, if we are teaching index-pack a new trick
anyway, why not make it do the keepalive?", but that would not work
because a keepalive is a 0-byte payload on band#1, i.e. "0005\1",
and index-pack is not aware of the sideband framing at all.

So I agree that a-nul-in-fd#2 is the best mechansim we can make it
work.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] receive-pack: send keepalives during quiet periods

[Stefan Beller]

On Fri, Jul 15, 2016 at 3:43 AM, Jeff King  wrote:

>
> Signed-off-by: Jeff King 

Read-entirely-by Stefan ;)
Thanks!

> @@ -319,10 +331,60 @@ static void rp_error(const char *err, ...)
>  static int copy_to_sideband(int in, int out, void *arg)
>  {
> char data[128];

While looking at this code, do you think it is feasible to increase the
size of data[] to 1024 ? (The largest that is possible when
side-band, but no side-band-64k is given).

> +   int keepalive_active = 0;
> +
> +   if (keepalive_in_sec <= 0)
> +   use_keepalive = KEEPALIVE_NEVER;
> +   if (use_keepalive == KEEPALIVE_ALWAYS)
> +   keepalive_active = 1;
> +
> while (1) {
> -   ssize_t sz = xread(in, data, sizeof(data));
> +   ssize_t sz;
> +
> +   if (keepalive_active) {
> +   struct pollfd pfd;
> +   int ret;
> +
> +   pfd.fd = in;
> +   pfd.events = POLLIN;
> +   ret = poll(, 1, 1000 * keepalive_in_sec);
> +
> +   if (ret < 0) {
> +   if (errno == EINTR)
> +   continue;
> +   else
> +   break;

The method was short and concise, this adds a lot of lines.
Remembering d751dd11 (2016-07-10, hoist out handle_nonblock
function for xread and xwrite), do you think it would be reasonable to
put the whole poll handling into a dedicated function, maybe even reuse the
that function?

if (keepalive_active) {
if (wrapper_around_poll(_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
}

I am not sure if that makes this function more legible, just food for thought.


> +   } else if (ret == 0) {
> +   /* no data; send a keepalive packet */
> +   static const char buf[] = "0005\1";

and the \1 is the first sideband. Why do we choose that sideband?

> +   write_or_die(1, buf, sizeof(buf) - 1);
> +   continue;
> +   } /* else there is actual data to read */

"If there is data to read, we need to break the while(1), to actually
read the data?"
I got confused and needed to go back and read the actual code again,
would it make sense to rather have a loop here?

while (1) {
   while(keepalive_active) {
if (wrapper_around_poll(_in) < 0) // handles EINTR internally
break;
if (!data_in)
send_keep_alive();
else
break;
}

   sz = xread(in, data, sizeof(data));
 if (sz <= 0)
 break;

turn_on_keepalive_on_NUL();
 }

> +   }
> +
> +   sz = xread(in, data, sizeof(data));
> if (sz <= 0)
> break;
> +
> +   if (use_keepalive == KEEPALIVE_AFTER_NUL && 
> !keepalive_active) {
> +   const char *p = memchr(data, '\0', sz);
> +   if (p) {
> +   /*
> +* The NUL tells us to start sending 
> keepalives. Make
> +* sure we send any other data we read along
> +* with it.
> +*/
> +   keepalive_active = 1;
> +   send_sideband(1, 2, data, p - data, 
> use_sideband);
> +   send_sideband(1, 2, p + 1, sz - (p - data + 
> 1), use_sideband);
> +   continue;

Oh, I see why the turn_on_keepalive_on_NUL doesn't work as well as I thought.
I wonder if we can use a better read function, that would stop reading at a NUL,
and return early instead?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/12] receive-pack: send keepalives during quiet periods

[Jeff King]

After a client has sent us the complete pack, we may spend
some time processing the data and running hooks. If the
client asked us to be quiet, receive-pack won't send any
progress data during the index-pack or connectivity-check
steps. And hooks may or may not produce their own progress
output. In these cases, the network connection is totally
silent from both ends.

Git itself doesn't care about this (it will wait forever),
but other parts of the system (e.g., firewalls,
load-balancers, etc) might hang up the connection. So we'd
like to send some sort of keepalive to let the network and
the client side know that we're still alive and processing.

We can use the same trick we did in 05e9515 (upload-pack:
send keepalive packets during pack computation, 2013-09-08).
Namely, we will send an empty sideband data packet every `N`
seconds that we do not relay any stderr data over the
sideband channel. As with 05e9515, this means that we won't
bother sending keepalives when there's actual progress data,
but will kick in when it has been disabled (or if there is a
lull in the progress data).

The concept is simple, but the details are subtle enough
that they need discussing here.

Before the client sends us the pack, we don't want to do any
keepalives. We'll have sent our ref advertisement, and we're
waiting for them to send us the pack (and tell us that they
support sidebands at all).

While we're receiving the pack from the client (or waiting
for it to start), there's no need for keepalives; it's up to
them to keep the connection active by sending data.
Moreover, it would be wrong for us to do so. When we are the
server in the smart-http protocol, we must treat our
connection as half-duplex. So any keepalives we send while
receiving the pack would potentially be buffered by the
webserver. Not only does this make them useless (since they
would not be delivered in a timely manner), but it could
actually cause a deadlock if we fill up the buffer with
keepalives. (It wouldn't be wrong to send keepalives in this
phase for a full-duplex connection like ssh; it's simply
pointless, as it is the client's responsibility to speak).

As soon as we've gotten all of the pack data, then the
client is waiting for us to speak, and we should start
keepalives immediately. From here until the end of the
connection, we send one any time we are not otherwise
sending data.

But there's a catch. Receive-pack doesn't know the moment
we've gotten all the data. It passes the descriptor to
index-pack, who reads all of the data, and then starts
resolving the deltas. We have to communicate that back.

To make this work, we instruct the sideband muxer to enable
keepalives in three phases:

  1. In the beginning, not at all.

  2. While reading from index-pack, wait for a signal
 indicating end-of-input, and then start them.

  3. Afterwards, always.

The signal from index-pack in phase 2 has to come over the
stderr channel which the muxer is reading. We can't use an
extra pipe because the portable run-command interface only
gives us stderr and stdout.

Stdout is already used to pass the .keep filename back to
receive-pack. We could also send a signal there, but then we
would find out about it in the main thread. And the
keepalive needs to be done by the async muxer thread (since
it's the one writing sideband data back to the client). And
we can't reliably signal the async thread from the main
thread, because the async code sometimes uses threads and
sometimes uses forked processes.

Therefore the signal must come over the stderr channel,
where it may be interspersed with other random
human-readable messages from index-pack. This patch makes
the signal a single NUL byte.  This is easy to parse, should
not appear in any normal stderr output, and we don't have to
worry about any timing issues (like seeing half the signal
bytes in one read(), and half in a subsequent one).

This is a bit ugly, but it's simple to code and should work
reliably.

Another option would be to stop using an async thread for
muxing entirely, and just poll() both stderr and stdout of
index-pack from the main thread. This would work for
index-pack (because we aren't doing anything useful in the
main thread while it runs anyway). But it would make the
connectivity check and the hook muxers much more
complicated, as they need to simultaneously feed the
sub-programs while reading their stderr.

The index-pack phase is the only one that needs this
signaling, so it could simply behave differently than the
other two. That would mean having two separate
implementations of copy_to_sideband (and the keepalive
code), though. And it still doesn't get rid of the
signaling; it just means we can write a nicer message like
"END_OF_INPUT" or something on stdout, since we don't have
to worry about separating it from the stderr cruft.

One final note: this signaling trick is only done with
index-pack, not with unpack-objects. There's no point in
doing it for the latter, because by