{"thread":{"id":"40245","subject":"[PATCH 0/2] clarify sideband muxing in GIT_TRACE_PACKET","startedAt":"2015-09-01T20:22:16Z","lastAt":"2015-09-01T22:38:08Z","messageCount":12,"participants":["Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"269148","messageId":"20150901202215.GA17370@sigill.intra.peff.net","threadId":"40245","inReplyTo":null,"subject":"[PATCH 0/2] clarify sideband muxing in GIT_TRACE_PACKET","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T20:22:16Z","receivedAt":"2015-09-01T20:22:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I happened to be debugging push with GIT_TRACE_PACKET today, and got\nconfused by the mixture of trace from the sideband demuxer and push\nitself (details in patch 2/2). This series fixes it by showing a\ndistinct prefix for the sideband reads.\n\nThere's some trickery with start_async() involved, so I've tested it\nboth with and without NO_PTHREADS set.\n\n  [1/2]: run-command: provide in_async query function\n  [2/2]: pkt-line: show packets in async processes as \"sideband\"\n\n-Peff\n"},{"id":"269149","messageId":"20150901202243.GA6931@sigill.intra.peff.net","threadId":"40245","inReplyTo":"20150901202215.GA17370@sigill.intra.peff.net","subject":"[PATCH 1/2] run-command: provide in_async query function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T20:22:43Z","receivedAt":"2015-09-01T20:22:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's not easy for arbitrary code to find out whether it is\nrunning in an async process or not. A top-level function\nwhich is fed to start_async() can know (you just pass down\nan argument saying \"you are async\"). But that function may\ncall other global functions, and we would not want to have\nto pass the information all the way through the call stack.\n\nNor can we simply set a global variable, as those may be\nshared between async threads and the main thread (if the\nplatform supports pthreads). We need pthread tricks _or_ a\nglobal variable, depending on how start_async is\nimplemented.\n\nThe callers don't have enough information to do this right,\nso let's provide a simple query function that does.\nFortunately we can reuse the existing infrastructure to make\nthe pthread case simple (and even simplify die_async() by\nusing our new function).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c | 16 +++++++++++++++-\n run-command.h |  1 +\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3277cf7..c8029f2 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n \n-\tif (!pthread_equal(main_thread, pthread_self())) {\n+\tif (in_async()) {\n \t\tstruct async *async = pthread_getspecific(async_key);\n \t\tif (async->proc_in >= 0)\n \t\t\tclose(async->proc_in);\n@@ -614,6 +614,13 @@ static int async_die_is_recursing(void)\n \treturn ret != NULL;\n }\n \n+int in_async(void)\n+{\n+\tif (!main_thread_set)\n+\t\treturn 0; /* no asyncs started yet */\n+\treturn !pthread_equal(main_thread, pthread_self());\n+}\n+\n #else\n \n static struct {\n@@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))\n }\n #define atexit git_atexit\n \n+static int process_is_async;\n+int in_async(void)\n+{\n+\treturn process_is_async;\n+}\n+\n #endif\n \n int start_async(struct async *async)\n@@ -712,6 +725,7 @@ int start_async(struct async *async)\n \t\tif (need_out)\n \t\t\tclose(fdout[0]);\n \t\tgit_atexit_clear();\n+\t\tprocess_is_async = 1;\n \t\texit(!!async->proc(proc_in, proc_out, async->data));\n \t}\n \ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a..629fab7 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -118,5 +118,6 @@ struct async {\n \n int start_async(struct async *async);\n int finish_async(struct async *async);\n+int in_async(void);\n \n #endif\n-- \n2.5.1.739.g7891f6b\n"},{"id":"269150","messageId":"20150901202412.GA8020@sigill.intra.peff.net","threadId":"40245","inReplyTo":"20150901202215.GA17370@sigill.intra.peff.net","subject":"[PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T20:24:13Z","receivedAt":"2015-09-01T20:24:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run \"GIT_TRACE_PACKET=1 git push\", you may get\nconfusing output like (line prefixes omitted for clarity):\n\n   packet:      push< \\1000eunpack ok0019ok refs/heads/master0000\n   packet:      push< unpack ok\n   packet:      push< ok refs/heads/master\n   packet:      push< 0000\n   packet:      push< 0000\n\nWhy do we see the data twice, once apparently wrapped inside\nanother pkt-line, and once unwrapped? Why do we get two\nflush packets?\n\nThe answer is that we start an async process to demux the\nsideband data. The first entry comes from the sideband\nprocess reading the data, and the second from push itself.\nLikewise, the first flush is inside the demuxed packet, and\nthe second is an actual sideband flush.\n\nWe can make this a bit more clear by marking the sideband\ndemuxer explicitly as \"sideband\" rather than \"push\". The\nmost elegant way to do this would be to simply call\npacket_trace_identity() inside the sideband demuxer. But we\ncan't do that reliably, because it relies on a global\nvariable, which might be shared if pthreads are in use.\n\nWhat we really need is thread-local storage for\npacket_trace_identity. But the async code does not provide\nan interface for that, and it would be messy to add it here\n(we'd have to care about pthreads, initializing our\npthread_key_t ahead of time, etc).\n\nSo instead, let us just assume that any async process is\nhandling sideband data. That's always true now, and is\nlikely to remain so in the future.\n\nThe output looks like:\n\n   packet:  sideband< \\1000eunpack ok0019ok refs/heads/master0000\n   packet:      push< unpack ok\n   packet:      push< ok refs/heads/master\n   packet:      push< 0000\n   packet:  sideband< 0000\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pkt-line.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 08a1427..62fdb37 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"pkt-line.h\"\n+#include \"run-command.h\"\n \n char packet_buffer[LARGE_PACKET_MAX];\n static const char *packet_trace_prefix = \"git\";\n@@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)\n \tpacket_trace_prefix = xstrdup(prog);\n }\n \n+static const char *get_trace_prefix(void)\n+{\n+\treturn in_async() ? \"sideband\" : packet_trace_prefix;\n+}\n+\n static int packet_trace_pack(const char *buf, unsigned int len, int sideband)\n {\n \tif (!sideband) {\n@@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_init(&out, len+32);\n \n \tstrbuf_addf(&out, \"packet: %12s%c \",\n-\t\t    packet_trace_prefix, write ? '>' : '<');\n+\t\t    get_trace_prefix(), write ? '>' : '<');\n \n \t/* XXX we should really handle printable utf8 */\n \tfor (i = 0; i < len; i++) {\n-- \n2.5.1.739.g7891f6b\n"},{"id":"269151","messageId":"20150901202609.GA8063@sigill.intra.peff.net","threadId":"40245","inReplyTo":"20150901202243.GA6931@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] run-command: provide in_async query function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T20:26:10Z","receivedAt":"2015-09-01T20:26:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 04:22:43PM -0400, Jeff King wrote:\n\n> diff --git a/run-command.c b/run-command.c\n> index 3277cf7..c8029f2 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n\nThis diff is a little funny to read because the interesting context is\nfar away, and it is not immediately obvious why we are defining the\nfunction twice. This:\n\n> @@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list params)\n>  {\n>  \tvreportf(\"fatal: \", err, params);\n>  \n> -\tif (!pthread_equal(main_thread, pthread_self())) {\n> +\tif (in_async()) {\n>  \t\tstruct async *async = pthread_getspecific(async_key);\n>  \t\tif (async->proc_in >= 0)\n>  \t\t\tclose(async->proc_in);\n> @@ -614,6 +614,13 @@ static int async_die_is_recursing(void)\n>  \treturn ret != NULL;\n>  }\n>  \n> +int in_async(void)\n> +{\n> +\tif (!main_thread_set)\n> +\t\treturn 0; /* no asyncs started yet */\n> +\treturn !pthread_equal(main_thread, pthread_self());\n> +}\n> +\n>  #else\n\nis all inside #ifndef NO_PTHREADS, and this:\n\n> @@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))\n>  }\n>  #define atexit git_atexit\n>  \n> +static int process_is_async;\n> +int in_async(void)\n> +{\n> +\treturn process_is_async;\n> +}\n> +\n>  #endif\n\nis the \"#else\" case where we fork.\n\n-Peff\n"},{"id":"269162","messageId":"xmqq613trdu3.fsf@gitster.mtv.corp.google.com","threadId":"40245","inReplyTo":"20150901202243.GA6931@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] run-command: provide in_async query function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-01T22:09:56Z","receivedAt":"2015-09-01T22:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It's not easy for arbitrary code to find out whether it is\n> running in an async process or not. A top-level function\n> which is fed to start_async() can know (you just pass down\n> an argument saying \"you are async\"). But that function may\n> call other global functions, and we would not want to have\n> to pass the information all the way through the call stack.\n>\n> Nor can we simply set a global variable, as those may be\n> shared between async threads and the main thread (if the\n> platform supports pthreads). We need pthread tricks _or_ a\n> global variable, depending on how start_async is\n> implemented.\n>\n> The callers don't have enough information to do this right,\n> so let's provide a simple query function that does.\n> Fortunately we can reuse the existing infrastructure to make\n> the pthread case simple (and even simplify die_async() by\n> using our new function).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nWhat is not immediately obvious from the above description is why a\ncode may want to care if it is in_async() in the first place.\n\nIf there weren't the die_async() update, the readers might have been\nleft utterly baffled (or they can somehow see this is related to\n2/2) but it is a bit hard to arrange in \"git log\" as going to child\nis harder.\n\nThe patch looks good.  Thanks.\n\n>  run-command.c | 16 +++++++++++++++-\n>  run-command.h |  1 +\n>  2 files changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 3277cf7..c8029f2 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list params)\n>  {\n>  \tvreportf(\"fatal: \", err, params);\n>  \n> -\tif (!pthread_equal(main_thread, pthread_self())) {\n> +\tif (in_async()) {\n>  \t\tstruct async *async = pthread_getspecific(async_key);\n>  \t\tif (async->proc_in >= 0)\n>  \t\t\tclose(async->proc_in);\n> @@ -614,6 +614,13 @@ static int async_die_is_recursing(void)\n>  \treturn ret != NULL;\n>  }\n>  \n> +int in_async(void)\n> +{\n> +\tif (!main_thread_set)\n> +\t\treturn 0; /* no asyncs started yet */\n> +\treturn !pthread_equal(main_thread, pthread_self());\n> +}\n> +\n>  #else\n>  \n>  static struct {\n> @@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))\n>  }\n>  #define atexit git_atexit\n>  \n> +static int process_is_async;\n> +int in_async(void)\n> +{\n> +\treturn process_is_async;\n> +}\n> +\n>  #endif\n>  \n>  int start_async(struct async *async)\n> @@ -712,6 +725,7 @@ int start_async(struct async *async)\n>  \t\tif (need_out)\n>  \t\t\tclose(fdout[0]);\n>  \t\tgit_atexit_clear();\n> +\t\tprocess_is_async = 1;\n>  \t\texit(!!async->proc(proc_in, proc_out, async->data));\n>  \t}\n>  \n> diff --git a/run-command.h b/run-command.h\n> index 5b4425a..629fab7 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -118,5 +118,6 @@ struct async {\n>  \n>  int start_async(struct async *async);\n>  int finish_async(struct async *async);\n> +int in_async(void);\n>  \n>  #endif\n"},{"id":"269163","messageId":"xmqq1tehrdoa.fsf@gitster.mtv.corp.google.com","threadId":"40245","inReplyTo":"20150901202412.GA8020@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-01T22:13:25Z","receivedAt":"2015-09-01T22:13:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> What we really need is thread-local storage for\n> packet_trace_identity. But the async code does not provide\n> an interface for that, and it would be messy to add it here\n> (we'd have to care about pthreads, initializing our\n> pthread_key_t ahead of time, etc).\n\nTrue.\n\n> So instead, let us just assume that any async process is\n> handling sideband data. That's always true now, and is\n> likely to remain so in the future.\n\nHmm, does Stefan's thread-pool thing interact with this decision in\nany way?\n>\n> The output looks like:\n>\n>    packet:  sideband< \\1000eunpack ok0019ok refs/heads/master0000\n>    packet:      push< unpack ok\n>    packet:      push< ok refs/heads/master\n>    packet:      push< 0000\n>    packet:  sideband< 0000\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  pkt-line.c | 8 +++++++-\n>  1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 08a1427..62fdb37 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -1,5 +1,6 @@\n>  #include \"cache.h\"\n>  #include \"pkt-line.h\"\n> +#include \"run-command.h\"\n>  \n>  char packet_buffer[LARGE_PACKET_MAX];\n>  static const char *packet_trace_prefix = \"git\";\n> @@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)\n>  \tpacket_trace_prefix = xstrdup(prog);\n>  }\n>  \n> +static const char *get_trace_prefix(void)\n> +{\n> +\treturn in_async() ? \"sideband\" : packet_trace_prefix;\n> +}\n> +\n>  static int packet_trace_pack(const char *buf, unsigned int len, int sideband)\n>  {\n>  \tif (!sideband) {\n> @@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n>  \tstrbuf_init(&out, len+32);\n>  \n>  \tstrbuf_addf(&out, \"packet: %12s%c \",\n> -\t\t    packet_trace_prefix, write ? '>' : '<');\n> +\t\t    get_trace_prefix(), write ? '>' : '<');\n>  \n>  \t/* XXX we should really handle printable utf8 */\n>  \tfor (i = 0; i < len; i++) {\n"},{"id":"269166","messageId":"20150901221705.GA7862@sigill.intra.peff.net","threadId":"40245","inReplyTo":"xmqq613trdu3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] run-command: provide in_async query function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T22:17:05Z","receivedAt":"2015-09-01T22:17:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 03:09:56PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > It's not easy for arbitrary code to find out whether it is\n> > running in an async process or not. A top-level function\n> > which is fed to start_async() can know (you just pass down\n> > an argument saying \"you are async\"). But that function may\n> > call other global functions, and we would not want to have\n> > to pass the information all the way through the call stack.\n> >\n> > Nor can we simply set a global variable, as those may be\n> > shared between async threads and the main thread (if the\n> > platform supports pthreads). We need pthread tricks _or_ a\n> > global variable, depending on how start_async is\n> > implemented.\n> >\n> > The callers don't have enough information to do this right,\n> > so let's provide a simple query function that does.\n> > Fortunately we can reuse the existing infrastructure to make\n> > the pthread case simple (and even simplify die_async() by\n> > using our new function).\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> \n> What is not immediately obvious from the above description is why a\n> code may want to care if it is in_async() in the first place.\n> \n> If there weren't the die_async() update, the readers might have been\n> left utterly baffled (or they can somehow see this is related to\n> 2/2) but it is a bit hard to arrange in \"git log\" as going to child\n> is harder.\n> \n> The patch looks good.  Thanks.\n\nYeah, I almost mentioned \"...in the next patch we'll need this\", but I\nwasn't sure how to bring that up without going into the complicated\nreasoning in that patch. I don't know if it's worth adding \"we don't\nhave any callers yet, but we will add one in a moment\".\n\n-Peff\n"},{"id":"269168","messageId":"20150901222230.GB7862@sigill.intra.peff.net","threadId":"40245","inReplyTo":"xmqq1tehrdoa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T22:22:30Z","receivedAt":"2015-09-01T22:22:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 03:13:25PM -0700, Junio C Hamano wrote:\n\n> > So instead, let us just assume that any async process is\n> > handling sideband data. That's always true now, and is\n> > likely to remain so in the future.\n> \n> Hmm, does Stefan's thread-pool thing interact with this decision in\n> any way?\n\nI don't think so. It is true that:\n\n  start_async(...);\n  finish_async(...);\n  pthread_create(foo);\n\nwould incorrectly report in_async() inside the foo function (because we\nknow that async has been kicked off, and we know that we are in a thread\nthat is not the main one).\n\nBut I don't think it matters in the current code base, because we tend\nto use async for I/O tasks like muxing/demuxing, and real threads in\nCPU-heavy tasks like pack-objects.\n\nStill, I admit it is a little gross, and may be a problem in the future.\n\nAn alternate approach would be to have the async system provide a\nthread-local storage abstraction. With pthreads, we'd build on\npthread_setspecific(), and without, it becomes a simple global variable.\n\nThat's more work, but a lot less error-prone, and it may come in handy\nin the future.\n\n-Peff\n"},{"id":"269169","messageId":"CAGZ79kZKJF2epU=2gb7jFWni2tBu03dYGgttvZeW40ra=r2iiA@mail.gmail.com","threadId":"40245","inReplyTo":"xmqq1tehrdoa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T22:23:06Z","receivedAt":"2015-09-01T22:23:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 1, 2015 at 3:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> What we really need is thread-local storage for\n>> packet_trace_identity. But the async code does not provide\n>> an interface for that, and it would be messy to add it here\n>> (we'd have to care about pthreads, initializing our\n>> pthread_key_t ahead of time, etc).\n>\n> True.\n>\n>> So instead, let us just assume that any async process is\n>> handling sideband data. That's always true now, and is\n>> likely to remain so in the future.\n>\n> Hmm, does Stefan's thread-pool thing interact with this decision in\n> any way?\n\nI do not plan to actually fetch from inside the thread pool, but each thread\nis just a proxy for starting a new process doing the fetch and getting\nthe output\nin order.\n\nThat seems to be the least amount of work to me. Very long term we may want to\ndo the fetch directly in a thread pool worker, but then we can also\nadd the thread\nlocal storage interface.\n\n>>\n>> The output looks like:\n>>\n>>    packet:  sideband< \\1000eunpack ok0019ok refs/heads/master0000\n>>    packet:      push< unpack ok\n>>    packet:      push< ok refs/heads/master\n>>    packet:      push< 0000\n>>    packet:  sideband< 0000\n>>\n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>>  pkt-line.c | 8 +++++++-\n>>  1 file changed, 7 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/pkt-line.c b/pkt-line.c\n>> index 08a1427..62fdb37 100644\n>> --- a/pkt-line.c\n>> +++ b/pkt-line.c\n>> @@ -1,5 +1,6 @@\n>>  #include \"cache.h\"\n>>  #include \"pkt-line.h\"\n>> +#include \"run-command.h\"\n>>\n>>  char packet_buffer[LARGE_PACKET_MAX];\n>>  static const char *packet_trace_prefix = \"git\";\n>> @@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)\n>>       packet_trace_prefix = xstrdup(prog);\n>>  }\n>>\n>> +static const char *get_trace_prefix(void)\n>> +{\n>> +     return in_async() ? \"sideband\" : packet_trace_prefix;\n>> +}\n>> +\n>>  static int packet_trace_pack(const char *buf, unsigned int len, int sideband)\n>>  {\n>>       if (!sideband) {\n>> @@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n>>       strbuf_init(&out, len+32);\n>>\n>>       strbuf_addf(&out, \"packet: %12s%c \",\n>> -                 packet_trace_prefix, write ? '>' : '<');\n>> +                 get_trace_prefix(), write ? '>' : '<');\n>>\n>>       /* XXX we should really handle printable utf8 */\n>>       for (i = 0; i < len; i++) {\n"},{"id":"269172","messageId":"20150901222639.GD7862@sigill.intra.peff.net","threadId":"40245","inReplyTo":"CAGZ79kZKJF2epU=2gb7jFWni2tBu03dYGgttvZeW40ra=r2iiA@mail.gmail.com","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T22:26:39Z","receivedAt":"2015-09-01T22:26:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 03:23:06PM -0700, Stefan Beller wrote:\n\n> > Hmm, does Stefan's thread-pool thing interact with this decision in\n> > any way?\n> \n> I do not plan to actually fetch from inside the thread pool, but each thread\n> is just a proxy for starting a new process doing the fetch and getting\n> the output\n> in order.\n\nAh, right, I think I misunderstood Junio's question. Yes, if we start\ncalling cmd_fetch() from inside the threads, things may get confusing.\n\nI'll see how painful the thread storage approach would be.\n\n-Peff\n"},{"id":"269173","messageId":"CAGZ79kYoWWc34GOxyAi5WvYf=UQvt--qwEr_m+bZwKO-uVqkiw@mail.gmail.com","threadId":"40245","inReplyTo":"20150901222639.GD7862@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T22:31:41Z","receivedAt":"2015-09-01T22:31:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 1, 2015 at 3:26 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Sep 01, 2015 at 03:23:06PM -0700, Stefan Beller wrote:\n>\n>> > Hmm, does Stefan's thread-pool thing interact with this decision in\n>> > any way?\n>>\n>> I do not plan to actually fetch from inside the thread pool, but each thread\n>> is just a proxy for starting a new process doing the fetch and getting\n>> the output\n>> in order.\n>\n> Ah, right, I think I misunderstood Junio's question. Yes, if we start\n> calling cmd_fetch() from inside the threads, things may get confusing.\n>\n> I'll see how painful the thread storage approach would be.\n\nI think that may be part of the thread pool API eventually.\n\nI don't plan on relying on thread local storage, which is why I did\nnot add it there,\nthough I guess it can be added in there quite conveniently.\n\n>\n> -Peff\n"},{"id":"269176","messageId":"20150901223807.GF7862@sigill.intra.peff.net","threadId":"40245","inReplyTo":"CAGZ79kYoWWc34GOxyAi5WvYf=UQvt--qwEr_m+bZwKO-uVqkiw@mail.gmail.com","subject":"Re: [PATCH 2/2] pkt-line: show packets in async processes as \"sideband\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T22:38:08Z","receivedAt":"2015-09-01T22:38:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 03:31:41PM -0700, Stefan Beller wrote:\n\n> > Ah, right, I think I misunderstood Junio's question. Yes, if we start\n> > calling cmd_fetch() from inside the threads, things may get confusing.\n> >\n> > I'll see how painful the thread storage approach would be.\n> \n> I think that may be part of the thread pool API eventually.\n> \n> I don't plan on relying on thread local storage, which is why I did\n> not add it there,\n> though I guess it can be added in there quite conveniently.\n\nI think the task_queue and the async code are two separate things,\nthough. The task_queue, when we do not have threads, falls back to doing\nthings serially in a single process. Changing \"thread local\" variables\nthere is OK, but tasks need to be aware that they might affect\nsubsequent tasks.\n\nWhereas for async code, we fall back to doing things in a forked\nsub-process. And there \"thread local\" really is local to the async\nprocess, no cleanup required.\n\n-Peff\n"}]}