space here, while (

Ah, I need to rework that. I frequently forget about colored output. =)

So, there's two scenarios where we go down this code path: -o and --color. The current behavior is wrong in the former scenario since multiple matches in a line are separated by newlines, and each *line* should prefixed with file name/line number/offset based on input flags.

We should probably just print before the loop for not -o output and print within the loop for -o, rather than print before the loop and then every match but the first for -o. I'll do this and throw in a test for the color case (as well as color+o).

It also looks like the putchar() in the !oflag branch at the end of this section got reverted to '\n' instead of fileeol, but I'll leave that out of here.

I botched this part again -- $type should be $?. If this revision is otherwise good, can this change be slipped into the commit?

Yep!

Err... please update the diff (it'll make it easier for the person that does the final commit)

Add a comment for consistency with the other fields?

What is nst?

What is st?

Alrighty, I'm going to have to update the diff anywho for other concerns presented -- I've been told in the past that small changes like this don't necessarily need a full round through Phabricator on their own.

Well, I think of it as 'new start'.

I think of it as 'start', for 'start of section to match.' This is going to get changed (along with c, because the name/type bother me) in a future changeset, but thus far I've tried to avoid renaming unless I'm actually changing semantics. st will more likely just go away in favor of using the new pc->lnstart, since that should always be accurate.

I should note that I haven't done this yet because I've yet to do my due diligence in making sure that using pc->lnstart in that way won't complicate other things existing in local branches or future plans.

I wouldn't have thought nst was new start and st was start (also: start of what?). Could you please expound the variable names so their purposes are more intuitive please?

"Next" start and start are intuitive given their usage. I don't think there's any value in blowing out some already long lines with longer names.

If anything can be done for clarity, I think refactoring this larger function into smaller routines might be good. But it doesn't have to happen in this patch.

Refactoring this a little further should be fairly intuitive now that I've simplified this function quite a bit and moved match information out into the parsing context.

Was this bit added intentionally?

This fails until D10580 goes in so I am going to comment it out initially, and enable it with D10580.

This could have been done with: jot -b 'x' 4096 | tr -d '\n' > test.in

Why commented out?

Ah, ok... I didn't catch that part before I posted my previous comment.