Implement modified_files in PostCheckout, PostMerge, and PostRewrite hook contexts

[lencioni]

I would like to write a post-checkout, post-merge, and post-rewrite hook that outputs a warning if one specific file has any changes in it. Something along the lines of:

module Overcommit::Hook::PostCheckout
  # Checks if the file has changed
  class FileCheck < Base
    def run
      applicable_files.each do |file|
        if file == 'the file I care about'
          return [:warn, 'the file has changed']
        end
      end

      :pass
    end
  end
end

In my case, I specifically want to print out something like "Run npm install" whenever the npm-shrinkwrap.json file changes (or possibly even just have overcommit run npm install itself).

Since these hook contexts don't implement modified_files, the applicable_files array is blank and I can't run my check. It looks like git diff-tree -r --name-only --no-commit-id ORIG_HEAD HEAD might give you the list of files, so I'm hoping that this won't be too difficult to implement.

[jawshooah]

Not related to implementing modified_files, but couldn't you skip the if file == 'foo' check by just specifying that file in your includes config setting?

[lencioni]

Yes, but I believe that also depends on modified_files working.
On Jul 9, 2015 5:25 PM, "Josh Hagins" notifications@github.com wrote:

Not related to implementing modified_files, but couldn't you skip the if
file == 'foo' check by just specifying that file in your includes config
setting?

—
Reply to this email directly or view it on GitHub
#245 (comment).

[jawshooah]

Yup, my comment was tangential.

Regarding implementation, we need to define exactly what it would mean for a file to be modified in this context. Files that are new/different from the last checked-out commit? Or from the newly checked-out commit's parent(s)?

My guess is the former, but we should be explicit in our specification before diving into the code.

[jawshooah]

Guess I spoke too soon and took the dive anyway 😄

PostCheckout now has a modified_files helper, and PostMerge had one already. PostRewrite is all that's left, and I think the new rewritten_commits helper may be useful there.

[jawshooah]

So PostRewrite is a little tricky; this is where we need to be more specific about what we want modified_files to mean.

In the case of an amendment, should modified_files only include files that were changed between the old and new versions of the commit? Or should it include changes from the old commit as well?

Rebase is even more unclear, as any number of commits could be rewritten. Should we include all files modified in each rewrite?

[sds]

In the case of an amendment, should modified_files only include files that were changed between the old and new versions of the commit? Or should it include changes from the old commit as well?

My intuition would be that if you are implementing a post-rewrite hook that you are more interested in what was specifically rewritten (so your first suggestion). pre-commit hooks already provide you with a way to inspect modified files during an amendment (though admittedly at a fundamentally different point in the process—pre- instead of post-).

Rebase is even more unclear, as any number of commits could be rewritten. Should we include all files modified in each rewrite?

For the use cases I see post-rewrite hooks being written for, I think this makes sense.

---

In either case I think we can ultimately use the same code, since we'll just be iterating through the list of rewritten commits, collecting the files that were modified, and then aggregating those into a single result (though we'll have to be careful with renames/deletions, since modifications in earlier rewrites could be nullified by a rename/deletion in later rewrites).

[sds]

Implemented in #246 and #249.

[lencioni]

Thanks for the hard work on this @jawshooah! 💃