{"thread":{"id":"51108","subject":"[RFC PATCH] grep: provide sane default to grep_source struct","startedAt":"2019-05-16T02:00:46Z","lastAt":"2019-05-28T17:57:31Z","messageCount":15,"participants":["Emily Shaffer","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"375685","messageId":"20190516020023.61161-1-emilyshaffer@google.com","threadId":"51108","inReplyTo":null,"subject":"[RFC PATCH] grep: provide sane default to grep_source struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-16T02:00:23Z","receivedAt":"2019-05-16T02:00:46Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"grep_buffer creates a struct grep_source gs and calls grep_source()\nwith it. However, gs.name is null, which eventually produces a\nsegmentation fault in\ngrep_source()->grep_source_1()->show_line() when grep_opt.status_only is\nnot set.\n\nThis seems to be unreachable from existing commands but is reachable in\nthe event that someone rolls their own revision walk and neglects to set\nrev_info->grep_filter->status_only. Conceivably, someone might want to\nprint all changed lines of all commits which match some regex.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nHiya,\n\nI ran into this issue while I was working on a tutorial on rolling your\nown revision walk. I didn't want to print all the lines associated with\na matching change, but it didn't seem good that a seemingly-sane\ngrep_filter config was segfaulting.\n\nI don't know if adding a placeholder name is the right answer (hence RFC\npatch).\n\nJonathan Nieder proposed alternatively adding some check to grep_source()\nto ensure that if opt->status_only is unset, gs->name must be non-NULL\n(and yell about it if not), as well as some extra comments indicating\nwhat assumptions are made about the data coming into functions like\ngrep_source(). I'm fine with that as well (although I'm not sure it\nmakes sense semantically to require a name which the user probably can't\neasily set, or else ban the user from printing LOC during grep). Mostly\nI'm happy with any solution besides a segfault with no error logging :)\n\nThanks in advance for everyone's thoughts.\nEmily\n\n\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0d50598acd..fd84454faf 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2045,7 +2045,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in memory)\"), NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n-- \n2.21.0.1020.gf2820cf01a-goog\n\n"},{"id":"375688","messageId":"xmqqy3373xnj.fsf@gitster-ct.c.googlers.com","threadId":"51108","inReplyTo":"20190516020023.61161-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH] grep: provide sane default to grep_source struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-16T03:07:12Z","receivedAt":"2019-05-16T03:07:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> I don't know if adding a placeholder name is the right answer (hence RFC\n> patch).\n>\n> Jonathan Nieder proposed alternatively adding some check to grep_source()\n> to ensure that if opt->status_only is unset, gs->name must be non-NULL\n> (and yell about it if not), as well as some extra comments indicating\n> what assumptions are made about the data coming into functions like\n> grep_source(). I'm fine with that as well (although I'm not sure it\n\nThat's an interesting problem.\n\nCurrently the only use of the function is to see if the log message\nmatches with the given pattern (yes/no), but it is conceivable that\nnew callers may want to prepare in-core data and use it to see if\nthat matches the pattern, or even to _show_ the lines that match the\npattern (possibly with its own .output callback).  Such a caller may\neven make repeated calls to the function, and want to give hint in\nthe output to help users identify which in-core data produced hits,\nwhich implies that a hardcoded \"(in core)\" is not such a great idea.\nIn a future where we support such a caller, the function signature\nof grep_buffer() would change to include the name, and you would be\npassing that to grep_source_init().\n\nUntil that happens, it would be OK to use a hardcoded \"in-core\"\nconstant here, but to futureproof for such a future, I think it\nmakes sense to have a check for a BUG() suggested by jrn---that\nwould become useful once callers of grep_buffer() can give names\nprevent them from passing NULL when it later would be used.\n\n> makes sense semantically to require a name which the user probably can't\n> easily set, or else ban the user from printing LOC during grep). Mostly\n> I'm happy with any solution besides a segfault with no error logging :)\n>\n> Thanks in advance for everyone's thoughts.\n> Emily\n>\n>\n>  grep.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 0d50598acd..fd84454faf 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -2045,7 +2045,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n>  \tstruct grep_source gs;\n>  \tint r;\n>  \n> -\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n> +\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in memory)\"), NULL, NULL);\n>  \tgs.buf = buf;\n>  \tgs.size = size;\n"},{"id":"375689","messageId":"20190516022911.GA135875@google.com","threadId":"51108","inReplyTo":"20190516020023.61161-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH] grep: provide sane default to grep_source struct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-16T03:11:52Z","receivedAt":"2019-05-16T03:11:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEmily Shaffer wrote:\n\n> grep_buffer creates a struct grep_source gs and calls grep_source()\n> with it. However, gs.name is null, which eventually produces a\n> segmentation fault in\n> grep_source()->grep_source_1()->show_line() when grep_opt.status_only is\n> not set.\n\nThanks for catching it.  Taking a step back, I think the problem is in\nthe definition of \"struct grep_source\":\n\n\tstruct grep_source {\n\t\tchar *name;\n\n\t\tenum grep_source_type {\n\t\t\tGREP_SOURCE_OID,\n\t\t\tGREP_SOURCE_FILE,\n\t\t\tGREP_SOURCE_BUF,\n\t\t} type;\n\t\tvoid *identifier;\n\n\t\t...\n\t};\n\nWhat is the difference between a 'name' and an 'identifier'?  Who is\nresponsible for free()ing them?  Can they be NULL?  This is pretty\nunderdocumented for a public type.\n\nIf we take the point of view that 'name' should always be non-NULL,\nthen I wonder:\n\n- can we document that?\n- can grep_source_init enforce that?\n- can we take advantage of that in grep_source as well, as a sanity\n  check that the grep_source has been initialized?\n- while we're here, can we describe what the field is used for\n  (prefixing output with context before a \":\", I believe)?\n\n[...]\n> This seems to be unreachable from existing commands but is reachable in\n> the event that someone rolls their own revision walk and neglects to set\n> rev_info->grep_filter->status_only.\n\ninit_revisions sets that field, so a caller would have to replace\ngrep_filter or unset status_only to trigger this.\n\n>                                     Conceivably, someone might want to\n> print all changed lines of all commits which match some regex.\n\nHm, does that work well?  The caller of grep_buffer in revision.c\ndoesn't seem to be careful about where in the output hits would be\nprinted, but I might be missing something.\n\n[...]\n> I ran into this issue while I was working on a tutorial on rolling your\n> own revision walk. I didn't want to print all the lines associated with\n> a matching change, but it didn't seem good that a seemingly-sane\n> grep_filter config was segfaulting.\n\nGood find, and thanks for taking the time to make it easier to debug for\nthe next person.\n\n[...]\n> Jonathan Nieder proposed alternatively adding some check to grep_source()\n> to ensure that if opt->status_only is unset, gs->name must be non-NULL\n> (and yell about it if not), as well as some extra comments indicating\n> what assumptions are made about the data coming into functions like\n> grep_source(). I'm fine with that as well (although I'm not sure it\n> makes sense semantically to require a name which the user probably can't\n> easily set, or else ban the user from printing LOC during grep). Mostly\n> I'm happy with any solution besides a segfault with no error logging :)\n\nLet's compare the two possibilities.\n\nThe advantage of \"(in memory)\" is that it Just Works, which should\nmake a nicer development experience with getting a new caller mostly\nworking on the way to getting them working just the way you want.\n\nThe disadvantage is that if we start outputting that in production, we\nand static analyzers are less likely to notice.  In other words,\nprinting \"(in memory)\" is leaking details to the end user that do not\nmatch what the end user asked for.  NULL would instead produce a\ncrash, prompting the author of the caller to fix it.\n\nWhat was particularly pernicious about this example is that the NULL\ndereference only occurs if the grep has a match.  So I suppose I'm\nleaning toward (in addition to adding some comments to document the\nstruct) adding a check like\n\n\tif (!gs->name && !opt->status_only)\n\t\tBUG(\"grep calls that could print name require name\");\n\nto grep_source.\n\nThat would also sidestep the question of whether this debugging aid\nshould be translated. :)\n\nSensible?\n\nThanks,\nJonathan\n"},{"id":"375690","messageId":"20190516031338.GA156553@google.com","threadId":"51108","inReplyTo":"xmqqy3373xnj.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH] grep: provide sane default to grep_source struct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-16T03:13:38Z","receivedAt":"2019-05-16T03:13:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Currently the only use of the function is to see if the log message\n> matches with the given pattern (yes/no), but it is conceivable that\n> new callers may want to prepare in-core data and use it to see if\n> that matches the pattern, or even to _show_ the lines that match the\n> pattern (possibly with its own .output callback).\n\nOh, good point about .output.  That answers my question about getting\nthe output in the right place.\n\n[...]\n>> Thanks in advance for everyone's thoughts.\n>> Emily\n\nThanks, both.\n\nJonathan\n"},{"id":"375741","messageId":"20190516210558.GB138048@google.com","threadId":"51108","inReplyTo":"20190516022911.GA135875@google.com","subject":"Re: [RFC PATCH] grep: provide sane default to grep_source struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-16T21:05:58Z","receivedAt":"2019-05-16T21:06:07Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, May 15, 2019 at 08:11:52PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Emily Shaffer wrote:\n> \n> > grep_buffer creates a struct grep_source gs and calls grep_source()\n> > with it. However, gs.name is null, which eventually produces a\n> > segmentation fault in\n> > grep_source()->grep_source_1()->show_line() when grep_opt.status_only is\n> > not set.\n> \n> Thanks for catching it.  Taking a step back, I think the problem is in\n> the definition of \"struct grep_source\":\n> \n> \tstruct grep_source {\n> \t\tchar *name;\n> \n> \t\tenum grep_source_type {\n> \t\t\tGREP_SOURCE_OID,\n> \t\t\tGREP_SOURCE_FILE,\n> \t\t\tGREP_SOURCE_BUF,\n> \t\t} type;\n> \t\tvoid *identifier;\n> \n> \t\t...\n> \t};\n> \n> What is the difference between a 'name' and an 'identifier'?  Who is\n> responsible for free()ing them?  Can they be NULL?  This is pretty\n> underdocumented for a public type.\n> \n> If we take the point of view that 'name' should always be non-NULL,\n> then I wonder:\n> \n> - can we document that?\n> - can grep_source_init enforce that?\n\nToday grep_source_init() defaults to NULL. So if we decide that 'name'\nshould be non-NULL it will be somewhat changing the intent.\n\n\tvoid grep_source_init(struct grep_source *gs, enum grep_source_type type,        \n\t                      const char *name, const char *path,                        \n\t                      const void *identifier)                                    \n\t{                                                                                \n\t        gs->type = type;                                                         \n\t        gs->name = xstrdup_or_null(name); \n\t...\n\n> - can we take advantage of that in grep_source as well, as a sanity\n>   check that the grep_source has been initialized?\n> - while we're here, can we describe what the field is used for\n>   (prefixing output with context before a \":\", I believe)?\n\nIn general the documentation for grep.[ch] is pretty light. There aren't\nany header comments and `Documentation/technical/api-grep.txt` is a\ntodo. So I agree that we should document it anywhere we can.\n\n> > Jonathan Nieder proposed alternatively adding some check to grep_source()\n> > to ensure that if opt->status_only is unset, gs->name must be non-NULL\n> > (and yell about it if not), as well as some extra comments indicating\n> > what assumptions are made about the data coming into functions like\n> > grep_source(). I'm fine with that as well (although I'm not sure it\n> > makes sense semantically to require a name which the user probably can't\n> > easily set, or else ban the user from printing LOC during grep). Mostly\n> > I'm happy with any solution besides a segfault with no error logging :)\n> \n> Let's compare the two possibilities.\n> \n> The advantage of \"(in memory)\" is that it Just Works, which should\n> make a nicer development experience with getting a new caller mostly\n> working on the way to getting them working just the way you want.\n> \n> The disadvantage is that if we start outputting that in production, we\n> and static analyzers are less likely to notice.  In other words,\n> printing \"(in memory)\" is leaking details to the end user that do not\n> match what the end user asked for.  NULL would instead produce a\n> crash, prompting the author of the caller to fix it.\n> \n> What was particularly pernicious about this example is that the NULL\n> dereference only occurs if the grep has a match.  So I suppose I'm\n> leaning toward (in addition to adding some comments to document the\n> struct) adding a check like\n> \n> \tif (!gs->name && !opt->status_only)\n> \t\tBUG(\"grep calls that could print name require name\");\n> \n> to grep_source.\n\nWhy not both? :)\n\nBut seriously, I am planning to push a second patch with both, per\nJunio's reply.\n\nI'll consider the documentation out of scope for now since I'm not sure\nI know enough about grep.[ch]'s intent or history to document anything\nyet. :)\n\n> \n> That would also sidestep the question of whether this debugging aid\n> should be translated. :)\n> \n> Sensible?\n> \n> Thanks,\n> Jonathan\n"},{"id":"375745","messageId":"20190516214444.191743-1-emilyshaffer@google.com","threadId":"51108","inReplyTo":"20190516020023.61161-1-emilyshaffer@google.com","subject":"[PATCH v2] grep: provide sane default to grep_source struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-16T21:44:44Z","receivedAt":"2019-05-16T21:46:25Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"grep_buffer creates a struct grep_source gs and calls grep_source()\nwith it. However, gs.name is null, which eventually produces a\nsegmentation fault in\ngrep_source()->grep_source_1()->show_line() when grep_opt.status_only is\nnot set.\n\nThis seems to be unreachable from existing commands but is reachable in\nthe event that someone rolls their own revision walk and neglects to set\nrev_info->grep_filter->status_only. Conceivably, someone might want to\nprint all changed lines of all commits which match some regex.\n\nTo futureproof, give developers a heads-up if grep_source() is called\nand a valid name field is expected but not given. This path should be\nunreachable now, but in the future may become reachable if developers\nwant to add the ability to use grep_buffer() in prepared in-core data\nand provide hints to the user about where the data came from.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nI've added the BUG() line to grep_source(). I considered adding it to\ngrep_source_1() but didn't see much difference. Looks like\ngrep_source_1() exists purely as a helper to grep_source() and is never\ncalled by anybody else... but it's the function where the crash would\nhappen. So I'm not sure.\n\nI also modified the wording for the BUG(\"\") statement a little from\njrn's suggestion to hopefully make it more explicit, but I welcome\nwordsmithing.\n\n grep.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0d50598acd..6ceb880a8c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2021,6 +2021,9 @@ static int chk_hit_marker(struct grep_expr *x)\n \n int grep_source(struct grep_opt *opt, struct grep_source *gs)\n {\n+\tif (!opt->status_only && gs->name == NULL)\n+\t\tBUG(\"grep call which could print a name requires \"\n+\t\t    \"grep_source.name be non-NULL\");\n \t/*\n \t * we do not have to do the two-pass grep when we do not check\n \t * buffer-wide \"all-match\".\n@@ -2045,7 +2048,11 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n+\t/* TODO: In the future it may become desirable to pass in the name as\n+\t * an argument to grep_buffer(). At that time, \"(in core)\" should be\n+\t * replaced.\n+\t */\n+\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in core)\"), NULL, NULL);\n \tgs.buf = buf;\n \tgs.size = size;\n \n-- \n2.21.0.1020.gf2820cf01a-goog\n\n"},{"id":"375750","messageId":"20190516220254.GG10787@sigill.intra.peff.net","threadId":"51108","inReplyTo":"20190516214444.191743-1-emilyshaffer@google.com","subject":"Re: [PATCH v2] grep: provide sane default to grep_source struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T22:02:54Z","receivedAt":"2019-05-16T22:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 02:44:44PM -0700, Emily Shaffer wrote:\n\n> grep_buffer creates a struct grep_source gs and calls grep_source()\n> with it. However, gs.name is null, which eventually produces a\n> segmentation fault in\n> grep_source()->grep_source_1()->show_line() when grep_opt.status_only is\n> not set.\n\nFunny wrapping here.\n\n> This seems to be unreachable from existing commands but is reachable in\n> the event that someone rolls their own revision walk and neglects to set\n> rev_info->grep_filter->status_only. Conceivably, someone might want to\n> print all changed lines of all commits which match some regex.\n> \n> To futureproof, give developers a heads-up if grep_source() is called\n> and a valid name field is expected but not given. This path should be\n> unreachable now, but in the future may become reachable if developers\n> want to add the ability to use grep_buffer() in prepared in-core data\n> and provide hints to the user about where the data came from.\n\nSo I guess this is probably my fault, as I was the one who factored out\nthe grep_source bits long ago (though I would also not be surprised if\nit could be triggered before, too).\n\nI think adding a BUG() is the right thing to do.\n\n> I've added the BUG() line to grep_source(). I considered adding it to\n> grep_source_1() but didn't see much difference. Looks like\n> grep_source_1() exists purely as a helper to grep_source() and is never\n> called by anybody else... but it's the function where the crash would\n> happen. So I'm not sure.\n\nI agree it probably doesn't matter. I'd probably stick at at the top of\ngrep_source_1(), just with the rationale that it could possibly catch\nmore cases if somebody ever refactors grep_source() to have two\ndifferent callers (e.g., imagine we later add a variant that takes more\noptions, but leave the old one to avoid disrupting existing callers).\n\n> I also modified the wording for the BUG(\"\") statement a little from\n> jrn's suggestion to hopefully make it more explicit, but I welcome\n> wordsmithing.\n> [...]\n> +\t\tBUG(\"grep call which could print a name requires \"\n> +\t\t    \"grep_source.name be non-NULL\");\n\nIt looks fine to me. The point is that nobody should ever see this. And\nif they do, we should already get a file/line number telling us where\nthe problem is (and a coredump to poke at). So as long as it's enough to\nconvince a regular user that they should make a report to the mailing\nlist, it will have done its job.\n\n> diff --git a/grep.c b/grep.c\n> index 0d50598acd..6ceb880a8c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -2021,6 +2021,9 @@ static int chk_hit_marker(struct grep_expr *x)\n>  \n>  int grep_source(struct grep_opt *opt, struct grep_source *gs)\n>  {\n> +\tif (!opt->status_only && gs->name == NULL)\n> +\t\tBUG(\"grep call which could print a name requires \"\n> +\t\t    \"grep_source.name be non-NULL\");\n>  \t/*\n>  \t * we do not have to do the two-pass grep when we do not check\n>  \t * buffer-wide \"all-match\".\n\nMinor nit, but can we put a blank line between the new block and the\ncomment?\n\n> @@ -2045,7 +2048,11 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n>  \tstruct grep_source gs;\n>  \tint r;\n>  \n> -\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n> +\t/* TODO: In the future it may become desirable to pass in the name as\n> +\t * an argument to grep_buffer(). At that time, \"(in core)\" should be\n> +\t * replaced.\n> +\t */\n> +\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in core)\"), NULL, NULL);\n\nHmm. I don't see much point in this one, as it would just avoid\ntriggering our BUG(). If somebody is adding new grep_buffer() callers\nthat don't use status_only, wouldn't we want them to see the BUG() to\nknow that they need to refactor grep_buffer() to provide a name?\n\n-Peff\n"},{"id":"376023","messageId":"20190521235212.GA46998@google.com","threadId":"51108","inReplyTo":"20190516220254.GG10787@sigill.intra.peff.net","subject":"Re: [PATCH v2] grep: provide sane default to grep_source struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-21T23:52:12Z","receivedAt":"2019-05-21T23:52:21Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, May 16, 2019 at 06:02:54PM -0400, Jeff King wrote:\n> On Thu, May 16, 2019 at 02:44:44PM -0700, Emily Shaffer wrote:\n> \n> > grep_buffer creates a struct grep_source gs and calls grep_source()\n> > with it. However, gs.name is null, which eventually produces a\n> > segmentation fault in\n> > grep_source()->grep_source_1()->show_line() when grep_opt.status_only is\n> > not set.\n> \n> Funny wrapping here.\n> \n> > This seems to be unreachable from existing commands but is reachable in\n> > the event that someone rolls their own revision walk and neglects to set\n> > rev_info->grep_filter->status_only. Conceivably, someone might want to\n> > print all changed lines of all commits which match some regex.\n> > \n> > To futureproof, give developers a heads-up if grep_source() is called\n> > and a valid name field is expected but not given. This path should be\n> > unreachable now, but in the future may become reachable if developers\n> > want to add the ability to use grep_buffer() in prepared in-core data\n> > and provide hints to the user about where the data came from.\n> \n> So I guess this is probably my fault, as I was the one who factored out\n> the grep_source bits long ago (though I would also not be surprised if\n> it could be triggered before, too).\n> \n> I think adding a BUG() is the right thing to do.\n> \n> > I've added the BUG() line to grep_source(). I considered adding it to\n> > grep_source_1() but didn't see much difference. Looks like\n> > grep_source_1() exists purely as a helper to grep_source() and is never\n> > called by anybody else... but it's the function where the crash would\n> > happen. So I'm not sure.\n> \n> I agree it probably doesn't matter. I'd probably stick at at the top of\n> grep_source_1(), just with the rationale that it could possibly catch\n> more cases if somebody ever refactors grep_source() to have two\n> different callers (e.g., imagine we later add a variant that takes more\n> options, but leave the old one to avoid disrupting existing callers).\n\nI think this combined with needing a history lesson to know not to call\ngrep_source_1() makes a pretty strong case for moving it. I'll put the\nBUG() line at the top of grep_source_1() instead.\n\n> \n> > I also modified the wording for the BUG(\"\") statement a little from\n> > jrn's suggestion to hopefully make it more explicit, but I welcome\n> > wordsmithing.\n> > [...]\n> > +\t\tBUG(\"grep call which could print a name requires \"\n> > +\t\t    \"grep_source.name be non-NULL\");\n> \n> It looks fine to me. The point is that nobody should ever see this. And\n> if they do, we should already get a file/line number telling us where\n> the problem is (and a coredump to poke at). So as long as it's enough to\n> convince a regular user that they should make a report to the mailing\n> list, it will have done its job.\n> \n> > diff --git a/grep.c b/grep.c\n> > index 0d50598acd..6ceb880a8c 100644\n> > --- a/grep.c\n> > +++ b/grep.c\n> > @@ -2021,6 +2021,9 @@ static int chk_hit_marker(struct grep_expr *x)\n> >  \n> >  int grep_source(struct grep_opt *opt, struct grep_source *gs)\n> >  {\n> > +\tif (!opt->status_only && gs->name == NULL)\n> > +\t\tBUG(\"grep call which could print a name requires \"\n> > +\t\t    \"grep_source.name be non-NULL\");\n> >  \t/*\n> >  \t * we do not have to do the two-pass grep when we do not check\n> >  \t * buffer-wide \"all-match\".\n> \n> Minor nit, but can we put a blank line between the new block and the\n> comment?\n\nI made sure to include blank lines surrounding this block in its new\nhome.\n\n> \n> > @@ -2045,7 +2048,11 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n> >  \tstruct grep_source gs;\n> >  \tint r;\n> >  \n> > -\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n> > +\t/* TODO: In the future it may become desirable to pass in the name as\n> > +\t * an argument to grep_buffer(). At that time, \"(in core)\" should be\n> > +\t * replaced.\n> > +\t */\n> > +\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in core)\"), NULL, NULL);\n> \n> Hmm. I don't see much point in this one, as it would just avoid\n> triggering our BUG(). If somebody is adding new grep_buffer() callers\n> that don't use status_only, wouldn't we want them to see the BUG() to\n> know that they need to refactor grep_buffer() to provide a name?\n\nFor me, I'd rather have *some* functionality available without doing a\nlarge refactor. The line now isn't inaccurate, and `git grep --fixed\n\"(in core)\"` shows it's the only instance of that string in the\ncodebase, so it's easy to track down where it's happening. Though the\nBUG() line is pretty easy to track down too.\n\nHaving a default field like this might help folks move quickly if they\njust want proof of concept that their idea works without adding a\nbunch of plumbing. But then, they could add it on their own if they're\nthat early in their feature....\n\nCan we think of a reason anybody would want to be able to use it this\nway with the placeholder string?\n\n> \n> -Peff\n\nThanks for your comments Peff.\n\n- Emily\n"},{"id":"376025","messageId":"20190522001736.GA219159@google.com","threadId":"51108","inReplyTo":"20190521235212.GA46998@google.com","subject":"Re: [PATCH v2] grep: provide sane default to grep_source struct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-22T00:17:36Z","receivedAt":"2019-05-22T00:17:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEmily Shaffer wrote:\n> On Thu, May 16, 2019 at 06:02:54PM -0400, Jeff King wrote:\n>> On Thu, May 16, 2019 at 02:44:44PM -0700, Emily Shaffer wrote:\n\n>>> +\t/* TODO: In the future it may become desirable to pass in the name as\n>>> +\t * an argument to grep_buffer(). At that time, \"(in core)\" should be\n>>> +\t * replaced.\n>>> +\t */\n\n(micronit, likely moot: Git's multi-line comments start with \"/*\" on\nits own line:\n\n\t/*\n\t * NEEDSWORK: Passing the name in as an argument would allow\n\t * \"(in core)\" to be replaced.\n\t */\n\n.)\n\n>>> +\tgrep_source_init(&gs, GREP_SOURCE_BUF, _(\"(in core)\"), NULL, NULL);\n>>\n>> Hmm. I don't see much point in this one, as it would just avoid\n>> triggering our BUG(). If somebody is adding new grep_buffer() callers\n>> that don't use status_only, wouldn't we want them to see the BUG() to\n>> know that they need to refactor grep_buffer() to provide a name?\n[...]\n> Can we think of a reason anybody would want to be able to use it this\n> way with the placeholder string?\n\nI agree with Peff here: using NULL puts this in a better place with\nrespect to Rusty's API design manifesto[1].\n\nWith the \"(in core)\" default, I may end up triggering the \"(in core)\"\nbehavior in production, because there is not a clear enough signal\nthat my code path is making a mistake.  That's problematic because it\ngives the end user a confusing experience: the end user cares where\nthe line comes from, not that it spent a second or two in core.\n\nWith the NULL default, *especially* after this patch, such usage would\ninstead trigger a BUG: line in output, meaning\n\n- if it gets exercised in tests, the test will fail, prompting the\n  patch auther to pass in a more appropriate label\n\n- if it gets missed in tests and gets triggered in production, the error\n  message makes it clear that this is a mistake so the user is likely\n  to report a bug instead of assuming this is deliberate but confusing\n  behavior\n\nIn that vein, this patch is very helpful, since the BUG would trip\n*consistently*, not only when the grep pattern matches.  Failing\nconsistently like this is a huge improvement in API usability.\n\nIt would be even better if we could catch the problem at compile time,\nbut one thing at a time.\n\nThanks,\nJonathan\n\n[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html,\nhttps://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html\n"},{"id":"376032","messageId":"20190522003402.77767-1-emilyshaffer@google.com","threadId":"51108","inReplyTo":"20190516214444.191743-1-emilyshaffer@google.com","subject":"[PATCH v3] grep: provide sane default to grep_source struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-22T00:34:02Z","receivedAt":"2019-05-22T00:35:01Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"grep_buffer creates a struct grep_source gs and calls grep_source()\nwith it. However, gs.name is null, which eventually produces a\nsegmentation fault in grep_source()->grep_source_1()->show_line()\nwhen grep_opt.status_only is not set.\n\nThis seems to be unreachable from existing commands but is reachable in\nthe event that someone rolls their own revision walk and neglects to set\nrev_info->grep_filter->status_only. Conceivably, someone might want to\nprint all changed lines of all commits which match some regex.\n\nTo futureproof, give developers a heads-up if grep_source() is called\nand a valid name field is expected but not given. This path should be\nunreachable now, but in the future may become reachable if developers\nwant to add the ability to use grep_buffer() in prepared in-core data\nand provide hints to the user about where the data came from.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n\nI think Peff and Jonathan are right. If someone does want to hack away\non something in the very early stages, the BUG() line gives a pretty\nclear indicator of what to modify to make it work.\n\nI also moved the BUG() to grep_source_1() to keep it close to the error\nitself and reflowed the commit message.\n\n - Emily\n\n grep.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex 0d50598acd..f7c3a5803e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1780,6 +1780,10 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n+\tif (!opt->status_only && gs->name == NULL)\n+\t\tBUG(\"grep call which could print a name requires \"\n+\t\t    \"grep_source.name be non-NULL\");\n+\n \tif (!opt->output)\n \t\topt->output = std_output;\n \n-- \n2.21.0.1020.gf2820cf01a-goog\n\n"},{"id":"376035","messageId":"20190522005858.GB219159@google.com","threadId":"51108","inReplyTo":"20190522003402.77767-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] grep: provide sane default to grep_source struct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-22T00:58:58Z","receivedAt":"2019-05-22T00:59:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> Subject: grep: provide sane default to grep_source struct\n\nThis subject line doesn't describe what the patch does any more.  How\nabout something like\n\n\tgrep: fail early if call could print output and name is not set\n\n?\n\n> grep_buffer creates a struct grep_source gs and calls grep_source()\n> with it. However, gs.name is null, which eventually produces a\n> segmentation fault in grep_source()->grep_source_1()->show_line()\n> when grep_opt.status_only is not set.\n\nThis is the place to describe the motivation behind the patch:\n\n- what is the current behavior?\n- what is the motivation behind the current behavior?\n- in what scenario does it fail?\n- how does it fail, and what is undesirable about that failure?\n\nPerhaps something like\n\n\tgrep_source, the workhorse of Git's grep library, allows passing in an\n\tarbitrary grep_source representing the haystack in which to search for\n\ta needle matching a pattern.  In most callers, the grep_source::name\n\tfield is set to an appropriate prefix to print before a colon when a\n\tresult matches:\n\n\t\tREADME:Git is an Open Source project covered by the GNU General Public\n\n\tOne caller, grep_buffer, leaves the 'name' field set to NULL because\n\tthere is not enough context to determine an appropriate name to put in\n\tsuch output lines.  In practice, this has been fine because the only\n\tcaller of grep_buffer is \"git log --grep\" and that caller sets\n\tstatus_only to disable output (and only check whether a match exists).\n\tBut this is brittle: a future caller can call grep_buffer without\n\tstatus_only set, and as soon as it hits a match, grep_source will\n\tsegfault in the process of trying to print\n\n\t\t(null):Git is an Open Source project covered by the GNU General Public\n\n> This seems to be unreachable from existing commands but is reachable in\n> the event that someone rolls their own revision walk and neglects to set\n> rev_info->grep_filter->status_only. Conceivably, someone might want to\n> print all changed lines of all commits which match some regex.\n\nFocusing on the use case instead of the details of how it is implemented,\nwe can simplify this to\n\n\tFor example, such a future caller might want to print all matching\n\tlines from commits which match a regex.\n\n> To futureproof, give developers a heads-up if grep_source() is called\n> and a valid name field is expected but not given. This path should be\n> unreachable now, but in the future may become reachable if developers\n> want to add the ability to use grep_buffer() in prepared in-core data\n> and provide hints to the user about where the data came from.\n\nHere we can concisely describe the improved behavior with the fix:\n\n\tFutureproof by diagnosing a use of the API that could trigger that\n\tcondition early, before we know whether the pattern matches:\n\n\t\tBUG: git.c:832: grep call which could print a name requires grep_source.name be non-NULL\n\t\tAborted\n\nAnd then what it makes possible:\n\n\tThis way, the caller's author gets a quick indication of how to fix\n\tit, by ensuring that either grep_source.name is set or that\n\tstatus_only is set to prevent printing output.\n\nAnd how it came up:\n\n\tNoticed while adding such a call in a tutorial on revision walks.\n\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n[...]\n> I also moved the BUG() to grep_source_1() to keep it close to the error\n> itself and reflowed the commit message.\n\nMakes sense.  Thanks for these summaries of what changed between each\nrevision of the patch --- they're very helpful.\n\nWith whatever subset of the above suggested commit message changes\nmake sense,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[...]\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1780,6 +1780,10 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n>  \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n>  \txdemitconf_t xecfg;\n>  \n> +\tif (!opt->status_only && gs->name == NULL)\n\noptional: can use !gs->name here.\n\n> +\t\tBUG(\"grep call which could print a name requires \"\n> +\t\t    \"grep_source.name be non-NULL\");\n"},{"id":"376046","messageId":"20190522040100.GA31212@sigill.intra.peff.net","threadId":"51108","inReplyTo":"20190522003402.77767-1-emilyshaffer@google.com","subject":"Re: [PATCH v3] grep: provide sane default to grep_source struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-22T04:01:00Z","receivedAt":"2019-05-22T04:01:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 21, 2019 at 05:34:02PM -0700, Emily Shaffer wrote:\n\n> I think Peff and Jonathan are right. If someone does want to hack away\n> on something in the very early stages, the BUG() line gives a pretty\n> clear indicator of what to modify to make it work.\n> \n> I also moved the BUG() to grep_source_1() to keep it close to the error\n> itself and reflowed the commit message.\n\nThanks. This version looks good to me (though if you took any of\nJonathan's suggestions in the other part of the thread that would be\nfine with me, too).\n\n-Peff\n"},{"id":"376118","messageId":"20190523202355.152742-1-emilyshaffer@google.com","threadId":"51108","inReplyTo":"20190522003402.77767-1-emilyshaffer@google.com","subject":"[PATCH v4] grep: fail if call could output and name is null","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-23T20:23:56Z","receivedAt":"2019-05-23T20:24:49Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"grep_source(), which performs much of the work for Git's grep library,\nallows passing an arbitrary struct grep_source which represents the text\nwhich grep_source() should search to match a pattern in the provided\nstruct grep_opt. In most callers, the grep_source::name field is set to\nan appropriate prefix to print before a colon when a result matches:\n\n\tREADME:Git is an Open Source project covered by the GNU General\n\nOne caller, grep_buffer(), leaves the grep_source::name field set to\nNULL because there isn't enough context to determine an appropriate name\nfor this kind of output line. In practice, this has been fine: the only\ncaller of grep_buffer() is \"git log --grep\", and that caller sets\ngrep_opt::status_only, which disables output and only checks whether a\nmatch exists. But this is brittle: a future caller can call\ngrep_buffer() without grep_opt::status_only set, and as soon as it hits\na match, grep_source() will try to print the match and segfault:\n\n\t(null):Git is an Open Source project covered by the GNU General\n\nFor example, a future caller might want to print all matching lines from\ncommits which match a regex.\n\nFutureproof by diagnosing early a use of the API that could trigger that\ncondition, before we know whether the pattern matches:\n\n\tBUG: grep.c:1783: grep call which could print a name requires\n\t\tgrep_source.name be non-NULL\n\tAborted\n\nThis way, the caller's author gets an indication of how to fix the issue\n- by providing grep_source::name or setting grep_opt::status_only - and\nthey are warned of the potential for a segfault unconditionally, rather\nthan only if there is a match.\n\nNoticed while adding such a call to a tutorial on revision walks.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nSince v3, only the commit message has changed. Reworked based on\nJonathan Nieder's suggestions (with some modifications for readability).\n\n - Emily\n\n grep.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex 0d50598acd..f7c3a5803e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1780,6 +1780,10 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tenum grep_context ctx = GREP_CONTEXT_HEAD;\n \txdemitconf_t xecfg;\n \n+\tif (!opt->status_only && gs->name == NULL)\n+\t\tBUG(\"grep call which could print a name requires \"\n+\t\t    \"grep_source.name be non-NULL\");\n+\n \tif (!opt->output)\n \t\topt->output = std_output;\n \n-- \n2.22.0.rc1.257.g3120a18244-goog\n\n"},{"id":"376120","messageId":"20190523203415.GB70860@google.com","threadId":"51108","inReplyTo":"20190523202355.152742-1-emilyshaffer@google.com","subject":"Re: [PATCH v4] grep: fail if call could output and name is null","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-23T20:34:15Z","receivedAt":"2019-05-23T20:34:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Emily Shaffer wrote:\n\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Since v3, only the commit message has changed. Reworked based on\n> Jonathan Nieder's suggestions (with some modifications for readability).\n>\n>  grep.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n\nThis is indeed\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for your patient work.\n"},{"id":"376327","messageId":"xmqqmuj631j0.fsf@gitster-ct.c.googlers.com","threadId":"51108","inReplyTo":"20190523203415.GB70860@google.com","subject":"Re: [PATCH v4] grep: fail if call could output and name is null","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T17:57:23Z","receivedAt":"2019-05-28T17:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Emily Shaffer wrote:\n>\n>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>> ---\n>> Since v3, only the commit message has changed. Reworked based on\n>> Jonathan Nieder's suggestions (with some modifications for readability).\n>>\n>>  grep.c | 4 ++++\n>>  1 file changed, 4 insertions(+)\n>\n> This is indeed\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>\n> Thanks for your patient work.\n\nThanks, both.  Will queue.\n"}]}