[NETBEANS-643] Mercurial Annotate fails on files with spaces in them …

[SirIntellegence]

…and files named like .hgignore

Fixed that. I have tested it a bit with my copy and no test failures were added by the change (there are still unrelated tests that fail though…)
TODO: Use something like json instead of a line of text with a regex to get blame info to avoid possible escaping issues.

[SirIntellegence]

Could someone review this? I am going to reference @JaroslavTulach because I don't know who I should be referencing...

[SirIntellegence]

Oh, the page was out of date XD. I will reload the page before commenting in the future...

[emilianbold]

Is this PR current? Why is that big commented chunk of JSON related code included?

[SirIntellegence]

I was going to do it with json, but then some wacky things happened with the diffing engine. I would prefer to use the json method if possible, and it technically works with all of the commented code. You can uncomment it and see what I mean by wacky things.

[emilianbold]

Ignoring the commented JSON, you patch boils down to replacing (\\b\\S*) with \\S.*?. It would be nice if you would explain / comment what that does.

[sdedic]

@SirIntellegence does JSON (if it worked) offer some substational (which ? stability, correctness, ...) benefits over plaintext ? Asking bcs it requires Mercurial to encode and NB code to parse JSON, both of which may be sources of errors (and decreased performance).

[emilianbold]

@sdedic I think the idea is that files with spaces in them are better encoded with JSON compared to whatever plain text format Mercurial uses which gets parsed by regexp?

[sdedic]

@emilianbold what I'd like to understand, what "better" means: are there some filenames (or other data) which cannot be parsed out of the plaintext reliably ? If so, it could justify change of the serialization format to JSON.

[emilianbold]

@sdedic I don't know, I guess @SirIntellegence would have to answer as the PR owner.

I'm just imagining that this is why JSON was tried.

[SirIntellegence]

@sdedic I was running under the assumption that sometime, somehow, somewhere, the regex will be insufficient. I don't know what possible problems could crop up in the future, but if we were reading a formal structure such as json or xml from the output, it likely won't happen. The reason the checked in code isn't using JSON (as I mentioned before) is because something wacky happens when the resulting Array of AnnotationLines is given to the diffing engine. Also, be forewarned, any very long strings displayed in a JDK 9 or 10 debugger show up as "Unreadable", but the full string is still there. I wasted a good hour or two over that.

[emilianbold]

Although the JSON remark is interesting, could you restrict the PR to what actually fixes the bug? Seems to me it should literally be one line.

Could you add some test for the problem the modified regexp fixes? It's kinda hard to imagine nobody had spaces in filenames until now...

[SirIntellegence]

Very well. I will do that when I get time and will file the "use json instead" thing as a different bug.

[SirIntellegence]

I don't see any existing tests for the blame logic. I would add to them if they were there, but they aren't and I don't know how to write those kinds of tests.

[SirIntellegence]

I forgot to save before pushing the first one. Stupid mistake...

[emilianbold]

Yes, a test would be nice. That regexp pattern is still hard to read but at least your comment mentions the space/dot problem.

[SirIntellegence]

So, do I need to take the time to write a test, or is it good to merge into the master?

[geertjanw]

When in doubt, write a test. When not in doubt and there is no test, write two tests.

[SirIntellegence]

I did a rebase since someone moved directories around.

[matthiasblaesing]

@SirIntellegence I opened a new PR (see #851) as I noticed problems while doing manual testing and decided to look closed at the mercurial annotate command. The PR decriptions holds some more information. It would be great if you could give it a look. Thank you.