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

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;
>  	}
>  }

  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 [this message]
2017-03-17 18:24   ` Brandon Williams
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=xmqqy3w37ptd.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --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