On 03/17, Junio C Hamano wrote:
> Rahul Bedarkar <rahul.bedarkar@xxxxxxxxxx> 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;
> > }
> > }
Thanks for pointing out the bug (there seem to just be more and more in
this grep code...but what else can you expect from my first
contribution! :D) but I think it would be better to make the way the
recursive code output's more consistent with the way the rest of the
grep handles output. That way we can get rid of some of the special
casing that I had done here.
I'll send out what I have locally in just a second.
--
Brandon Williams