Support apps in non-root locations (#40)

* Allow passing in a filename transformer

* attempt to pull out git wrapper

* Easy PR comments (rename, types, comments)

* comment

* add some tests

* Fix sorbet, increment version

* Fix the last cop

Ended up pulling some functions private to get around the max 100
lines cop.

Author: jarredhawkins

Gemfile.lock

@@ -17,7 +17,7 @@ GIT
 PATH
   remote: .
   specs:
-    danger-packwerk (0.14.1)
+    danger-packwerk (0.14.2)
       code_ownership
       danger-plugin-api (~> 1.0)
       packwerk

lib/danger-packwerk/danger_package_todo_yml_changes.rb

@@ -29,22 +29,25 @@ class DangerPackageTodoYmlChanges < Danger::Plugin
         offenses_formatter: T.nilable(Update::OffensesFormatter),
         before_comment: BeforeComment,
         max_comments: Integer,
-        violation_types: T::Array[String]
+        violation_types: T::Array[String],
+        root_path: T.nilable(String)
       ).void
     end
     def check(
       offenses_formatter: nil,
       before_comment: DEFAULT_BEFORE_COMMENT,
       max_comments: DEFAULT_MAX_COMMENTS,
-      violation_types: DEFAULT_VIOLATION_TYPES
+      violation_types: DEFAULT_VIOLATION_TYPES,
+      root_path: nil
     )
       offenses_formatter ||= Update::DefaultFormatter.new
       repo_link = github.pr_json[:base][:repo][:html_url]
       org_name = github.pr_json[:base][:repo][:owner][:login]
 
-      changed_package_todo_ymls = (git.modified_files + git.added_files + git.deleted_files).grep(PACKAGE_TODO_PATTERN)
+      git_filesystem = Private::GitFilesystem.new(git: git, root: root_path || '')
+      changed_package_todo_ymls = (git_filesystem.modified_files + git_filesystem.added_files + git_filesystem.deleted_files).grep(PACKAGE_TODO_PATTERN)
 
