-
Notifications
You must be signed in to change notification settings - Fork 32
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
Auto lock support #3
Comments
There's APIs that could make this possible, but git lfs is very, hmmm, convoluted when it comes to locking apis, it requires an actual server user, which Git for Unity doesn't have, so this requires a bit more work than just hooking up to IsOpenForEdit and popping up a dialog. I would say this would need
Some of this was done in GitHub for Unity, but the GitHub-specific auth backend was removed in the fork, and GitHub deprecated all the auth apis, so none of that works anymore. And, of course, none of this should be GitHub-specific, so the user configuration for lfs needs to be done in a way that would work for any lfs server. lfs sucks, and lfs locking extra sucks, and while the use case for locking is very valid, I don't think lfs should be handling locking at all - it doesn't do it well, and it does it too late, and why is a storage service handling editing permissions in the first place anyway? It's a storage server, it stores things, it shouldn't be responsible for whether you can edit your scene or not. I'll think about it. |
yeah, agreed it's quite frankenstein. |
Git providers shouldn't be providing locking - like lfs, git is a storage database, whatever it does is way too late when it comes to you wanting to edit a scene. Every single provider is going to implement a completely different system, with completely different features and separate user matching implementations, and chasing that is the worst. lfs locking doesn't support per branch or per group of branches locking, which means if you request a lock on a feature branch, nobody working on a development branch will be able to edit something - it kills whatever usefulness locking has in lfs, so you can throw that out of the window as well. No, this needs a completely separate permissions service that can track the information of who wants the lock in which scope, a service that only does that one thing, independent of whatever git or large file storage you happen to be using to store your stuff. Also, nitpick here, but git is not a distributed system in the reality of 99% of teams - you have a git server where you store your stuff, that server is authoritative in the large majority of cases, nobody is going to be going around juggling multiple different git server remotes to share their stuff with other people. You push to the server, you pull from the server, it's centralized as far as you're concerned. What's really important to keep in mind is that it's not a "server" with features that you can configure, it's just a dumb database running somewhere that you have no control over. Trying to tack on features like locking on top of it is really a case of round hole, square peg (or the other way around, I can never remember). That's not to say it's useless - we can certainly run other services to provide extra features for version control workflows - it's just that it's a database, that's all it is, it tracks no state beyond what you store in it. |
I'd say this is a feature actually. The goal of a locking mechanism is to prevent merge conflicts on unmergeable assets like images and the such. If I work on a feature branch and want to merge to develop while someone else already changed the asset on develop, that'll create a merge conflict I can't resolve when trying to merge my feature branch back to develop. A whole-repo locking mechanism would help with this. Perforce has per-stream locking cause their workflow involves way less branches than git. They added branches quite recently and most of the main workflows are branch less. When I was working on productions that were using perforce, yes there was always the odd "hey! the intern forgot to unlock asset x before going on vacation, no one can edit it!" but there was always a tech lead able to force unlock the thing. Plus this forces good practices on asset separation (for example making sure your scenes are separated correctly with domain based additive scene loading; lighting artists would work in their own scene, audio same, level designers same, etc). This way, you reduce the surface for merge conflicts in the first place, locking becomes an edge case safeguard, not something you bump your head against all the time. What other locking provider exists that'd be easy to use by users and that's in sync with the whole repo? |
Plus thinking of it, the locking mechanism would need to be aware of git logic, no? Take this case: |
Will these functions be added? I also downloaded this plug-in for these functions, but found that it was not implemented. |
+1 |
Hello! Just tested real quick with this updated version with 2021LTS. It's great this project has been packagified and has been ported to latest LTS! Thank you!
Dunno where you're at with porting over issues from the old github, but here's one:
This is mostly a feature request: right now when editing a scene, I'll still be able to edit it even though the scene is unlocked. There's no prompt to ask me to lock a scene.
Same when someone else has locked a scene already, at that point I shouldn't be able to edit it at all.
The above would be the main reason for me to use this package, as locking + unity can't be achieved outside of a unity plugin (same as the perforce plugin that's required to use unity).
Thanks a bunch!
Prerequisites
%LOCALAPPDATA%\GitHubUnity\github-unity.log
~/Library/Logs/GitHubUnity/github-unity.log
~/.local/share/GitHubUnity/github-unity.log
Description
Steps to Reproduce
Expected behavior: [What you expect to happen]
Actual behavior: [What actually happens]
Reproduces how often: [What percentage of the time does it reproduce?]
Additional Information
Any additional information, configuration or data that might be necessary to reproduce the issue.
The text was updated successfully, but these errors were encountered: