Provide exclusive lock information to the frontend in CE as well as EE

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Context

The Frontend team identified that the lock icon isn't being shown on the CE version of GitLab's repository tree view.

See this discussion for more detailed context: #525679 (comment 2432221338)

Unrefined Description

Details

From the discussion linked above:

Without checking, I would assume the reason we have this inconsistency is because we probably implemented LFS (which include file locking) after GitLab's own Path Locks which only exists in EE. Or we just didn't think to add it to the LFS locks for some reason, maybe it was an oversight.

Git LFS' File Locks and GitLab's Path Locks are 2 different mechanisms but produce the same result and in EE are synced up with each other (assuming the file is an LFS file).

  • TreeSummary checks feature availability: return unless project.feature_available?(:file_locks). CE would return false for this.

Interestingly I think this is likely causing a secondary bug in EE where you have file_locks disabled but LFS is enabled, the lock icon won't show for locked LFS files but haven't verified that.

For lfs File Locks we don't check feature_available? we use project.lfs_enabled? (or something like this). We should be able to modify the logic to check for a PathLock if file_locks feature is enabled or LfsFileLock if the file is an project.lfs_enabled? and file.lfs? (not real code).

  • Can locks created with Git LFS/.gitattributes be find with finder = ::Gitlab::PathLocksFinder.new(project)?

Not exactly. If the locks are synced then one would show up, but normally we'd use the Lfs::LocksFinderService

One important note, all LfsFileLocks can also be PathLocks, but a PathLock must be for an LFS file for it to be synced with LfsFileLocks.

A Path Lock can lock any file or directory. An LFS File Lock can only lock a file that is stored in LFS.

^ from @jwoodwardgl

Goal of this issue

Work on plugging the gap by the fact LFS can be used in CE but the code doesn't seem to expect file locking to occur in CE because the feature of Path Locks is EE only.

We still need to detail this a bit further to identify what needs changing.

Goal of this issue

Work on plugging the gap by the fact LFS can be used in CE but the code doesn't seem to expect file locking to occur in CE because the feature of Path Locks is EE only.

Proposal

In Gitlab::TreeSummary we have a summarize method which is then overridden in EE::Gitlab::TreeSummary#summarize

This override fills the path lock information

      def summarize
        super.tap { |summary| fill_path_locks!(summary) }
      end

      private

      def fill_path_locks!(entries)
        return unless project.feature_available?(:file_locks)

        finder = ::Gitlab::PathLocksFinder.new(project)
        paths = entries.map { |entry| entry_path(entry) }
        finder.preload_for_paths(paths)

        entries.each do |entry|
          path = entry_path(entry)
          path_lock = finder.find_by_path(path)

          entry[:lock_label] = path_lock && text_label_for_lock(path_lock, path)
        end
      end

      def entry_path(entry)
        File.join(*[path, entry[:file_name]].compact).force_encoding(Encoding::ASCII_8BIT)
      end

Important note

If project.lfs_enabled? and licensed_feature_available?(:file_locks) in EE we sync the 2 features when an LFS file is locked so any LfsFileLock will also have an accompanying PathLock if file locks are enabled. However, in file locks is an EE feature so in CE we will always only be able to find LfsFileLocks, but only if project.lfs_enabled?.

Due to this quirk I think we can add logic to CE to check for LfsFileLocks if project.lfs_enabled? and then keep the fill_path_locks! override in EE but instead of

      def fill_path_locks!(entries)
        return unless project.feature_available?(:file_locks)

it would return super.

      def fill_path_locks!(entries)
        return super unless project.feature_available?(:file_locks)

We can remove the override for summarize and move the logic for this into CE

-      def summarize
-        super.tap { |summary| fill_path_locks!(summary) }
-      end
    def summarize
      return [] if offset < 0

      commits_hsh = fetch_last_cached_commits_list
      prerender_commit_full_titles!(commits_hsh.values)

      entries = commits_hsh.map do |path_key, commit|
        commit = cache_commit(commit)

        {
          file_name: File.basename(path_key).force_encoding(Encoding::UTF_8),
          commit: commit,
          commit_path: commit_path(commit),
          commit_title_html: markdown_field(commit, :full_title)
        }
      end
      fill_path_locks!(entries)
    end

    private

    def fill_path_locks!(entries)
      return unless project.lfs_enabled?

      find the lfs file locks and do the same thing as EE
    end

Refactoring the solution to find the locks before looping?

I'm not sure if this would improve anything but it may be possible to remove a loop here if initialize the finder before mapping the commits_hsh.map do |path_key, commit| so we lazy load the locks as needed instead of looping once, then taking all of the paths and looping again.

Edited by 🤖 GitLab Bot 🤖