Deduplicate disk abstraction infrastructure

[bradking]

Currently we have two disk abstractions: DiskInterface and ManifestParser::FileReader.

Revise DiskInterface to support both use cases and then drop FileReader. This also simplifies test infrastructure by avoiding custom FileReader implementations.

[tfarina]

Nice! Thanks for doing this! I always felt there was something that could be done, since the two were very similar!

[jcfr]

👍

[bradking]

Rebased on master to resolve conflicts.

[nico]

Thank you for the patch. All that deleted code is nice to see, and the bonus build fix is much appreciated too :-) I think this makes the codebase better than it was before. I'll explain briefly why I didn't just hit "merge" long ago (sorry for the delay). If after reading this you think this patch as-is is the way to go, then I'll merge it as-is (as I do think it's better than what's there today).

• This allows ManifestParser to allow all of DiskInterface, even though ManifestParser doesn't have to stat files, write files, remove files, or make directories. In larger projects, having more, smaller interfaces tends to reduce unnecessary dependencies between targets in my experience. (But ninja is small enough that this doesn't matter, and e.g. introducing a FileReader interface and making DiskInterface a subclass of that seems like overkill, so this is fine as-is I think.)
• It adds errno to the interface of DiskInterface, while this was abstracted out previously. Almost nothing uses this errno return. (And it's not really errno on Windows, except for "file not found".) True, ::ReadFile() also exposes this, but I understood the functions in util.cc to be thin convenience wrappers around OS-specific functions while DiskInterface tries to be a real abstraction. Maybe ReadFile() could return an enum FileStatus { Ok, FileNotFound, OtherError }?
• It gives DiskInterface two ReadFile() methods, one of them more or less named DoAAndB(). At my current employer, we have this story about a File class that grew larger and larger over the decades due to people adding "XOrDie()", "XOrDieExceptIfFoo()" variants for several of its methods. The lesson from that was that it's preferable for clients of a class to have some control flow -- if a client wants "XOrDie()" they can write "if (!foo->X()) Die()" themselves. Since ReadFileIfExistsElseEmpty() (empty what?) only has two callers, maybe the two callers could check for "file not found" and clear out err themselves explicitly? (it is currently not obvious that the callers are fine with "file not found", so that might be a good change anyhow)

I hope that makes sense :-) As I said, if you prefer to not make any changes, that's fine too.

[bradking]

@nico, all good comments, thanks. I've rewritten the topic to address them.

This allows ManifestParser to allow all of DiskInterface

I split out a FileReader base class interface from DiskInterface to give to ManifestParser instead. This will restrict what it can use from a DiskInterface implementation.

It adds errno to the interface of DiskInterface

I changed the interface to return an enumeration as suggested.

It gives DiskInterface two ReadFile() methods, one of them...ReadFileIfExistsElseEmpty

I removed the latter in favor of updating the call sites. I had intended it to avoid duplication but there are only two places and a switch on the enumeration value makes the logic more transparent than trying to encode it in a long method name.

[nico]

Thanks!