git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Rahul Bedarkar <rahul.bedarkar@imgtec.com>, git@vger.kernel.org
Subject: Re: [PATCH] grep: fix build with no thread support
Date: Fri, 17 Mar 2017 11:24:03 -0700
Message-ID: <20170317182403.GA110341@google.com> (raw)
In-Reply-To: <xmqqy3w37ptd.fsf@gitster.mtv.corp.google.com>

On 03/17, Junio C Hamano wrote:
> 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;
> >  	}
> >  }

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-17  5:47 Rahul Bedarkar
2017-03-17 17:53 ` Junio C Hamano
2017-03-17 18:24   ` Brandon Williams [this message]
2017-03-17 18:41     ` [PATCH 1/2] grep: set default output method Brandon Williams
2017-03-17 18:41       ` [PATCH 2/2] grep: fix builds with with no thread support Brandon Williams
2017-03-17 23:00         ` Jonathan Nieder
2017-03-20  5:55         ` Rahul Bedarkar
2017-03-17 18:47   ` [PATCH] grep: fix build " Brandon Williams
2017-03-17 22:37     ` Jeff King
2017-03-17 22:42       ` Brandon Williams
2017-03-17 22:58         ` Jeff King
2017-03-17 23:03           ` Brandon Williams

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170317182403.GA110341@google.com \
    --to=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rahul.bedarkar@imgtec.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox