feat: add common observable interface for hashers (APP-100) #34
Conversation
asurkov
left a comment
asurkov
left a comment
There was a problem hiding this comment.
This is rather a rubber stamp review, since the patch is about moving existing code around, but the patch looks good from an architectural standpoint, and I believe it's a good step forward to bring our classes to a common base. Nevertheless I made some comments about moved code, but the comments may be handled in a followup if you want to (and if it needed for sure).
| return try { | ||
| git.repository | ||
| .open(objectId).bytes.toString(Charset.defaultCharset()) | ||
| .split('\n') |
There was a problem hiding this comment.
It's probably RawText text is safer here, '\n' and defaultCharset() might not work for each case. I realize that RawText might be doing the same under the hood, but at least we have someone to blame :)
Also inlined code works equally well: RawText(repo.open(objectId).getBytes()) and probably doesn't deserve own function.
| }) | ||
| init { | ||
| // Delete locally missing commits from server. If found at least one | ||
| // common commit then next commits are not deleted because hash of a |
There was a problem hiding this comment.
I suppose you meant "preceding commits" not "next commits"
| val firstOverlapCommitRehash = findFirstOverlappingCommitRehash() | ||
| val deletedCommits = serverRepo.commits | ||
| .takeWhile { it.rehash != firstOverlapCommitRehash } | ||
| deleteCommitsOnServer(deletedCommits) |
There was a problem hiding this comment.
I think I would send a common found commit to a server and let the server to delete the following commits on its own
There was a problem hiding this comment.
Let's stay on current version of a protocol.
| .map { commit -> commit.rehash } | ||
| .toHashSet() | ||
| return rehashes.firstOrNull { rehash -> | ||
| serverHistoryRehashes.contains(rehash) |
There was a problem hiding this comment.
You could simply traverse the commits looking for a rehash, and that'd be faster than putting everything into a hash set, because there are good chances to never traverse all commits, right?
There was a problem hiding this comment.
I'm looking for first mapping of M(local) to N(server) commits not M(local) to 1. HashSet speed up the process.
There was a problem hiding this comment.
Not sure I follow it, anyway, this code wasn't introduced by this pull request, so I guess I can revisit this part later and file followup issue if needed.
| new.rehash != lastKnownCommit?.rehash | ||
| } | ||
| .filter { commit -> // Don't hash known. | ||
| knownCommits.isEmpty() || !knownCommits.contains(commit) |
There was a problem hiding this comment.
how could we reach it if we do takeWhile() until the lastKnownCommit?
There was a problem hiding this comment.
Server could miss some of the commits.
| import java.nio.charset.Charset | ||
|
|
||
| object CommitObservable { | ||
| fun getObservable(git: Git, repo: Repo) = Observable |
There was a problem hiding this comment.
It's a tautology to have CommitObservable.getObservable(). You may want to rename it to something like Commits.asObservable().
There was a problem hiding this comment.
Changed name to CommitCrawler, method name the same.
| subscriber.onError(e) | ||
| } | ||
| subscriber.onComplete() | ||
| } // TODO(anatoly): Rewrite diff calculation in non-weird way. |
There was a problem hiding this comment.
It's a funny TODO item but not clear what exactly you want to do here. It's up to you to replace on something more descriptive or just ignore it.
| val observable = CommitObservable.getObservable(git, serverRepo) | ||
| .publish() | ||
| CommitHasher(localRepo, serverRepo, api, rehashes) | ||
| .updateFromObservable(observable) |
There was a problem hiding this comment.
I would probably rename the method to chainWith(observable) or JSish then(observable) or you could wrap that by some fancy interfaces like (names and code are sketchy)
interface Thenable {
then(commits: Observable);
};
FunHasher : Thenable {
override then(Observable)
}
and then
Commits.asObservalbe().
then(FunHasher).
then(CommitHasher)
Codes of facts will be updated in other PR.