File Lock (EE option)

Title changed from Binary File Lock to Binary File Lock (EE option)

@dzaporozhets can you give an estimate of the amount of work that is required?

What would be the minimal implementation of this?

How does LFS play into this?

@JobV I propose @DouweM will do estimates from now since he is doing development planning now. I hope @DouweM does not mind.

What would be the minimal implementation of this?

UI with lock button. Put file path to database when file locked. Deny push which affect this file unless pushed by person who locked.

How does LFS play into this?

Should affect any file LFS or not. But lets five developer who will implement it to share some light on it.

Thanks @dzaporozhets. Agree with all.

If we add the project.path_locked?(path) check into Gitlab::GitAccess, it will automatically apply to web edits, git pushes and LFS pushes.

So this feature entails:

Adding model for locked files or directories
Adding "Lock"/"Unlock" button in UI. A locked file or directory can only be unlocked by the lock user or a project master
Disable edit, replace, delete, new file buttons with a tooltip as appropriate
Show lock icon by any locked file/dir with a tooltip
Add page to list all locked files/dirs to get overview in case people forget to unlock
Add path_locked? check en error message in Gitlab::GitAccess

@JobV What release are you thinking? EE only or not?

@DouweM EE option. I'm not sure whether this should be an EE option. It is definitely interesting, but because it's so integrated I worry about the implications on the code and the UI. I also wonder about the business viability of this as an option

@JobV Thanks, as @DouweM said "EE vs EE option doesn’t make a different implementation wise"

Milestone changed to 8.10

Moved from gitlab-org/gitlab-ce#7889

Added repository label

Added ~126459 label

Milestone changed to 8.10

Deny push which affect this file unless pushed by person who locked.

If the goal is to prevent accidental changes, we should block pushes even by the person who locked it. Make them unlock first before allowing the change.

A locked file or directory can only be unlocked by the lock user or a project master

As a simplification, how about only allowing project masters to lock and unlock? Then you don't need to record who did the locking. I don't know how customers intend on using this feature, so I don't know if that's a palatable solution or not. But it would be simpler. :) And it might be easier for people to reason about.

Eventually, if this is a really useful feature, I could imagine people asking for a new role specifically for this, in between developer and master. But if it's lightly used, then requiring master might be sufficient.

Think of this more as checking out files for you to work on more than locking them against change, hence the person that locked can push.
Developers should be able to work on files too, see use case 1.

Maybe Reb can describe the use case better, I emailed him to comment since he doesn't have an account added yet.

@markpundsack you had an idea for a better name, feel free to change it

If the goal is to prevent accidental changes, we should block pushes even by the person who locked it. Make them unlock first before allowing the change.

@markpundsack @sytses This feature originated from the problem with LFS and binary files, where if two people edit a binary file, the files becomes unmergeable after the first person pushed. So you want to be able to lock a file to edit it safely, preventing your colleagues from editing the file before you manage to push. So not only do you want to be able to push to the file you locked, we might even want to specific who locked the file.

So developers and up should be able to lock any file and push to it, preventing everyone else from pushing.

Git LFS is considering implementing this, but it's in a very early stage: https://github.com/github/git-lfs/pull/666/files

I've edited the description for a more clear explanation.

I'd expect users to use this largely as @sytses said, as a way of checking things out and not necessarily as a way of preventing change. But the latter is something I would absolutely expect to see in some cases. I'd also say from experience that as soon as it is introduced users will lock a file and then disappear (for a beer, for a vacation, etc.). So, a well-defined way of allowing an administrator to easily unlock files is absolutely required.

@markpundsack I'm not sure of the infrastructure costs associated with keeping track of who created the lock is, but it would absolutely be useful to keep track of this. When overriding/removing a lock knowing who did what and why is valuable information.

In monolithic tools such as Perforce this sort of feature is regularly used by default on files with defined extensions and/or paths (.jpg, .pdf, .exe, .gz, etc.) One may consider it bad practice but as LFS makes storage of files far beyond the realm of source code and text files easier, some organisations are going to start putting everything in.

So, a well-defined way of allowing an administrator to easily unlock files is absolutely required.

...

I'm not sure of the infrastructure costs associated with keeping track of who created the lock is, but it would absolutely be useful to keep track of this.

@xyzzy Both are addressed in my proposal at https://gitlab.com/gitlab-org/gitlab-ee/issues/497#note_4922640

I like @DouweM proposal https://gitlab.com/gitlab-org/gitlab-ee/issues/497#note_4922640.

I, too, like the proposal. I would change it slightly when it comes to "Show lock icon by any locked file/dir with a tooltip" to add an indication on the file without a tooltip. Having to hover over the file entry to see whether it is locked isn't user friendly if one is looking at a large list of files. I think a slightly different file icon, with a lock on/next to it, would be a better way to implement this.

@DouweM -- does my file icon with lock proposal (see previous message) seem okay? I'm not sure what the protocol is for suggesting changes to implementation proposals.

@sytses @JobV Thanks for clarifying! I completely misunderstood the feature.

OK, now that I've thought about it a bit more, with a better understanding of the use cases, I'm not sure the proposal actually works. Let me know if I've missed something.

The git-lfs proposal gets to the heart of it. You need to make sure that you're on HEAD when you lock (which is hard for a web interface that has no idea what you've got locally) and that any subsequent pushes to the file in question are based off other revisions in a linear fashion.

Consider the following examples:

Sequential

__________________________
  \________/    \_____/
      A            B

Encompassed

______________________________
  \          \_____/      /
   \            B        /
    \___________________/
           A

Overlapping

__________________________
    \       \ _/____/        
     \        /  B             
      \______/
         A

