Skip to content
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

Move Git working directory to private storage #21

Open
amberin opened this issue Oct 11, 2023 · 17 comments
Open

Move Git working directory to private storage #21

amberin opened this issue Oct 11, 2023 · 17 comments
Labels
enhancement New feature or request Git repos Related to Git syncing

Comments

@amberin
Copy link
Member

amberin commented Oct 11, 2023

Copying from orgzly/orgzly-android#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. orgzly/orgzly-android#24 (comment) and orgzly/orgzly-android#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 orgzly/orgzly-android#543 (comment):

@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.

@amberin amberin added the enhancement New feature or request label Oct 11, 2023
amberin referenced this issue in amberin/orgzly-android-revived Oct 12, 2023
and remove all Git settings related to the workdir location.

The directory containing the workdir is named using the repo ID. This
should allow changing the URL of an existing repo.

I thought for a while about a feature to copy/export the directory to
public storage for troubleshooting purposes, but I came to the
conclusion that it should not be necessary. If the user is worried about
losing local state, they can always export the relevant notebooks as
text files, and re-import them later. If the repo is somehow broken, it
is best to delete it and add a new one, re-linking the notebooks (and
deleting the local copies, if necessary). If there are tenacious bugs,
they should be ironed out on a virtual device with full root access.

This resolves #21.
amberin referenced this issue in amberin/orgzly-android-revived Oct 12, 2023
and remove all Git settings related to the workdir location.

The directory containing the workdir is named using the repo ID. This
should allow changing the URL of an existing repo.

I thought for a while about a feature to copy/export the directory to
public storage for troubleshooting purposes, but I came to the
conclusion that it should not be necessary. If the user is worried about
losing local state, they can always export the relevant notebooks as
text files, and re-import them later. If the repo is somehow broken, it
is best to delete it and add a new one, re-linking the notebooks (and
deleting the local copies, if necessary). If there are tenacious bugs,
they should be ironed out on a virtual device with full root access.

This resolves #21.
amberin referenced this issue in amberin/orgzly-android-revived Oct 12, 2023
and remove all Git settings related to the workdir location.

The directory containing the workdir is named using the repo ID. This
should allow changing the URL of an existing repo.

I thought for a while about a feature to copy/export the directory to
public storage for troubleshooting purposes, but I came to the
conclusion that it should not be necessary. If the user is worried about
losing local state, they can always export the relevant notebooks as
text files, and re-import them later. If the repo is somehow broken, it
is best to delete it and add a new one, re-linking the notebooks (and
deleting the local copies, if necessary). If there are tenacious bugs,
they should be ironed out on a virtual device with full root access.

This resolves #21.
amberin referenced this issue in amberin/orgzly-android-revived Oct 12, 2023
and remove all Git settings related to the workdir location.

The directory containing the workdir is named using the repo ID. This
should allow changing the URL of an existing repo.

I thought for a while about a feature to copy/export the directory to
public storage for troubleshooting purposes, but I came to the
conclusion that it should not be necessary. If the user is worried about
losing local state, they can always export the relevant notebooks as
text files, and re-import them later. If the repo is somehow broken, it
is best to delete it and add a new one, re-linking the notebooks (and
deleting the local copies, if necessary). If there are tenacious bugs,
they should be ironed out on a virtual device with full root access.

This resolves #21.
@dwoffinden
Copy link
Contributor

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
Copy link
Member Author

amberin commented Oct 13, 2023

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
Copy link
Member Author

amberin commented Oct 13, 2023

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
Copy link
Contributor

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

@amberin
Copy link
Member Author

amberin commented Oct 13, 2023

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 amberin added the Git repos Related to Git syncing label Oct 14, 2023
@amberin
Copy link
Member Author

amberin commented Oct 27, 2023

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
Copy link
Contributor

alensiljak commented Nov 13, 2023

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
Copy link
Member Author

amberin commented May 4, 2024

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
Copy link
Member Author

amberin commented May 15, 2024

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
Copy link
Member Author

amberin commented May 15, 2024

Maybe start with 1, and then do 2 later?

@alensiljak
Copy link
Contributor

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
Copy link
Member Author

amberin commented May 15, 2024

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
Copy link
Contributor

alensiljak commented May 15, 2024

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
Copy link
Member Author

amberin commented May 15, 2024

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
Copy link
Member Author

amberin commented May 18, 2024

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
Copy link

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
Copy link
Member Author

amberin commented Sep 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Git repos Related to Git syncing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants