Re: [PATCH] grep: fix build with no thread support

> Rahul Bedarkar <rahul.bedarkar@imgtec.com> writes:
> 
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 2c727ef..4373d79 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -183,11 +183,13 @@ static void *run(void *arg)
> >  		if (!w)
> >  			break;
> >  
> > -		opt->output_priv = w;
> > -		if (w->source.type == GREP_SOURCE_SUBMODULE)
> > +		if (w->source.type == GREP_SOURCE_SUBMODULE) {
> > +			opt->output_priv = &w->out;
> >  			hit |= grep_submodule_launch(opt, &w->source);
> > -		else
> > +		} else {
> > +			opt->output_priv = w;
> >  			hit |= grep_source(opt, &w->source);
> > +		}
> >  		grep_source_clear_data(&w->source);
> >  		work_done(w);
> >  	}
> 
> This is not a part of the "fix" but merely a code clean-up, right?
> Just double-checking.
> 
> > @@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
> >  	int status, i;
> >  	const char *end_of_base;
> >  	const char *name;
> > -	struct work_item *w = opt->output_priv;
> > +	struct strbuf *w = opt->output_priv;
> >  
> >  	end_of_base = strchr(gs->name, ':');
> >  	if (gs->identifier && end_of_base)
> 
> OK, so the new code points output_priv at a strbuf; work_item
> contains an "out" strbuf, and that was why the original code was
> passing one, but this codepath does not need a full work_item to
> work with.  Is that what is going on?
> 
> > @@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,
> >  	 * child process.  A '0' indicates a hit, a '1' indicates no hit and
> >  	 * anything else is an error.
> >  	 */
> > -	status = capture_command(&cp, &w->out, 0);
> > +	status = capture_command(&cp, w, 0);
> 
> And this is consistent with the above change.
> 
> >  	if (status && (status != 1)) {
> >  		/* flush the buffer */
> > -		write_or_die(1, w->out.buf, w->out.len);
> > +		write_or_die(1, w->buf, w->len);
> 
> So is this.
> 
> >  		die("process for submodule '%s' failed with exit code: %d",
> >  		    gs->name, status);
> >  	}
> > @@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> >  	} else
> >  #endif
> >  	{
> > -		struct work_item w;
> > +		struct grep_source gs;
> >  		int hit;
> > +		struct strbuf outbuf = STRBUF_INIT;
> >  
> > -		grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> > +		grep_source_init(&gs, GREP_SOURCE_SUBMODULE,
> >  				 filename, path, sha1);
> 
> Likewise for w.source that happened to have grep_source, but passing
> a bare grep_source is sufficient for the purpose of this codepath,
> without giving it to w.source?  
> 
> I didn't look at code that this patch does not touch, but if
> anything still looks at w.out and w.source and expect these to
> contain the string accumulated in the strbuf and the grep source the
> work item is working on, they will get broken by this change, no?
> 
> The first hunk that had a pure clean-up shows that w->source being
> the correct grep source seems to matter.
> 
> > -		strbuf_init(&w.out, 0);
> > -		opt->output_priv = &w;
> > -		hit = grep_submodule_launch(opt, &w.source);
> > +		opt->output_priv = &outbuf;
> > +		hit = grep_submodule_launch(opt, &gs);
> >  
> > -		write_or_die(1, w.out.buf, w.out.len);
> > +		write_or_die(1, outbuf.buf, outbuf.len);
> >  
> > -		grep_source_clear(&w.source);
> > -		strbuf_release(&w.out);
> > +		grep_source_clear(&gs);
> > +		strbuf_release(&outbuf);
> >  		return hit;
> >  	}
> >  }