{"thread":{"id":"45404","subject":"[PATCH] grep: fix build with no thread support","startedAt":"2017-03-17T05:48:34Z","lastAt":"2017-03-20T05:55:17Z","messageCount":12,"participants":["Rahul Bedarkar","Junio C Hamano","Brandon Williams","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"314311","messageId":"1489729656-17709-1-git-send-email-rahul.bedarkar@imgtec.com","threadId":"45404","inReplyTo":null,"subject":"[PATCH] grep: fix build with no thread support","fromName":"Rahul Bedarkar","fromEmail":"rahul.bedarkar@imgtec.com","sentAt":"2017-03-17T05:47:36Z","receivedAt":"2017-03-17T05:48:34Z","isPatch":true,"sender":{"key":"rahul.bedarkar@imgtec.com","avatar":null},"body":"Commit 0281e487fd91 (\"grep: optionally recurse into submodules\")\nadded functions grep_submodule() and grep_submodule_launch() which\nuses \"struct work_item\" which is defined only when thread support\nis available.\n\nWhen building git with toolchain that don't have thread support, we\nget following build errors:\n\n CC builtin/hash-object.o\n builtin/grep.c: In function 'grep_submodule_launch':\n builtin/grep.c:596:34: error: dereferencing pointer to incomplete type 'struct work_item'\n status = capture_command(&cp, &w->out, 0);\n ^\n builtin/grep.c: In function 'grep_submodule':\n builtin/grep.c:644:20: error: storage size of 'w' isn't known\n struct work_item w;\n ^\n make[2]: *** [builtin/grep.o] Error 1\n make[2]: *** Waiting for unfinished jobs....\n\nThis commit fixes above build issue by using \"struct grep_source\" and\n\"struct strbuf\" instead of \"struct work_item\" in grep_submodule() and\ngrep_submodule_launch() when thread support is not available.\n\nThis build failure is detected by Buildroot Autobuilder:\nhttp://autobuild.buildroot.net/results/94b/94bce9a99a5ce9894a6918774ab75e23d12c1394/\n\nCc: Brandon Williams \nFixes: 0281e487fd91 (\"grep: optionally recurse into submodules\")\nSigned-off-by: Rahul Bedarkar \n---\n\nThis patch is tested by running t7814-grep-recurse-submodules.sh with\nboth with/without thread support builds.\n\n builtin/grep.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef..4373d79 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -183,11 +183,13 @@ static void *run(void *arg)\n \t\tif (!w)\n \t\t\tbreak;\n \n-\t\topt->output_priv = w;\n-\t\tif (w->source.type == GREP_SOURCE_SUBMODULE)\n+\t\tif (w->source.type == GREP_SOURCE_SUBMODULE) {\n+\t\t\topt->output_priv = &w->out;\n \t\t\thit |= grep_submodule_launch(opt, &w->source);\n-\t\telse\n+\t\t} else {\n+\t\t\topt->output_priv = w;\n \t\t\thit |= grep_source(opt, &w->source);\n+\t\t}\n \t\tgrep_source_clear_data(&w->source);\n \t\twork_done(w);\n \t}\n@@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tint status, i;\n \tconst char *end_of_base;\n \tconst char *name;\n-\tstruct work_item *w = opt->output_priv;\n+\tstruct strbuf *w = opt->output_priv;\n \n \tend_of_base = strchr(gs->name, ':');\n \tif (gs->identifier && end_of_base)\n@@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \t * child process. A '0' indicates a hit, a '1' indicates no hit and\n \t * anything else is an error.\n \t */\n-\tstatus = capture_command(&cp, &w->out, 0);\n+\tstatus = capture_command(&cp, w, 0);\n \tif (status && (status != 1)) {\n \t\t/* flush the buffer */\n-\t\twrite_or_die(1, w->out.buf, w->out.len);\n+\t\twrite_or_die(1, w->buf, w->len);\n \t\tdie(\"process for submodule '%s' failed with exit code: %d\",\n \t\t gs->name, status);\n \t}\n@@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,\n \t} else\n #endif\n \t{\n-\t\tstruct work_item w;\n+\t\tstruct grep_source gs;\n \t\tint hit;\n+\t\tstruct strbuf outbuf = STRBUF_INIT;\n \n-\t\tgrep_source_init(&w.source, GREP_SOURCE_SUBMODULE,\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SUBMODULE,\n \t\t\t\t filename, path, sha1);\n-\t\tstrbuf_init(&w.out, 0);\n-\t\topt->output_priv = &w;\n-\t\thit = grep_submodule_launch(opt, &w.source);\n+\t\topt->output_priv = &outbuf;\n+\t\thit = grep_submodule_launch(opt, &gs);\n \n-\t\twrite_or_die(1, w.out.buf, w.out.len);\n+\t\twrite_or_die(1, outbuf.buf, outbuf.len);\n \n-\t\tgrep_source_clear(&w.source);\n-\t\tstrbuf_release(&w.out);\n+\t\tgrep_source_clear(&gs);\n+\t\tstrbuf_release(&outbuf);\n \t\treturn hit;\n \t}\n }\n-- \n2.6.2\n\n"},{"id":"314365","messageId":"xmqqy3w37ptd.fsf@gitster.mtv.corp.google.com","threadId":"45404","inReplyTo":"1489729656-17709-1-git-send-email-rahul.bedarkar@imgtec.com","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T17:53:18Z","receivedAt":"2017-03-17T17:53:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rahul Bedarkar writes:\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 2c727ef..4373d79 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -183,11 +183,13 @@ static void *run(void *arg)\n> \t\tif (!w)\n> \t\t\tbreak;\n> \n> -\t\topt->output_priv = w;\n> -\t\tif (w->source.type == GREP_SOURCE_SUBMODULE)\n> +\t\tif (w->source.type == GREP_SOURCE_SUBMODULE) {\n> +\t\t\topt->output_priv = &w->out;\n> \t\t\thit |= grep_submodule_launch(opt, &w->source);\n> -\t\telse\n> +\t\t} else {\n> +\t\t\topt->output_priv = w;\n> \t\t\thit |= grep_source(opt, &w->source);\n> +\t\t}\n> \t\tgrep_source_clear_data(&w->source);\n> \t\twork_done(w);\n> \t}\n\nThis is not a part of the \"fix\" but merely a code clean-up, right?\nJust double-checking.\n\n> @@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n> \tint status, i;\n> \tconst char *end_of_base;\n> \tconst char *name;\n> -\tstruct work_item *w = opt->output_priv;\n> +\tstruct strbuf *w = opt->output_priv;\n> \n> \tend_of_base = strchr(gs->name, ':');\n> \tif (gs->identifier && end_of_base)\n\nOK, so the new code points output_priv at a strbuf; work_item\ncontains an \"out\" strbuf, and that was why the original code was\npassing one, but this codepath does not need a full work_item to\nwork with. Is that what is going on?\n\n> @@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,\n> \t * child process. A '0' indicates a hit, a '1' indicates no hit and\n> \t * anything else is an error.\n> \t */\n> -\tstatus = capture_command(&cp, &w->out, 0);\n> +\tstatus = capture_command(&cp, w, 0);\n\nAnd this is consistent with the above change.\n\n> \tif (status && (status != 1)) {\n> \t\t/* flush the buffer */\n> -\t\twrite_or_die(1, w->out.buf, w->out.len);\n> +\t\twrite_or_die(1, w->buf, w->len);\n\nSo is this.\n\n> \t\tdie(\"process for submodule '%s' failed with exit code: %d\",\n> \t\t gs->name, status);\n> \t}\n> @@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,\n> \t} else\n> #endif\n> \t{\n> -\t\tstruct work_item w;\n> +\t\tstruct grep_source gs;\n> \t\tint hit;\n> +\t\tstruct strbuf outbuf = STRBUF_INIT;\n> \n> -\t\tgrep_source_init(&w.source, GREP_SOURCE_SUBMODULE,\n> +\t\tgrep_source_init(&gs, GREP_SOURCE_SUBMODULE,\n> \t\t\t\t filename, path, sha1);\n\nLikewise for w.source that happened to have grep_source, but passing\na bare grep_source is sufficient for the purpose of this codepath,\nwithout giving it to w.source? \n\nI didn't look at code that this patch does not touch, but if\nanything still looks at w.out and w.source and expect these to\ncontain the string accumulated in the strbuf and the grep source the\nwork item is working on, they will get broken by this change, no?\n\nThe first hunk that had a pure clean-up shows that w->source being\nthe correct grep source seems to matter.\n\n> -\t\tstrbuf_init(&w.out, 0);\n> -\t\topt->output_priv = &w;\n> -\t\thit = grep_submodule_launch(opt, &w.source);\n> +\t\topt->output_priv = &outbuf;\n> +\t\thit = grep_submodule_launch(opt, &gs);\n> \n> -\t\twrite_or_die(1, w.out.buf, w.out.len);\n> +\t\twrite_or_die(1, outbuf.buf, outbuf.len);\n> \n> -\t\tgrep_source_clear(&w.source);\n> -\t\tstrbuf_release(&w.out);\n> +\t\tgrep_source_clear(&gs);\n> +\t\tstrbuf_release(&outbuf);\n> \t\treturn hit;\n> \t}\n> }\n"},{"id":"314372","messageId":"20170317182403.GA110341@google.com","threadId":"45404","inReplyTo":"xmqqy3w37ptd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T18:24:03Z","receivedAt":"2017-03-17T18:25:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Junio C Hamano wrote:\n> Rahul Bedarkar writes:\n> \n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 2c727ef..4373d79 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -183,11 +183,13 @@ static void *run(void *arg)\n> > \t\tif (!w)\n> > \t\t\tbreak;\n> > \n> > -\t\topt->output_priv = w;\n> > -\t\tif (w->source.type == GREP_SOURCE_SUBMODULE)\n> > +\t\tif (w->source.type == GREP_SOURCE_SUBMODULE) {\n> > +\t\t\topt->output_priv = &w->out;\n> > \t\t\thit |= grep_submodule_launch(opt, &w->source);\n> > -\t\telse\n> > +\t\t} else {\n> > +\t\t\topt->output_priv = w;\n> > \t\t\thit |= grep_source(opt, &w->source);\n> > +\t\t}\n> > \t\tgrep_source_clear_data(&w->source);\n> > \t\twork_done(w);\n> > \t}\n> \n> This is not a part of the \"fix\" but merely a code clean-up, right?\n> Just double-checking.\n> \n> > @@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n> > \tint status, i;\n> > \tconst char *end_of_base;\n> > \tconst char *name;\n> > -\tstruct work_item *w = opt->output_priv;\n> > +\tstruct strbuf *w = opt->output_priv;\n> > \n> > \tend_of_base = strchr(gs->name, ':');\n> > \tif (gs->identifier && end_of_base)\n> \n> OK, so the new code points output_priv at a strbuf; work_item\n> contains an \"out\" strbuf, and that was why the original code was\n> passing one, but this codepath does not need a full work_item to\n> work with. Is that what is going on?\n> \n> > @@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,\n> > \t * child process. A '0' indicates a hit, a '1' indicates no hit and\n> > \t * anything else is an error.\n> > \t */\n> > -\tstatus = capture_command(&cp, &w->out, 0);\n> > +\tstatus = capture_command(&cp, w, 0);\n> \n> And this is consistent with the above change.\n> \n> > \tif (status && (status != 1)) {\n> > \t\t/* flush the buffer */\n> > -\t\twrite_or_die(1, w->out.buf, w->out.len);\n> > +\t\twrite_or_die(1, w->buf, w->len);\n> \n> So is this.\n> \n> > \t\tdie(\"process for submodule '%s' failed with exit code: %d\",\n> > \t\t gs->name, status);\n> > \t}\n> > @@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,\n> > \t} else\n> > #endif\n> > \t{\n> > -\t\tstruct work_item w;\n> > +\t\tstruct grep_source gs;\n> > \t\tint hit;\n> > +\t\tstruct strbuf outbuf = STRBUF_INIT;\n> > \n> > -\t\tgrep_source_init(&w.source, GREP_SOURCE_SUBMODULE,\n> > +\t\tgrep_source_init(&gs, GREP_SOURCE_SUBMODULE,\n> > \t\t\t\t filename, path, sha1);\n> \n> Likewise for w.source that happened to have grep_source, but passing\n> a bare grep_source is sufficient for the purpose of this codepath,\n> without giving it to w.source? \n> \n> I didn't look at code that this patch does not touch, but if\n> anything still looks at w.out and w.source and expect these to\n> contain the string accumulated in the strbuf and the grep source the\n> work item is working on, they will get broken by this change, no?\n> \n> The first hunk that had a pure clean-up shows that w->source being\n> the correct grep source seems to matter.\n> \n> > -\t\tstrbuf_init(&w.out, 0);\n> > -\t\topt->output_priv = &w;\n> > -\t\thit = grep_submodule_launch(opt, &w.source);\n> > +\t\topt->output_priv = &outbuf;\n> > +\t\thit = grep_submodule_launch(opt, &gs);\n> > \n> > -\t\twrite_or_die(1, w.out.buf, w.out.len);\n> > +\t\twrite_or_die(1, outbuf.buf, outbuf.len);\n> > \n> > -\t\tgrep_source_clear(&w.source);\n> > -\t\tstrbuf_release(&w.out);\n> > +\t\tgrep_source_clear(&gs);\n> > +\t\tstrbuf_release(&outbuf);\n> > \t\treturn hit;\n> > \t}\n> > }\n\nThanks for pointing out the bug (there seem to just be more and more in\nthis grep code...but what else can you expect from my first\ncontribution! :D) but I think it would be better to make the way the\nrecursive code output's more consistent with the way the rest of the\ngrep handles output. That way we can get rid of some of the special\ncasing that I had done here.\n\nI'll send out what I have locally in just a second.\n\n-- \nBrandon Williams\n"},{"id":"314374","messageId":"20170317184155.128674-1-bmwill@google.com","threadId":"45404","inReplyTo":"20170317182403.GA110341@google.com","subject":"[PATCH 1/2] grep: set default output method","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T18:41:54Z","receivedAt":"2017-03-17T18:43:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams \n---\n grep.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0dbdc1d00..56ef0ecbf 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -12,6 +12,11 @@ static int grep_source_is_binary(struct grep_source *gs);\n \n static struct grep_opt grep_defaults;\n \n+static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n+{\n+\tfwrite(buf, size, 1, stdout);\n+}\n+\n /*\n * Initialize the grep_defaults template with hardcoded defaults.\n * We could let the compiler do this, but without C99 initializers\n@@ -42,6 +47,7 @@ void init_grep_defaults(void)\n \tcolor_set(opt->color_selected, \"\");\n \tcolor_set(opt->color_sep, GIT_COLOR_CYAN);\n \topt->color = -1;\n+\topt->output = std_output;\n }\n \n static int parse_pattern_type_arg(const char *opt, const char *arg)\n@@ -152,6 +158,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \topt->pathname = def->pathname;\n \topt->regflags = def->regflags;\n \topt->relative = def->relative;\n+\topt->output = def->output;\n \n \tcolor_set(opt->color_context, def->color_context);\n \tcolor_set(opt->color_filename, def->color_filename);\n@@ -1379,11 +1386,6 @@ static int look_ahead(struct grep_opt *opt,\n \treturn 0;\n }\n \n-static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n-{\n-\tfwrite(buf, size, 1, stdout);\n-}\n-\n static int fill_textconv_grep(struct userdiff_driver *driver,\n \t\t\t struct grep_source *gs)\n {\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314375","messageId":"20170317184155.128674-2-bmwill@google.com","threadId":"45404","inReplyTo":"20170317184155.128674-1-bmwill@google.com","subject":"[PATCH 2/2] grep: fix builds with with no thread support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T18:41:55Z","receivedAt":"2017-03-17T18:43:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit 0281e487fd91 (\"grep: optionally recurse into submodules\")\nadded functions grep_submodule() and grep_submodule_launch() which\nuses \"struct work_item\" which is defined only when thread support\nis available.\n\nThe original implementation of grep_submodule() used the \"struct\nwork_item\" in order to gain access to a strbuf to store its output which\nwas to be printed at a later point in time. This differs from how both\ngrep_file() and grep_sha1() handle their output. This patch eliminates\nthe reliance on the \"struct work_item\" and instead opts to use the\noutput function stored in the output field of the \"struct grep_opt\"\nobject directly, making it behave similarly to both grep_file() and\ngrep_sha1().\n\nReported-by: Rahul Bedarkar \nSigned-off-by: Brandon Williams \n---\n builtin/grep.c | 21 +++++++++------------\n 1 file changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9304c33e7..3f1251bab 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -538,7 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tint status, i;\n \tconst char *end_of_base;\n \tconst char *name;\n-\tstruct work_item *w = opt->output_priv;\n+\tstruct strbuf child_output = STRBUF_INIT;\n \n \tend_of_base = strchr(gs->name, ':');\n \tif (gs->identifier && end_of_base)\n@@ -593,14 +593,16 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \t * child process. A '0' indicates a hit, a '1' indicates no hit and\n \t * anything else is an error.\n \t */\n-\tstatus = capture_command(&cp, &w->out, 0);\n+\tstatus = capture_command(&cp, &child_output, 0);\n \tif (status && (status != 1)) {\n \t\t/* flush the buffer */\n-\t\twrite_or_die(1, w->out.buf, w->out.len);\n+\t\twrite_or_die(1, child_output.buf, child_output.len);\n \t\tdie(\"process for submodule '%s' failed with exit code: %d\",\n \t\t gs->name, status);\n \t}\n \n+\topt->output(opt, child_output.buf, child_output.len);\n+\tstrbuf_release(&child_output);\n \t/* invert the return code to make a hit equal to 1 */\n \treturn !status;\n }\n@@ -641,19 +643,14 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,\n \t} else\n #endif\n \t{\n-\t\tstruct work_item w;\n+\t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&w.source, GREP_SOURCE_SUBMODULE,\n+\t\tgrep_source_init(&gs, GREP_SOURCE_SUBMODULE,\n \t\t\t\t filename, path, sha1);\n-\t\tstrbuf_init(&w.out, 0);\n-\t\topt->output_priv = &w;\n-\t\thit = grep_submodule_launch(opt, &w.source);\n+\t\thit = grep_submodule_launch(opt, &gs);\n \n-\t\twrite_or_die(1, w.out.buf, w.out.len);\n-\n-\t\tgrep_source_clear(&w.source);\n-\t\tstrbuf_release(&w.out);\n+\t\tgrep_source_clear(&gs);\n \t\treturn hit;\n \t}\n }\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314376","messageId":"20170317184701.GB110341@google.com","threadId":"45404","inReplyTo":"xmqqy3w37ptd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T18:47:01Z","receivedAt":"2017-03-17T18:47:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"While taking a look at this bug I discovered that the test suite doesn't\npass 100% of the test when compiled with the NO_PTHREADS option. The\nfollowing tests seem to be failing:\n\nt1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)\n Failed tests: 7-9\n Non-zero exit status: 1\nt5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)\n Failed test: 4\n Non-zero exit status: 1\nt5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)\n Failed tests: 4-5\n Non-zero exit status: 1\nt5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)\n Failed test: 10\n Non-zero exit status: 1\n\nI didn't take a close look at it but this would seem to indicate that we\ndon't worry to much about systems without pthreads support. Just food\nfor thought.\n\n-- \nBrandon Williams\n"},{"id":"314405","messageId":"20170317223741.qwfh2zw37y3jbeev@sigill.intra.peff.net","threadId":"45404","inReplyTo":"20170317184701.GB110341@google.com","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T22:37:41Z","receivedAt":"2017-03-17T22:39:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 17, 2017 at 11:47:01AM -0700, Brandon Williams wrote:\n\n> While taking a look at this bug I discovered that the test suite doesn't\n> pass 100% of the test when compiled with the NO_PTHREADS option. The\n> following tests seem to be failing:\n> \n> t1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)\n> Failed tests: 7-9\n> Non-zero exit status: 1\n> t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)\n> Failed test: 4\n> Non-zero exit status: 1\n> t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)\n> Failed tests: 4-5\n> Non-zero exit status: 1\n> t5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)\n> Failed test: 10\n> Non-zero exit status: 1\n> \n> I didn't take a close look at it but this would seem to indicate that we\n> don't worry to much about systems without pthreads support. Just food\n> for thought.\n\nHmm. We used to. What version did you test? Everything passes for me at\n0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the\nfirst release which does not work with NO_PTHREADS.\n\n-Peff\n"},{"id":"314406","messageId":"20170317224232.GE63813@google.com","threadId":"45404","inReplyTo":"20170317223741.qwfh2zw37y3jbeev@sigill.intra.peff.net","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T22:42:32Z","receivedAt":"2017-03-17T22:43:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Jeff King wrote:\n> On Fri, Mar 17, 2017 at 11:47:01AM -0700, Brandon Williams wrote:\n> \n> > While taking a look at this bug I discovered that the test suite doesn't\n> > pass 100% of the test when compiled with the NO_PTHREADS option. The\n> > following tests seem to be failing:\n> > \n> > t1060-object-corruption.sh (Wstat: 256 Tests: 13 Failed: 3)\n> > Failed tests: 7-9\n> > Non-zero exit status: 1\n> > t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)\n> > Failed test: 4\n> > Non-zero exit status: 1\n> > t5504-fetch-receive-strict.sh (Wstat: 256 Tests: 12 Failed: 2)\n> > Failed tests: 4-5\n> > Non-zero exit status: 1\n> > t5530-upload-pack-error.sh (Wstat: 256 Tests: 10 Failed: 1)\n> > Failed test: 10\n> > Non-zero exit status: 1\n> > \n> > I didn't take a close look at it but this would seem to indicate that we\n> > don't worry to much about systems without pthreads support. Just food\n> > for thought.\n> \n> Hmm. We used to. What version did you test? Everything passes for me at\n> 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the\n> first release which does not work with NO_PTHREADS.\n> \n> -Peff\n\nThe version I ran tests on was what the master branch was pointing to a\nday or so ago:\n\nv2.12.0-264-gd6db3f216\n\n-- \nBrandon Williams\n"},{"id":"314412","messageId":"20170317230000.GS26789@aiede.mtv.corp.google.com","threadId":"45404","inReplyTo":"20170317184155.128674-2-bmwill@google.com","subject":"Re: [PATCH 2/2] grep: fix builds with with no thread support","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-17T23:00:00Z","receivedAt":"2017-03-17T23:00:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> Commit 0281e487fd91 (\"grep: optionally recurse into submodules\")\n> added functions grep_submodule() and grep_submodule_launch() which\n> uses \"struct work_item\" which is defined only when thread support\n\nnit: which use (s/uses/use/)\n\n> is available.\n[...]\n> Reported-by: Rahul Bedarkar \n> Signed-off-by: Brandon Williams \n> ---\n> builtin/grep.c | 21 +++++++++------------\n> 1 file changed, 9 insertions(+), 12 deletions(-)\n\nWith or without that commit message tweak,\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"314413","messageId":"20170317230319.GF63813@google.com","threadId":"45404","inReplyTo":"20170317225847.g5776lntj7d5ni3v@sigill.intra.peff.net","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T23:03:19Z","receivedAt":"2017-03-17T23:04:50Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Jeff King wrote:\n> On Fri, Mar 17, 2017 at 03:42:32PM -0700, Brandon Williams wrote:\n> \n> > > > I didn't take a close look at it but this would seem to indicate that we\n> > > > don't worry to much about systems without pthreads support. Just food\n> > > > for thought.\n> > > \n> > > Hmm. We used to. What version did you test? Everything passes for me at\n> > > 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the\n> > > first release which does not work with NO_PTHREADS.\n> > > \n> > > -Peff\n> > \n> > The version I ran tests on was what the master branch was pointing to a\n> > day or so ago:\n> > \n> > v2.12.0-264-gd6db3f216\n> \n> Ah, OK. I couldn't get such a recent version to build with NO_PTHREADS,\n> but I assume you mean after applying your two patches.\n> \n\nOh yes, sorry I should have mentioned that was with my patches applied.\nMy bad! :D\n\n> v2.11.0 is fine, but v2.12.0 with your patches shows the problem.\n> Bisecting (and applying the patches when necessary) points to my\n> 46df6906f (execv_dashed_external: wait for child on signal death,\n> 2017-01-06).\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"314427","messageId":"20170317225847.g5776lntj7d5ni3v@sigill.intra.peff.net","threadId":"45404","inReplyTo":"20170317224232.GE63813@google.com","subject":"Re: [PATCH] grep: fix build with no thread support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-17T22:58:48Z","receivedAt":"2017-03-17T23:59:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 17, 2017 at 03:42:32PM -0700, Brandon Williams wrote:\n\n> > > I didn't take a close look at it but this would seem to indicate that we\n> > > don't worry to much about systems without pthreads support. Just food\n> > > for thought.\n> > \n> > Hmm. We used to. What version did you test? Everything passes for me at\n> > 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the\n> > first release which does not work with NO_PTHREADS.\n> > \n> > -Peff\n> \n> The version I ran tests on was what the master branch was pointing to a\n> day or so ago:\n> \n> v2.12.0-264-gd6db3f216\n\nAh, OK. I couldn't get such a recent version to build with NO_PTHREADS,\nbut I assume you mean after applying your two patches.\n\nv2.11.0 is fine, but v2.12.0 with your patches shows the problem.\nBisecting (and applying the patches when necessary) points to my\n46df6906f (execv_dashed_external: wait for child on signal death,\n2017-01-06).\n\n-Peff\n"},{"id":"314630","messageId":"91e0c631-1ce3-e76c-325b-fde0123c54b1@imgtec.com","threadId":"45404","inReplyTo":"20170317184155.128674-2-bmwill@google.com","subject":"Re: [PATCH 2/2] grep: fix builds with with no thread support","fromName":"Rahul Bedarkar","fromEmail":"rahul.bedarkar@imgtec.com","sentAt":"2017-03-20T05:55:06Z","receivedAt":"2017-03-20T05:55:17Z","isPatch":true,"sender":{"key":"rahul.bedarkar@imgtec.com","avatar":null},"body":"Hello,\n\nOn Saturday 18 March 2017 12:11 AM, Brandon Williams wrote:\n> Commit 0281e487fd91 (\"grep: optionally recurse into submodules\")\n> added functions grep_submodule() and grep_submodule_launch() which\n> uses \"struct work_item\" which is defined only when thread support\n> is available.\n>\n> The original implementation of grep_submodule() used the \"struct\n> work_item\" in order to gain access to a strbuf to store its output which\n> was to be printed at a later point in time. This differs from how both\n> grep_file() and grep_sha1() handle their output. This patch eliminates\n> the reliance on the \"struct work_item\" and instead opts to use the\n> output function stored in the output field of the \"struct grep_opt\"\n> object directly, making it behave similarly to both grep_file() and\n> grep_sha1().\n>\n> Reported-by: Rahul Bedarkar \n> Signed-off-by: Brandon Williams \n> ---\n> builtin/grep.c | 21 +++++++++------------\n> 1 file changed, 9 insertions(+), 12 deletions(-)\n>\n\nIt works for me.\n\nTested-by: Rahul Bedarkar \n\nThanks,\nRahul\n"}]}