In (1), there's no problem, assuming A and B both pull before they start to write changes because the changes are sequential and there is no lock contention.

(2) and (3) aren't problems IF A and B try to grab the lock before making any changes since B would be blocked from grabbing the lock while A has it. Yay, right!

The problem is that the files are read-write locally. Other VCSs would have your local version be read-only until you checked out the file, and at that time, they would block other users from checking out. But git doesn't work that way.

It's going to be really easy to forget to grab the lock before you start modifying it. So back to case (2), if you only try to grab the lock at checkin time, B will grab the lock, commit the file, then release the lock. Then some time later, A finally goes to commit the long-running changes, grabs the lock, commits (overwriting previous changes) and then releases the lock.

How does the proposal above protect against this? Or do we consider it not worth protecting against this?

An alternate proposal might be a bit more complicated. For example, you could check every push on the file to see if it is derived from master/HEAD and block anything that isn't. I think you need a protected? attribute in addition to knowing if it is currently locked?. That way you do the HEAD check for every push, not just when it's currently locked. It's precisely when it's not locked that the merge conflicts come in. At this point, locked? is really just a courtesy to other people; the real work is done by enforcing linear changes for every push.

As a side note, if locking is available via the API and you use something like git lab lock foo, the CLI could make sure foo is at master/HEAD before proceeding, which might help reduce surprises. This is likely something best solved in git itself. :)

How does the proposal above protect against this? Or do we consider it not worth protecting against this?

@markpundsack I think we should keep it simple as a first iteration and only lock the remote file, not protecting against diverging remotes.

Situation 1 would be covered.
Situation 2 (encompassed), would act as you describe (A overwrite B). This is a typical danger with long-diverging remotes and one that you should take into account anyway. Eventually we should think about warning the user.
Situation 3 is similar. I think for the first iteration we can assume that before work starts, you update your local copy, therefore checking the lock.

I think this is the simplest issue we can solve right now: preventing conflicts by blocking users from pushes that make changes to the file that is locked. It's relatively easy to implement and gives us plenty of space to improve. As a means to alleviate the issues you mentioned, we can think about reporting to the user whether certain files are locked on fetching and making it obvious who locked what and when.

CLI for this would be excellent, but if it's git itself we don't have the engineering capacity at the moment.

@JobV OK, if we're not worried about the robustness of the lock, we can tell people to lock the file before they start making any changes, and sync to HEAD before making any changes. It'll be almost purely a policy thing rather than enforcement. So if users don't lock before starting changes, they likely will run into problems. But if everyone follows the policy, then the lock will help prevent accidental problems.

@xyzzy My idea was to add an always-visible lock icon, with a tooltip on that lock icon with text like "Locked by Douwe Maan 5 minutes ago"

@DouweM sounds good! I only wrote what I did above because the indicator was defined in the feature description as being made via a tooltip.

@DouweM can you add to the description?

@JobV Done

We can consider releasing most of the code of this feature in CE and allow additional management as an EE option (override lock by owner/master/admin).

Added ~139512 label

Milestone changed to 8.9

Title changed from Binary File Lock (EE option) to File Lock (EE option)

Reassigned to @vsizov

I started working on this. I will create MR once I have something.

I'm back, spent all this time merging CE to EE.

@JobV What the final decision on should it be EE feature or EE option?

@vsizov option.

@DouweM we are checking the locks when push goes to the default branch, right? I think locks should not affect user's branches.

The WIP MR is here https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/440

There is one problem. Currently we have a bug in git hooks since we implemented the MR merging by using rugged instead of satellites. In fact our git hooks does not work when we do something with repository using UI, there is an issue https://gitlab.com/gitlab-org/gitlab-ee/issues/587. The issue is fresh but the bug existed a long time ago and I described this problem here https://gitlab.com/gitlab-org/gitlab-ee/issues/279#note_3783022 so it's not easy to fix. (cc @jameslopez )

OK, how is it related to current feature?! File Lock feature relies on the same functionality as git hooks. In other words until git hooks is not fixed the file lock feature will not work when you change file using UI.

@DouweM

@JobV Why is this being implemented as an option vs. as a feature? I think it's correct that this belongs in EE -- but I'm curious why we think this shouldn't be part of the base EE product.

@vsizov Excited to try this out. Is there a way to lock files from command line and view locked files from command line before a push?

nope, and we didn't plan to do it AFAIK.

@deilering we'll use Lab CLI for that, planned for 8.10 :)

@xyzzy we think this is something that is not necessarily interesting for all teams and offers substantial value for larger teams working with media. If we're wrong, we can always merge it back in EE or even CE, but the other direction is not possible; therefore we start with it as EE option.

We might consider one day offering the basic feature in CE and offer advanced access control as EE option.

mentioned in merge request !440 (merged)

Status changed to closed by merge request !440 (merged)

mentioned in commit 385807e5

mentioned in commit cda8de63

Status changed to reopened

Reopened, there are still TODOs left. cc @vsizov

Status changed to closed by commit dblessing/gitlab-ee@cda8de632c3c412afec26f2b5700402d6dc1e663

Mentioned in commit 385807e5

@JobV is there an API to toggle and view lock status? Could not see anything on https://docs.gitlab.com/ee/api/README.html

@deilering Nope, this feature is not present in the API.

Mentioned in issue #1299

Created an issue https://gitlab.com/gitlab-org/gitlab-ee/issues/1299

mentioned in issue #1972

File Lock (EE option)

Description

Use cases

Implementation

References

Todo

Designs

Child items ...

Activity

Admin message

Admin message

File Lock (EE option)

Description

Use cases

Implementation

References

Todo

Activity