orgzly-revived / orgzly-android-revived

Outliner for taking notes and managing to-do lists
https://www.orgzlyrevived.com
GNU General Public License v3.0
654 stars 41 forks source link

Move Git working directory to private storage #21

Open amberin opened 1 year ago

amberin commented 1 year ago

Copying from https://github.com/orgzly/orgzly-android/issues/941:

It always seemed wrong to me that the Git working directory is placed in public storage.

The other day, my Pixel suggested I clean up some "unused files" in my public storage, and because I was not paying proper attention, I ended up trashing some files from Orgzy's .git directory. (I then tried to restore them from the trashbin, but my workdir was still broken for some reason.)

This is just one of the issues with storing the Git dir in public. I have also seen multiple bugs related to directory selection/creation and storage permissions (e.g. https://github.com/orgzly/orgzly-android/issues/24#issuecomment-1025506911 and https://github.com/orgzly/orgzly-android/pull/916). The fact that the user needs to create an empty directory before adding a Git repo is also pretty bad UX, in my opinion.

I'm thinking this issue could be a forum for discussing the pros and cons of private vs public storage for this directory.

I'll start by quoting an old comment by @IvanMalison in https://github.com/orgzly/orgzly-android/pull/543#discussion_r288245604:

@amberin, yes, conflict resolution DOES likely happen on another system, but even in that case, we need to be able to manipulate the raw git repository, because when there IS a conflict, we will need to put the changes from orgzly somewhere back out there (i.e. on a branch that is not matched up with master). Then those conflicts need to be resolved and synced back in to the main master branch.

I suppose that we could have orgzly simply push its view of master to another branch, while leaving its view of the world as the local master, but I still think there is value to exposing the underlying git repository to the user just in case things get kind of hairy.

(Since then, this has more or less been implemented; we push "conflict branches" to remote, and conflicts are expected to be resolved on another device. We then automatically return to the main branch as soon as possible.)

I can see the value in being able to manually poke around in the Git working directory using some Git app on the phone. However, I have personally never gone to the trouble of doing so, despite many hours of Orgzly development which has put my phone in various weird states. And as my recent experience shows, there is a flipside to this ability. In my case, I was indeed unable to restore my .git dir to a working condition despite having access to all the files and tools. (This may of course be attributed to my own stupidity, but it is probably not completely unparalleled.)

If this is ever implemented, one might add a button in the RepoActivity for copying the .git and working directories to public storage, so that its contents can easily be salvaged if the remote is somehow lost.

dwoffinden commented 1 year ago

I think this is a worthwhile change, and should make setup a lot easier.

I usually resolve conflicts on another device. The only reason I have for poking around in there is to occasionally run git gc when syncing gets slow, which orgzly doesn't seem to do on its own?

It's also sometimes nice to just check that's it's up to date. Android Password Store does have a "commit log" UI for that?

amberin commented 1 year ago

Quoting @colonelpanic8 in #23:

@amberin I honestly kind of like having the ability to view the git repository. I sometimes manipulate it with an external git utility when there is a need to resolve conflicts.

This is a very valid point.

For me, it's become primarily a security issue. I really don't like the idea of so many apps on my phone being able to read all my notes "for free".

After that, my experience with Android cleaning out "unused" Git files (described above) seems like an issue to me.

The fact that we need to give Orgzly the "manage external storage" permission for the current solution to work seems to me like a hint that we're working against Android's design. We've also had quite a few other bugs related to the necessary storage permissions, and in my PR #23 I was able to remove a lot of checks and logic, which felt right.

Lastly, it's a UX issue for me. I feel the user should be able to add or remove repos easily, without preparing empty folders first.

But all of the above is perhaps outweighed by the ability to view the Git repo and resolve conflicts on the phone? I have never done it myself, but it sounds like a UX positive, compared to the complexity of having to resolve conflicts on another device.

Personally, I think I would prefer the design simplicity of private storage, but that's just me.

More thoughts, anyone?

amberin commented 1 year ago

I usually resolve conflicts on another device. The only reason I have for poking around in there is to occasionally run git gc when syncing gets slow, which orgzly doesn't seem to do on its own?

Off topic, but I have been wondering about git gc myself, and I have done the same once - before I found out that temp branches were not being deleted properly, which slowed things down immensely.

IIRC, the Jgit docs says that it should happen regularly as with any git client. I wonder what is preventing it...

colonelpanic8 commented 1 year ago

Why can't we just leave both modes of operation?

amberin commented 1 year ago

Why can't we just leave both modes of operation?

Yes, a button to choose between private or public workdir when creating a repo would be cool. I will look into it shortly.

It adds complexity, of course, but perhaps not an awful lot.

amberin commented 1 year ago

Just noting down for future reference:

I have looked into simplifying the file picker experience for external storage by using similar code as for the "directory" repo type. I concluded that this is not possible, since that code uses Android's Storage Access Framework, and it seems to be impossible to get a plain java.io.File object with that, which is the only type of storage abstraction that JGit supports.

alensiljak commented 12 months ago

I'm actually all for having the repositories in an accessible location. That's how I'm currently using them. They're available in Termux, which is where I use git, as well as to other Org tools. Android has already limited access to the public folders and a specific permission is now required. Probably adding a parameter to the setup would help to choose the storage type.

amberin commented 6 months ago

Google is becoming very restrictive about accepting new apps in the Play Store which declare the MANAGE_EXTERNAL_STORAGE permission, which is needed for Git workdir in external storage to work. Hopefully, they will deem our use of it legitimate, but if they don't, we must remove the current Git repo solution from the Play Store version. But I suppose that version could offer Git repos in private storage as the only alternative.

But judging by #237, the sync/merge logic is perhaps not yet reliable enough to move the workdir into private storage.

amberin commented 5 months ago

We were just rejected from the Play store with the following motivation:

Issue found: Not a core feature The app feature that you declared as dependent on the All files access permission didn’t meet the policy review requirements for critical core functionality.

Core functionality is defined as the main purpose of the app. Without this core functionality, the app is "broken" or rendered unusable. The core functionality, as well as any core features that comprise this core functionality, must all be prominently documented and promoted in your app's store listing description on Google Play.

Update your app so that the feature doesn't use this permission. If your app doesn’t require access to the MANAGE_EXTERNAL_STORAGE permission, you must remove it from your app's manifest in order to successfully meet the policy review requirements.

Alternatively, if your app is “broken” or rendered unusable without this permission, it’s likely your earlier declaration was misstated or is unclear. Ensure that the core functionality and technical justification are clearly documented and promoted in the app's description when you complete the Permissions Declaration Form, and resubmit your app on the Play Console.

I see three possible routes here:

  1. Remove Git repos from the Play store version
  2. Place Git repos in private storage in the Play store version
  3. Don't change the app, but try to portray Git repos as a critical core feature in the app description on Play store.

Since the feature is still in beta, I wouldn't feel great about number 3. I suppose I'd prefer number 2.

amberin commented 5 months ago

Maybe start with 1, and then do 2 later?

alensiljak commented 5 months ago

All I can say is that I wish you luck trying to comply with all the Play Store requirements over time. That is one of the main reasons why I have abandoned the maintenance of another open source app and I have no time or energy to do it again. Hence I will excuse myself from anything that has to do with the Play Store. My suggestion would be to use F-Droid exclusively, if necessary.

amberin commented 5 months ago

I hear you. In this particular case, though, I agree with Google. It has always bothered me that the Git workdir content is publicly accessible in this way. (Even though it can simplify troubleshooting.)

alensiljak commented 5 months ago

It all depends on how you use your device. A cornerstone piece of software on my phone is Termux, which is no longer available on Play Store due to the idiotic restrictions. It turns the phone into an actual computer, that it physically is, instead of being a play toy. There's nothing wrong with multiple applications having access to the same directory with files. If you want to have any sort of interaction or use multiple apps on the same set of files, this is a perfectly valid and simple way of doing it. I.e. using both Orgzly and Logseq on the same files, synchronizing them with the actual Git application and Lazygit UI from Termux. I understand the security repercussions, etc. But, the fact that you can smash your fingers with a hammer does not mean that a hammer should be prohibited. I don't like this lowest-common-denominator lowering of standards and abilities. Pure idiocracy.

amberin commented 5 months ago

Of course we should not limit people's choices any more than necessary. If you want to access the files using multiple apps, the directory repo type is not going anywhere. And as for the git repo type, I am fully in favor of providing the choice between private and public storage, for those who want to poke around in there (although I believe this is risky, as it will result in mixed file ownership).

amberin commented 5 months ago

Quoting @dwoffinden:

I usually resolve conflicts on another device. The only reason I have for poking around in there is to occasionally run git gc when syncing gets slow, which orgzly doesn't seem to do on its own?

I have reached the conclusion that JGit I/O performance is simply bad, which makes it necessary to GC quite often. Cloning repos is always incredibly slow, and I have seen complaints about this from JGit users on other platforms. This indicates to me that the file handling is just slow in general. I will suggest setting gc.auto to the lowest possible value, which is 256. I have tested with 500, but I then had to wait too long for GC.

It's also sometimes nice to just check that's it's up to date. Android Password Store does have a "commit log" UI for that?

I was recently reminded that you can see the latest commit hash for each notebook if you enable it in settings. Perhaps useful in this context.

felixwiemuth commented 1 month ago

Regarding allowing to place the git directory in a non-private directory - is this only possible with the external storage permission (giving access to all files)? With the Storage Access Framework the user can grant permission for a directory; is that possible to use with the git library?

amberin commented 1 month ago

@felixwiemuth Not until JGit has a storage abstraction which supports DocumentFile instead of java.nio.File.