LockFile reading has questionable error handling

[nguerrera]

See this comment going in to SDK:

// LockFileUtilties.GetLockFile has odd error handling:
//
//   1. Exceptions creating TextReader from path (after up to 3 tries) will
//      bubble out.
//
//   2. There's an up-front File.Exists that returns null without logging
//      anything.
//
//   3. Any other exception whatsoever is logged by its Message property
//      alone, and an empty, non-null lock file is returned.

I had to read the source code to figure this out:

LockFileUtilities.cs

public static LockFile GetLockFile(string lockFilePath, Common.ILogger logger)
{
    LockFile lockFile = null;

    if (File.Exists(lockFilePath))
    {
        var format = new LockFileFormat();

        // A corrupt lock file will log errors and return null
        lockFile = FileUtility.SafeRead(filePath: lockFilePath,read: (stream, path) => format.Read(stream, logger, path));
    }

    return lockFile;
}

The comment "A corrupt lock file will log errors and return null" is wrong.

LockFileFormat.cs:

public LockFile Read(TextReader reader, ILogger log, string path)
{
    try
    {
        var json = JsonUtility.LoadJson(reader);
        var lockFile = ReadLockFile(json);
        lockFile.Path = path;
        return lockFile;
    }
    catch (Exception ex)
    {
        log.LogError(string.Format(CultureInfo.CurrentCulture,
            Strings.Log_ErrorReadingLockFile,
            path, ex.Message));

        // Ran into parsing errors, mark it as unlocked and out-of-date
        return new LockFile
        {
            Version = int.MinValue,
            Path = path
        };
    }
}

In the recent assembly binding conflict episode, exception there was FileLoadException and the message was something about an assembly manifest mismatch!

When I read code like above, I see:

try
{
       ...
}
catch (Bug bug)
{
   Log(bug.MessageThatUserWillNotUnderstandWithoutDetailsThatDeveloperWillNeedToFix);
   return SomethingThatWillCauseTheProgramToContinueAndFailInAWayThatIsNotAtAllObviouslyRelatedToThis;
}

[nguerrera]

cc @rrelyea