Feature #13945

Disable autofetching of repository changesets if projects are closed

Added by Mischa The Evil over 4 years ago. Updated over 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:SCM
Target version:2.4.0
Resolution:Fixed

Description

When Setting.autofetch_changesets == true repository changesets are automatically fetched (and added) when Repository#show is executed.
This behavior feels a bit weird if a project is closed and thus read-only.

It can be disabled by adding a new condition to the if construct in source:/trunk/app/controllers/repositories_controller.rb@11784#L114, like:

Index: app/controllers/repositories_controller.rb
===================================================================
--- app/controllers/repositories_controller.rb  (revision 11784)
+++ app/controllers/repositories_controller.rb  (working copy)
@@ -111,7 +111,7 @@
   end

   def show
-    @repository.fetch_changesets if Setting.autofetch_changesets? && @path.empty?
+    @repository.fetch_changesets if Setting.autofetch_changesets? && @path.empty? && @project.active?

     @entries = @repository.entries(@path, @rev)
     @changeset = @repository.find_changeset_by_name(@rev)

Associated revisions

Revision 11838
Added by Jean-Philippe Lang over 4 years ago

Disable autofetching of repository changesets if projects are closed (#13945).

Patch by Mischa The Evil.

History

#1 Updated by Mischa The Evil over 4 years ago

  • Description updated (diff)

#2 Updated by Toshi MARUYAMA over 4 years ago

I think it is better to skip in Repository#fetch_changesets.

diff --git a/app/models/repository.rb b/app/models/repository.rb
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -327,6 +327,10 @@ class Repository < ActiveRecord::Base
   # Can be called periodically by an external script
   # eg. ruby script/runner "Repository.fetch_changesets" 
   def self.fetch_changesets
+    unless @project.active?
+      logger.warn "#{@project.name} is not active" 
+      return 
+    end
     Project.active.has_module(:repository).all.each do |project|
       project.repositories.each do |repository|
         begin

#3 Updated by Toshi MARUYAMA over 4 years ago

Sorry, Repository#fetch_changesets is class method, so note-2 is wrong.
This is correct code.

diff --git a/app/models/repository.rb b/app/models/repository.rb
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -328,6 +328,10 @@ class Repository < ActiveRecord::Base
   # eg. ruby script/runner "Repository.fetch_changesets" 
   def self.fetch_changesets
     Project.active.has_module(:repository).all.each do |project|
+      unless project.active?
+        logger.warn "#{project.name} is not active" 
+        next
+      end
       project.repositories.each do |repository|
         begin
           repository.fetch_changesets

#4 Updated by Toshi MARUYAMA over 4 years ago

Sorry, skipping is need in both view and Repository#fetch_changesets.

#5 Updated by Fernando Hartmann over 4 years ago

+1

#6 Updated by Mischa The Evil over 4 years ago

  • Description updated (diff)

Toshi MARUYAMA wrote:

Sorry, skipping is need in both view and Repository#fetch_changesets.

I've spent some time reviewing and testing this. I don't think we need to change Repository.fetch_changesets (the class method) at all, only the Repository#show method in the repositories controller needs to be modified.
The reason for this lies in the situation that Repository.fetch_changesets is already scoped to iterate only over projects which are active (as in STATUS_ACTIVE, as opposed to STATUS_CLOSED and STATUS_ARCHIVED) as a result of the use of the active scope (defined in source:/trunk/app/models/project.rb@11784#L89):

Project.active.has_module(:repository).all.each do |project|
 ...
end

I think by the way that the change I proposed in the description can be optimized by changing the order of the conditions (source:/trunk/app/controllers/repositories_controller.rb@11784#L114):

Index: app/controllers/repositories_controller.rb
===================================================================
--- app/controllers/repositories_controller.rb  (revision 11784)
+++ app/controllers/repositories_controller.rb  (working copy)
@@ -111,7 +111,7 @@
   end

   def show
-    @repository.fetch_changesets if Setting.autofetch_changesets? && @path.empty?
+    @repository.fetch_changesets if @project.active? && Setting.autofetch_changesets? && @path.empty?

     @entries = @repository.entries(@path, @rev)
     @changeset = @repository.find_changeset_by_name(@rev)

I've did a quick scan for any affected tests but couldn't find any. I do think that it would be good if this controller behavior is tested before it gets committed.

#7 Updated by Toshi MARUYAMA over 4 years ago

  • Target version set to 2.4.0

#8 Updated by Jean-Philippe Lang over 4 years ago

  • Status changed from New to Closed
  • Resolution set to Fixed

Patch committed with tests in r11838, thanks.

Also available in: Atom PDF