Save/restore unstaged changes before running hooks

Add support for "hiding" unstaged changes to files before running the
hooks against them. This prevents the situation where unstaged changes
would cause hooks to fail, but they shouldn't because they are
unchanged.

Previously, Overcommit accomplished this by copying staged changes to
temporary files and then running checks against those files. The problem
with this approach is that it breaks the functionality of tools which
perform certain checks based on filename/path. Since the temporary file
will not share the original path, the tool doesn't work as expected.

By stashing changes to the file instead, the path/filename remains the
same, but we still get the benefit of hiding the unstaged changes.

Change-Id: Id1d39d47be4be7348c4126a6d6793506605e8149
Reviewed-on: http://gerrit.causes.com/35603
Reviewed-by: Shane da Silva <shane@causes.com>
Tested-by: Shane da Silva <shane@causes.com>

Author: sds

lib/overcommit/hook_context/base.rb

@@ -25,6 +25,23 @@ def hook_type_name
       Overcommit::Utils.snake_case(hook_class_name)
     end
 
+    # Initializes anything related to the environment.
+    #
+    # This is called before the hooks are run by the [HookRunner]. Different
+    # hook types can perform different setup.
+    def setup_environment
+      # Implemented by subclass
+    end
+
+    # Resets the environment to an appropriate state.
+    #
+    # This is called after the hooks have been run by the [HookRunner].
+    # Different hook types can perform different cleanup operations, which are
+    # intended to "undo" the results of the call to {#setup_environment}.
+    def cleanup_environment
+      # Implemented by subclass
+    end
+
     # Returns a list of files that have been modified.
     #
     # By default, this returns an empty list. Subclasses should implement if

lib/overcommit/hook_context/pre_commit.rb

@@ -4,6 +4,30 @@ module Overcommit::HookContext
   # This includes staged files, which lines of those files have been modified,
   # etc.
   class PreCommit < Base
+    # Stash unstaged contents of files so hooks don't see changes that aren't
+    # about to be committed.
+    def setup_environment
+      store_modified_times
+
+      if modified_files.any?
+        `git stash save --keep-index --quiet #{<<-MSG}`
+          "Overcommit: Stash of repo state before hook run at #{Time.now}"
+        MSG
+      end
+
+      # While running the hooks make it appear as if nothing changed
+      restore_modified_times
+    end
+
+    # Restore unstaged changes and reset file modification times so it appears
+    # as if nothing ever changed.
+    def cleanup_environment
+      `git reset --hard`
+      `git stash pop --index --quiet` if modified_files.any?
+
+      restore_modified_times
+    end
+
     # Get a list of added, copied, or modified files that have been staged.
     # Renames and deletions are ignored, since there should be nothing to check.
     def modified_files
@@ -46,5 +70,25 @@ def extract_modified_lines(staged_file)
 
       lines
     end
+
+    def store_modified_times
+      @modified_times = {}
+
+      modified_files.each do |file|
+        @modified_times[file] = File.mtime(file)
+      end
+    end
+
+    # Stores the modification times for all modified files to make it appear like
+    # they never changed.
+    #
+    # This prevents editors from complaining about files changing when we stash
+    # changes before running the hooks.
+    def restore_modified_times
+      modified_files.each do |file|
+        time = @modified_times[file]
+        File.utime(time, time, file)
+      end
+    end
   end
 end

lib/overcommit/hook_runner.rb

@@ -2,21 +2,20 @@ module Overcommit
   # Responsible for loading the hooks the repository has configured and running
   # them, collecting and displaying the results.
   class HookRunner
-    def initialize(config, logger)
+    def initialize(config, logger, context)
       @config = config
       @logger = logger
+      @context = context
       @hooks = []
     end
 
     # Loads and runs the hooks registered for this {HookRunner}.
-    def run(context)
-      @context = context
-
-      # stash_unstaged_files
+    def run
       load_hooks
+      @context.setup_environment
       run_hooks
     ensure
-      # restore_unstaged_files
+      @context.cleanup_environment
     end
 
   private
@@ -82,24 +81,5 @@ def create_hook(hook_name)
             "Unable to load hook '#{hook_name}': #{error}",
             error.backtrace
     end
-
-    # Stashes untracked files and unstaged changes so that those changes aren't
-    # read by the hooks.
-    def stash_unstaged_files
-      # TODO: store mtime of all stashed files with File.new('blah').mtime
-      `git stash save --keep-index --include-untracked --quiet #{<<-MSG}`
-        "Stash of repo state before hook run - #{Time.now}"
-      MSG
-    end
-
-    # Restores the stashed files after hook has run.
-    #
-    # Assumes that any hooks that might have manipulated the stash have properly
-    # left the stash in its original state.
-    def restore_unstaged_files
-      # TODO: Restore mtime of all stashed files with
-      # FileUtils.touch('blah'), :mtime => time
-      `git stash pop --quiet`
-    end
   end
 end