-      violation_diff = get_violation_diff(violation_types)
+      violation_diff = get_violation_diff(violation_types, root_path: root_path)
 
       before_comment.call(
         violation_diff,
@@ -61,130 +64,30 @@ def check(
         markdown(
           offenses_formatter.format_offenses(violations, repo_link, org_name),
           line: location.line_number,
-          file: location.file
+          file: git_filesystem.convert_to_filesystem(location.file)
         )
 
         current_comment_count += 1
       end
     end
 
-    sig { params(violation_types: T::Array[String]).returns(ViolationDiff) }
-    def get_violation_diff(violation_types)
-      added_violations = T.let([], T::Array[BasicReferenceOffense])
-      removed_violations = T.let([], T::Array[BasicReferenceOffense])
-
-      git.added_files.grep(PACKAGE_TODO_PATTERN).each do |added_package_todo_yml_file|
-        # Since the file is added, we know on the base commit there are no violations related to this pack,
-        # and that all violations from this file are new
-        added_violations += BasicReferenceOffense.from(added_package_todo_yml_file)
-      end
-
-      git.deleted_files.grep(PACKAGE_TODO_PATTERN).each do |deleted_package_todo_yml_file|
-        # Since the file is deleted, we know on the HEAD commit there are no violations related to this pack,
-        # and that all violations from this file are deleted
-        deleted_violations = get_violations_before_patch_for(deleted_package_todo_yml_file)
-        removed_violations += deleted_violations
-      end
-
-      # The format for git.renamed_files is a T::Array[{after: "some/path/new", before: "some/path/old"}]
-      renamed_files_before = git.renamed_files.map { |before_after_file| before_after_file[:before] }
-      renamed_files_after = git.renamed_files.map { |before_after_file| before_after_file[:after] }
-
-      git.modified_files.grep(PACKAGE_TODO_PATTERN).each do |modified_package_todo_yml_file|
-        # We skip over modified files if one of the modified files is a renamed `package_todo.yml` file.
-        # This allows us to rename packs while ignoring "new violations" in those renamed packs.
-        next if renamed_files_before.include?(modified_package_todo_yml_file)
-
-        head_commit_violations = BasicReferenceOffense.from(modified_package_todo_yml_file)
-        base_commit_violations = get_violations_before_patch_for(modified_package_todo_yml_file)
-        added_violations += head_commit_violations - base_commit_violations
-        removed_violations += base_commit_violations - head_commit_violations
-      end
-
-      #
-      # This implementation creates some false negatives:
-      # That is – it doesn't capture some cases:
-      # 1) A file has been renamed without renaming a constant.
-      # That can happen if we change only the autoloaded portion of a filename.
-      # For example: `packs/foo/app/services/my_class.rb` (defines: `MyClass`)
-      # is changed to `packs/foo/app/public/my_class.rb` (still defines: `MyClass`)
-      #
-      # This implementation also doesn't cover these false positives:
-      # That is – it leaves a comment when it should not.
-      # 1) A CONSTANT within a class or module has been renamed.
-      # e.g. `class MyClass; MY_CONSTANT = 1; end` becomes `class MyClass; RENAMED_CONSTANT = 1; end`
-      # We would not detect based on file renames that `MY_CONSTANT` has been renamed.
-      #
-      renamed_constants = []
-
-      added_violations.each do |violation|
-        filepath_that_defines_this_constant = Private.constant_resolver.resolve(violation.class_name)&.location
-        renamed_constants << violation.class_name if renamed_files_after.include?(filepath_that_defines_this_constant)
-      end
-
-      relevant_added_violations = added_violations.reject do |violation|
-        renamed_files_after.include?(violation.file) ||
-          renamed_constants.include?(violation.class_name) ||
-          !violation_types.include?(violation.type)
-      end
+    sig do
+      params(
+        violation_types: T::Array[String],
+        root_path: T.nilable(String)
+      ).returns(ViolationDiff)
+    end
+    def get_violation_diff(violation_types, root_path: nil)
+      git_filesystem = Private::GitFilesystem.new(git: git, root: root_path || '')
 
-      relevant_removed_violations = removed_violations.select do |violation|
-        violation_types.include?(violation.type)
-      end
+      added_violations, removed_violations = Private::TodoYmlChanges.get_reference_offenses(
+        violation_types, git_filesystem
+      )
 
       ViolationDiff.new(
-        added_violations: relevant_added_violations,
-        removed_violations: relevant_removed_violations
+        added_violations: added_violations,
+        removed_violations: removed_violations
       )
     end
-
-    private
-
-    sig { params(package_todo_yml_file: String).returns(T::Array[BasicReferenceOffense]) }
-    def get_violations_before_patch_for(package_todo_yml_file)
-      # The strategy to get the violations before this PR is to reverse the patch on each `package_todo.yml`.
-      # A previous strategy attempted to use `git merge-base --fork-point`, but there are many situations where it returns
-      # empty values. That strategy is fickle because it depends on the state of the `reflog` within the CI suite, which appears
-      # to not be reliable to depend on.
-      #
-      # Instead, just inverting the patch should hopefully provide a more reliable way to figure out what was the state of the file before
-      # the PR without needing to use git commands that interpret the branch history based on local git history.
-      #
-      # We apply the patch to the original file so that we can seamlessly reverse the patch applied to that file (since patches are coupled to
-      # the files they modify). After parsing the violations from that `package_todo.yml` file with the patch reversed,
-      # we use a temporary copy of the original file to rewrite to it with the original contents.
-      # Note that practically speaking, we don't need to rewrite the original contents (since we already fetched the
-      # original contents above and the CI file system should be ephemeral). However, we do this anyways in case we later change these
-      # assumptions, or another client's environment is different and expects these files not to be mutated.
-
-      # Keep track of the original file contents. If the original file has been deleted, then we delete the file after inverting the patch at the end, rather than rewriting it.
-      package_todo_yml_file_copy = (File.read(package_todo_yml_file) if File.exist?(package_todo_yml_file))
-
-      Tempfile.create do |patch_file|
-        # Normally we'd use `git.diff_for_file(package_todo_yml_file).patch` here, but there is a bug where it does not work for deleted files yet.
-        # I have a fix for that here: https://github.com/danger/danger/pull/1357
-        # Until that lands, I'm just using the underlying implementation of that method to get the diff for a file.
-        # Note that I might want to use a safe escape operator, `&.patch` and return gracefully if the patch cannot be found.
-        # However I'd be interested in why that ever happens, so for now going to proceed as is.
-        # (Note that better yet we'd have observability into these so I can just log under those circumstances rather than surfacing an error to the user,
-        # but we don't have that quite yet.)
-        patch_for_file = git.diff[package_todo_yml_file].patch
-        # This appears to be a known issue that patches require new lines at the end. It seems like this is an issue with Danger that
-        # it gives us a patch without a newline.
-        # https://stackoverflow.com/questions/18142870/git-error-fatal-corrupt-patch-at-line-36
-        patch_file << "#{patch_for_file}\n"
-        patch_file.rewind
-        # https://git-scm.com/docs/git-apply
-        _stdout, _stderr, _status = Open3.capture3("git apply --reverse #{patch_file.path}")
-        # https://www.rubyguides.com/2019/05/ruby-tempfile/
-        BasicReferenceOffense.from(package_todo_yml_file)
-      end
-    ensure
-      if package_todo_yml_file_copy
-        File.write(package_todo_yml_file, package_todo_yml_file_copy)
-      else
-        File.delete(package_todo_yml_file)
-      end
-    end
   end
 end

lib/danger-packwerk/danger_packwerk.rb

@@ -6,6 +6,7 @@
 require 'parse_packwerk'
 require 'sorbet-runtime'
 require 'danger-packwerk/packwerk_wrapper'
+require 'danger-packwerk/private/git'
 
 module DangerPackwerk
   # Note that Danger names the plugin (i.e. anything that inherits from `Danger::Plugin`) by taking the name of the class and gsubbing out "Danger"
@@ -43,7 +44,8 @@ class CommentGroupingStrategy < ::T::Enum
         failure_message: String,
         on_failure: OnFailure,
         violation_types: T::Array[String],
-        grouping_strategy: CommentGroupingStrategy
+        grouping_strategy: CommentGroupingStrategy,
+        root_path: T.nilable(String)
       ).void
     end
     def check(
@@ -53,7 +55,8 @@ def check(
       failure_message: DEFAULT_FAILURE_MESSAGE,
       on_failure: DEFAULT_ON_FAILURE,
       violation_types: DEFAULT_VIOLATION_TYPES,
-      grouping_strategy: CommentGroupingStrategy::PerConstantPerLocation
+      grouping_strategy: CommentGroupingStrategy::PerConstantPerLocation,
+      root_path: nil
     )
       offenses_formatter ||= Check::DefaultFormatter.new
       repo_link = github.pr_json[:base][:repo][:html_url]
@@ -67,9 +70,12 @@ def check(
       # trigger the warning message, which is good, since we only want to trigger on new code.
       github.dismiss_out_of_range_messages
 
+      git_filesystem = Private::GitFilesystem.new(git: git, root: root_path || '')
+
       # https://github.com/danger/danger/blob/eca19719d3e585fe1cc46bc5377f9aa955ebf609/lib/danger/danger_core/plugins/dangerfile_git_plugin.rb#L80
-      renamed_files_after = git.renamed_files.map { |f| f[:after] }
-      targeted_files = (git.modified_files + git.added_files + renamed_files_after).select do |f|
+      renamed_files_after = git_filesystem.renamed_files.map { |f| f[:after] }
+
+      targeted_files = (git_filesystem.modified_files + git_filesystem.added_files + renamed_files_after).select do |f|
         path = Pathname.new(f)
 
         # We probably want to check the `include` key of `packwerk.yml`. By default, this value is "**/*.{rb,rake,erb}",
@@ -92,7 +98,7 @@ def check(
 
       packwerk_reference_offenses = PackwerkWrapper.get_offenses_for_files(targeted_files.to_a).compact
 
-      renamed_files = git.renamed_files.map { |before_after_file| before_after_file[:after] }
+      renamed_files = git_filesystem.renamed_files.map { |before_after_file| before_after_file[:after] }
 
       packwerk_reference_offenses_to_care_about = packwerk_reference_offenses.reject do |packwerk_reference_offense|
         constant_name = packwerk_reference_offense.reference.constant.name
@@ -131,8 +137,7 @@ def check(
         referencing_file = reference_offense.reference.relative_path
 
         message = offenses_formatter.format_offenses(unique_packwerk_reference_offenses, repo_link, org_name)
-
-        markdown(message, file: referencing_file, line: line_number)
+        markdown(message, file: git_filesystem.convert_to_filesystem(referencing_file), line: line_number)
       end
 
       if current_comment_count > 0

lib/danger-packwerk/private.rb

@@ -1,6 +1,7 @@
 # typed: strict
 
 require 'danger-packwerk/private/ownership_information'
+require 'danger-packwerk/private/todo_yml_changes'
 require 'constant_resolver'
 
 module DangerPackwerk

lib/danger-packwerk/private/git.rb

@@ -0,0 +1,65 @@
+# typed: strict
+
+require 'code_ownership'
+require 'packs'
+
+# In order to support running danger-packwerk from a non-root filepath, we need
+# to wrap some git functions in filesystem wrappers: packwerk runs relative to
+# the rails app root, whereas git returns paths on the actual filesystem.
+module DangerPackwerk
+  module Private
+    class GitFilesystem < T::Struct
+      extend T::Sig
+
+      const :git, Danger::DangerfileGitPlugin
+      const :root, String
+
+      sig { returns(T::Array[{ after: String, before: String }]) }
+      def renamed_files
+        @git.renamed_files.map do |f|
+          {
+            after: convert_file_from_filesystem(f[:after]),
+            before: convert_file_from_filesystem(f[:before])
+          }
+        end
+      end
+
+      sig { returns(T::Array[String]) }
+      def modified_files
+        convert_from_filesystem(@git.modified_files.to_a)
+      end
+
+      sig { returns(T::Array[String]) }
+      def deleted_files
+        convert_from_filesystem(@git.deleted_files.to_a)
+      end
+
+      sig { returns(T::Array[String]) }
+      def added_files
+        convert_from_filesystem(@git.added_files.to_a)
+      end
+
+      sig { params(filename_on_disk: String).returns(::Git::Diff::DiffFile) }
+      def diff(filename_on_disk)
+        @git.diff[filename_on_disk]
+      end
+
+      sig { params(path: String).returns(String) }
+      def convert_to_filesystem(path)
+        Pathname(@root).join(path).to_s
+      end
+
+      private
+
+      sig { params(files: T::Array[String]).returns(T::Array[String]) }
+      def convert_from_filesystem(files)
+        files.map { |f| convert_file_from_filesystem(f) }
+      end
+
+      sig { params(file: String).returns(String) }
+      def convert_file_from_filesystem(file)
+        Pathname(file).relative_path_from(@root).to_s
+      end
+    end
+  end
+end