Re: [RFC PATCH] grep: provide sane default to grep_source struct
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 16, 2019, 03:07 UTC
- Message-ID
- <xmqqy3373xnj.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190516020023.61161-1-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 8 quoted lines
> I don't know if adding a placeholder name is the right answer (hence RFC > patch). > > Jonathan Nieder proposed alternatively adding some check to grep_source() > to ensure that if opt->status_only is unset, gs->name must be non-NULL > (and yell about it if not), as well as some extra comments indicating > what assumptions are made about the data coming into functions like > grep_source(). I'm fine with that as well (although I'm not sure it
That's an interesting problem.
Currently the only use of the function is to see if the log message matches with the given pattern (yes/no), but it is conceivable that new callers may want to prepare in-core data and use it to see if that matches the pattern, or even to _show_ the lines that match the pattern (possibly with its own .output callback). Such a caller may even make repeated calls to the function, and want to give hint in the output to help users identify which in-core data produced hits, which implies that a hardcoded "(in core)" is not such a great idea. In a future where we support such a caller, the function signature of grep_buffer() would change to include the name, and you would be passing that to grep_source_init().
Until that happens, it would be OK to use a hardcoded "in-core" constant here, but to futureproof for such a future, I think it makes sense to have a check for a BUG() suggested by jrn---that would become useful once callers of grep_buffer() can give names prevent them from passing NULL when it later would be used.
Show 23 quoted lines
> makes sense semantically to require a name which the user probably can't
> easily set, or else ban the user from printing LOC during grep). Mostly
> I'm happy with any solution besides a segfault with no error logging :)
>
> Thanks in advance for everyone's thoughts.
> Emily
>
>
> grep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 0d50598acd..fd84454faf 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -2045,7 +2045,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)
> struct grep_source gs;
> int r;
>
> - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);
> + grep_source_init(&gs, GREP_SOURCE_BUF, _("(in memory)"), NULL, NULL);
> gs.buf = buf;
> gs.size = size;