spec/overcommit/hook_context/pre_commit_spec.rb

@@ -7,6 +7,133 @@
   let(:input) { '' }
   let(:context) { described_class.new(config, args, input) }
 
+  describe '#setup_environment' do
+    subject { context.setup_environment }
+
+    context 'when there are no staged changes' do
+      around do |example|
+        repo do
+          `echo "Hello World" > tracked-file`
+          `git add tracked-file`
+          `git commit -m "Add tracked-file"`
+          `echo "Hello Again" > untracked-file`
+          example.run
+        end
+      end
+
+      it 'keeps already-committed files' do
+        subject
+        File.open('tracked-file', 'r').read == 'Hello World'
+      end
+
+      it 'keeps untracked files' do
+        subject
+        File.open('untracked-file', 'r').read == 'Hello Again'
+      end
+
+      it 'keeps modification times the same' do
+        expect { subject }.
+          to_not change { [File.mtime('tracked-file'), File.mtime('untracked-file')] }
+      end
+    end
+
+    context 'when there are staged changes' do
+      around do |example|
+        repo do
+          `echo "Hello World" > tracked-file`
+          `git add tracked-file`
+          `git commit -m "Add tracked-file"`
+          `echo "Hello Again" > untracked-file`
+          `echo "Some more text" >> tracked-file`
+          `git add tracked-file`
+          `echo "Yet some more text" >> tracked-file`
+          example.run
+        end
+      end
+
+      it 'keeps staged changes' do
+        subject
+        File.open('tracked-file', 'r').read == 'Hello WorldSome more text'
+      end
+
+      it 'keeps untracked files' do
+        subject
+        File.open('untracked-file', 'r').read == 'Hello Again'
+      end
+
+      it 'keeps modification times the same' do
+        expect { subject }.
+          to_not change { [File.mtime('tracked-file'), File.mtime('untracked-file')] }
+      end
+    end
+  end
+
+  describe '#cleanup_environment' do
+    subject { context.cleanup_environment }
+
+    before do
+      context.setup_environment
+    end
+
+    context 'when there were no staged changes' do
+      around do |example|
+        repo do
+          `echo "Hello World" > tracked-file`
+          `git add tracked-file`
+          `git commit -m "Add tracked-file"`
+          `echo "Hello Again" > untracked-file`
+          example.run
+        end
+      end
+
+      it 'keeps already-committed files' do
+        subject
+        File.open('tracked-file', 'r').read.should == "Hello World\n"
+      end
+
+      it 'keeps untracked files' do
+        subject
+        File.open('untracked-file', 'r').read.should == "Hello Again\n"
+      end
+
+      it 'keeps modification times the same' do
+        expect { subject }.
+          to_not change { [File.mtime('tracked-file'), File.mtime('untracked-file')] }
+      end
+    end
+
+    context 'when there were staged changes' do
+      around do |example|
+        repo do
+          `echo "Hello World" > tracked-file`
+          `git add tracked-file`
+          `git commit -m "Add tracked-file"`
+          `echo "Hello Again" > untracked-file`
+          `echo "Some more text" >> tracked-file`
+          `git add tracked-file`
+          `echo "Yet some more text" >> tracked-file`
+          example.run
+        end
+      end
+
+      it 'restores the unstaged changes' do
+        subject
+        File.open('tracked-file', 'r').read.
+          should == "Hello World\nSome more text\nYet some more text\n"
+      end
+
+      it 'keeps untracked files' do
+        subject
+        File.open('untracked-file', 'r').read.should == "Hello Again\n"
+      end
+
+      it 'keeps modification times the same' do
+        expect { subject }.
+          to_not change { [File.mtime('tracked-file'), File.mtime('untracked-file')] }
+      end
+    end
+  end
+
   describe '#modified_files' do
     subject { context.modified_files }
 

template-dir/hooks/overcommit-hook

@@ -48,9 +48,9 @@ begin
 
   context = Overcommit::HookContext.create(hook_type_class, config, ARGV, STDIN)
   logger = Overcommit::Logger.new(STDOUT)
-  runner = Overcommit::HookRunner.new(config, logger)
+  runner = Overcommit::HookRunner.new(config, logger, context)
 
-  status = runner.run(context)
+  status = runner.run
 
   exit(status ? 0 : 65) # 65 = EX_DATAERR
 rescue Overcommit::Exceptions::ConfigurationError => error