mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <>
To: Emily Shaffer <>
Cc:, Jeff King <>
Subject: Re: [PATCH v3] grep: provide sane default to grep_source struct
Date: Tue, 21 May 2019 17:58:58 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Emily Shaffer wrote:

> Subject: grep: provide sane default to grep_source struct

This subject line doesn't describe what the patch does any more.  How
about something like

	grep: fail early if call could print output and name is not set


> grep_buffer creates a struct grep_source gs and calls grep_source()
> with it. However, is null, which eventually produces a
> segmentation fault in grep_source()->grep_source_1()->show_line()
> when grep_opt.status_only is not set.

This is the place to describe the motivation behind the patch:

- what is the current behavior?
- what is the motivation behind the current behavior?
- in what scenario does it fail?
- how does it fail, and what is undesirable about that failure?

Perhaps something like

	grep_source, the workhorse of Git's grep library, allows passing in an
	arbitrary grep_source representing the haystack in which to search for
	a needle matching a pattern.  In most callers, the grep_source::name
	field is set to an appropriate prefix to print before a colon when a
	result matches:

		README:Git is an Open Source project covered by the GNU General Public

	One caller, grep_buffer, leaves the 'name' field set to NULL because
	there is not enough context to determine an appropriate name to put in
	such output lines.  In practice, this has been fine because the only
	caller of grep_buffer is "git log --grep" and that caller sets
	status_only to disable output (and only check whether a match exists).
	But this is brittle: a future caller can call grep_buffer without
	status_only set, and as soon as it hits a match, grep_source will
	segfault in the process of trying to print

		(null):Git is an Open Source project covered by the GNU General Public

> This seems to be unreachable from existing commands but is reachable in
> the event that someone rolls their own revision walk and neglects to set
> rev_info->grep_filter->status_only. Conceivably, someone might want to
> print all changed lines of all commits which match some regex.

Focusing on the use case instead of the details of how it is implemented,
we can simplify this to

	For example, such a future caller might want to print all matching
	lines from commits which match a regex.

> To futureproof, give developers a heads-up if grep_source() is called
> and a valid name field is expected but not given. This path should be
> unreachable now, but in the future may become reachable if developers
> want to add the ability to use grep_buffer() in prepared in-core data
> and provide hints to the user about where the data came from.

Here we can concisely describe the improved behavior with the fix:

	Futureproof by diagnosing a use of the API that could trigger that
	condition early, before we know whether the pattern matches:

		BUG: git.c:832: grep call which could print a name requires be non-NULL

And then what it makes possible:

	This way, the caller's author gets a quick indication of how to fix
	it, by ensuring that either is set or that
	status_only is set to prevent printing output.

And how it came up:

	Noticed while adding such a call in a tutorial on revision walks.

> Signed-off-by: Emily Shaffer <>
> ---
> I also moved the BUG() to grep_source_1() to keep it close to the error
> itself and reflowed the commit message.

Makes sense.  Thanks for these summaries of what changed between each
revision of the patch --- they're very helpful.

With whatever subset of the above suggested commit message changes
make sense,

Reviewed-by: Jonathan Nieder <>


> --- a/grep.c
> +++ b/grep.c
> @@ -1780,6 +1780,10 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>  	enum grep_context ctx = GREP_CONTEXT_HEAD;
>  	xdemitconf_t xecfg;
> +	if (!opt->status_only && gs->name == NULL)

optional: can use !gs->name here.

> +		BUG("grep call which could print a name requires "
> +		    " be non-NULL");

  reply	other threads:[~2019-05-22  0:59 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  2:00 [RFC PATCH] grep: provide sane default to grep_source struct Emily Shaffer
2019-05-16  3:07 ` Junio C Hamano
2019-05-16  3:13   ` Jonathan Nieder
2019-05-16  3:11 ` Jonathan Nieder
2019-05-16 21:05   ` Emily Shaffer
2019-05-16 21:44 ` [PATCH v2] " Emily Shaffer
2019-05-16 22:02   ` Jeff King
2019-05-21 23:52     ` Emily Shaffer
2019-05-22  0:17       ` Jonathan Nieder
2019-05-22  0:34   ` [PATCH v3] " Emily Shaffer
2019-05-22  0:58     ` Jonathan Nieder [this message]
2019-05-22  4:01     ` Jeff King
2019-05-23 20:23     ` [PATCH v4] grep: fail if call could output and name is null Emily Shaffer
2019-05-23 20:34       ` Jonathan Nieder
2019-05-28 17:57         ` Junio C Hamano

Reply instructions:

You may reply publicly 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:

  List information:

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

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).