-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Working Git repositories #491
Conversation
This was the easiest way I could think of to let RepoPreferences connect a repo URI with a repo ID. I'm not sure if it's a good idea, as it required lots of changes to the SyncRepo creation calls. But they don't seem to have broken... yet. If we could store repository preferences in the database, instead of in an XML file, then RepoPreferences wouldn't have much need for the repo ID number anymore.
revision of a notebook, one for adding new notebooks, and one for deleting notebooks. I also renamed the variable repositoryPath to fileName, as I thought the name was misleading.
1. Create RepoPreferences in the new way, passing in the repo ID as a third argument. 2. Allow cloning into an existing but empty repository. 3. A working method for finding the latest revision of a notebook in the repo. 4. A working method for creating a new notebook. 5. A new method for deleting a notebook from the repository.
1. Use viewModel and RepoFactory, like the other repo activities. 2. Create RepoPreferences in the new way, with repoId as an argument. 3. Catch userData from RepoCreate which tells us the ID of a newly created repo. 4. Work around the file browser adding "file:" to the beginning of paths.
Hint to the user that the chosen local directory must be empty.
clone command fail. When JGit 5.3 or higher is released, we should switch to that. (The hard link issue seems to have been fixed in February 2019.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, I just realized the commit message of 553aa93 is wrong. I added the repo URI as a third argument to RepoPreferences, and the repo ID was added as a third argument to the SyncRepo build methods in RepoFactory. Oh well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in cabe012 the second change is to allow cloning into an empty folder, not repository...
@amberin Thanks for picking this up. I think that @nevenz wanted me to fix a bunch of smaller things (#173 (comment)) before making the git functionality live. Did you fix all of those? |
file, fileName, synchronizer.getFileRevision(fileName, commit)); | ||
synchronizer.tryPushIfUpdated(commit); | ||
return currentVersionedRook(Uri.EMPTY.buildUpon().appendPath(fileName).build()); | ||
synchronizer.addAndCommitNewFile(file, fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not make this change. I went through great pains to make it so that all of the operations here were safe. You've basically completely removed all of that safety.
You need to do what it actually says in the fixme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nevenz Can you clarify what you meant in this fixme? Is this information simply no longer stored at all? I'm not really sure what you mean by "get list from the remote". What we need is to be able to access that piece of meta data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanMalison I hear you, that's good feedback. But I found that this method is only used when a new notebook is created, and then I couldn't see neither the need for nor the possibility of checking for previous revisions of the new file.
I also had trouble understanding the fixme, fwiw...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIXME: Removed current_versioned_rooks table, just get the list from remote
@IvanMalison I think I just meant get this data by querying the remote repository (doing an API call).
To avoid the extra call - this data is available in getBooksFromAllRepos
. Interface could be changed so that VersionedRook
is passed where needed.
But as @amberin said, I don't think that storeBook
and retrieveBook
are used for TwoWaySyncRepo
repo when there is no matching remote notebook:
orgzly-android/app/src/main/java/com/orgzly/android/sync/SyncService.java
Lines 444 to 457 in cabe012
// FIXME: This is a pretty nasty hack that completely circumvents the existing code path | |
if (!namesake.getRooks().isEmpty()) { | |
VersionedRook rook = namesake.getRooks().get(0); | |
if (rook != null && namesake.getStatus() != BookSyncStatus.NO_CHANGE) { | |
Uri repoUri = rook.getRepoUri(); | |
SyncRepo repo = dataRepository.getRepo(repoUri); | |
if (repo instanceof TwoWaySyncRepo) { | |
handleTwoWaySync(dataRepository, (TwoWaySyncRepo) repo, namesake); | |
return BookAction.forNow( | |
BookAction.Type.INFO, | |
namesake.getStatus().msg(repo.getUri().toString())); | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memory is hazy, but I'm quite sure I had a reason for writing the code and structuring it as I did. It should not be hard to do the right thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as @amberin said, I don't think that storeBook and retrieveBook are used for TwoWaySyncRepo repo when there is no matching remote notebook
I've just checked, and it seems to me that storeBook()
only gets called 1) when there is no matching remote notebook, and 2) upon forced save. What risks do you see in those scenarios, @IvanMalison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) this is only true because this is a a twowaysync repo, right? storeBook is not an add file method.
b) I simply don't want any code paths at all that do things unconditionally. This makes it much harder to have anything go wrong, or for future changes to accidentally make something unexpected happen.
c) Doing the right thing here shouldn't be hard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (syncBackNeeded) { | ||
writeBack = synchronizer.repoDirectoryFile(fileName); | ||
} | ||
writeBack = synchronizer.repoDirectoryFile(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember all the details, but I couldn't get the TwoWaySyncResult() constructor to run when writeBack was not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't do this. Avoiding sync back when it is not needed makes syncing much more light weight and efficient in the case where many files are involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are probably right. I am now unable to reproduce the issue that I was having when I made this change, so I will revert this.
FWIW, I am now also unable to create a situation where this syncBook()
method is invoked without syncBackNeeded
ending up true, so it isn't being invoked for books without changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This was reverted in ccfb719)
@IvanMalison I definitely did not fix all of those issues -- I just got to the point where the functionality can actually be used. I'd be happy to fix more bugs and work more on it, but first I wanted to check if these changes are insane or not... I'm not sure if the browser-related issues still apply. I'm testing on my Samsung, and it seems like their file browser is a bit special. I believe the most demanding issue in that list (for me) is support for user/password auth. That is definitely not something I would know how to go about implementing. |
The changes are definitely not in line with the what I originally intended with the code.
That should not be very hard. JGit has user/password auth facilities: https://www.codeaffine.com/2014/12/09/jgit-authentication/ they give this example: CloneCommand cloneCommand = Git.cloneRepository();
cloneCommand.setURI( "https://example.com/repo.git" );
cloneCommand.setCredentialsProvider( new UsernamePasswordCredentialsProvider( "user", "password" ) ); Orgzly just needs to figure out how it wants to store this data. |
private VersionedRook currentVersionedRook(Uri uri) throws IOException { | ||
RevCommit newCommit = synchronizer.currentHead(); | ||
return new VersionedRook(getUri(), uri, newCommit.name(), newCommit.getCommitTime()*1000); | ||
private VersionedRook currentVersionedRook(Uri uri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are all of these changes for? Why would the name attribute be used in the versioned rook object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the mtime and revision hash of all notebooks was being set to that of the entire repo's latest commit. With this change, the mtime and revision hash shows correctly for each notebook. The previous situation also meant that after every commit, all unchanged notebooks would have their mtime and revision hash updated during the subsequent sync (to that of the latest commit), which produced the CONFLICT_LAST_SYNCED_ROOK_AND_LATEST_ROOK_ARE_DIFFERENT
status.
I haven't changed which arguments get passed to the VersionedRook
constructor. The name attribute is the commit hash which forms the revision ID of the rook.
if (syncBackNeeded) { | ||
writeBack = synchronizer.repoDirectoryFile(fileName); | ||
} | ||
writeBack = synchronizer.repoDirectoryFile(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't do this. Avoiding sync back when it is not needed makes syncing much more light weight and efficient in the case where many files are involved.
Before this, Git repo settings were not saved properly when an existing repo was updated.
@amberin if there's any help needed to make this reach it's goal i'd be happy to help out, programming wise or otherwise :) |
@lfxgroove Take a look at this comment on my original pull request: #173 (comment) That's the work that needs to get done. |
List to keep track of what's done, taken from #173:
Minor stuff
|
@lfxgroove Why are you tracking all of this here? Seems like #24 Would be the right place. I haven't looked at the most recent changes, but I'm not totally sold on all of the things @amberin is doing in this PR. If you resolver any of the issues @nevenz had it should probably be as part of a separate PR. |
@IvanMalison good question, don't really know, i'll post on that issue instead. |
OK, so, first of all... I'm not really a programmer, much less a Java programmer. I just really wanted this functionality myself. I used this as a kind of exercise to learn something about Java and app development.
I've probably made some horrible decisions here, but this code seems to work well for me (on a Samsung S7), and I'm hoping that someone more experienced will be able to suggest improvements if this is not acceptable. I'd also be willing to re-work things if I can get pointers in the right direction.
I believe the commit messages explain more or less all my changes.