fix(longevity): StackOverflowError on serializing (APP-126) #49
Conversation
| val repo: Repository = git.repository | ||
| val head: RevCommit = | ||
| RevWalk(repo).parseCommit(repo.resolve(RepoHelper.MASTER_BRANCH)) | ||
| try { RevWalk(repo).parseCommit(repo.resolve("HEAD")) } |
There was a problem hiding this comment.
All hashers should be consistent in what branch they hash. We have agreement of hashing master branch, ref defined in RepoHelper.MASTER_BRANCH.
| val head: RevCommit = | ||
| RevWalk(repo).parseCommit(repo.resolve(RepoHelper.MASTER_BRANCH)) | ||
| try { RevWalk(repo).parseCommit(repo.resolve("HEAD")) } | ||
| catch(e: Exception) { throw Exception("No default branch") } |
There was a problem hiding this comment.
No master branch. But existence of master branch checked before run of hasher, so this check is excess.
There was a problem hiding this comment.
np, I'll revert the branch name change. However I think this requirement is too restrictive though and overcomplicates the things in general. I'll file a bug on it to have further discussion.
There was a problem hiding this comment.
btw, I'd like to keep the try/catch thing if that was your point, because it's useful for testing outside the main infrastructure.
| if (Files.notExists(Paths.get(storageDir))) { | ||
| Files.createDirectories(Paths.get(storageDir)) | ||
| } | ||
| val oStream = ObjectOutputStream(FileOutputStream(storagePath)) |
There was a problem hiding this comment.
This call should be encapsulated with try-catch, cuz can produce exceptions.
There was a problem hiding this comment.
same, what do you want to see under a catch block?
| } | ||
|
|
||
| val ageData = (iStream?.readObject() ?: CodeLineAges()) as CodeLineAges | ||
| catch(e: FileNotFoundException) { } |
There was a problem hiding this comment.
ObjectInputStream and FileInputStream can possibly produce a lot of different exceptions, so e: Exception is more appropriate as it generic. We should fail hashing in case of problems with loading of data for incremental updates.
There was a problem hiding this comment.
This done on purpose, if something bad or unexpected happens I want to know that. This bug was also found, because we don't hide exceptions. We may want to hide that on release builds, but that's definitely useful thing for testing and debugging.
| try { | ||
| val iStream = ObjectInputStream(FileInputStream(storagePath)) | ||
| val storedHeadId = iStream.readUTF() | ||
| Logger.debug("Stored repo head: storedHeadId") |
| return null | ||
| try { | ||
| val iStream = ObjectInputStream(FileInputStream(storagePath)) | ||
| val storedHeadId = iStream.readUTF() |
There was a problem hiding this comment.
Maybe add comment that this method read first line of file? Really strange naming of method they've made.
There was a problem hiding this comment.
Not sure whether it read the first line or not, it reads a string whatever format it has. I can add a comment like "Storage format: commit id and CodeLineAges structure following it" if you think it makes sense.
No description provided.