Do not run stash operations when not necessary #669

Activity

1. deanrad commented on Sep 11, 2019

   

   deanrad

   on Sep 11, 2019

   Last edited by deanrad

   Author

   More actions

   So let me know how this sounds.. Because OC is good at restoring contents (and modification times) of files, the way to know whether stashing/restoring takes place could be the reflog.

   Starting with this reflog (with most recent commit on top)

   413aad983c HEAD@{0}: commit: test:: octest
   16cd46c551 HEAD@{1}: reset: moving to HEAD
   

   A commit that bypasses commit hooks and stashing goes directly on top:

   e4745fb9c2 HEAD@{0}: commit: test:: octest, no stash
   413aad983c HEAD@{1}: commit: test:: octest
   16cd46c551 HEAD@{1}: reset: moving to HEAD
   

   Whereas one that uses stashing adds more reflog entries:

   027ec917d0 HEAD@{0}: commit: test:: octest, w/ stash
   e4745fb9c2 HEAD@{1}: reset: moving to HEAD
   e4745fb9c2 HEAD@{2}: reset: moving to HEAD
   e4745fb9c2 HEAD@{3}: reset: moving to HEAD
   e4745fb9c2 HEAD@{4}: commit: test:: octest, no stash
   413aad983c HEAD@{5}: commit: test:: octest
   

   So is that the best way to see whether stashing occurred, or was bypassed? I've started the tests, I'm looking for ways to fill them out. Many thanks in advance!
2. sds commented on Sep 13, 2019

   

   sds

   on Sep 13, 2019

   Owner

   More actions

   Hey @deanius, thanks for putting together a proposal.

   If there isn't a clear way to determine whether a stash had occurred, then perhaps we should simply structure the code to check if a given method was called.

   For example, extract:

   overcommit/lib/overcommit/hook_context/pre_commit.rb

   Lines 53 to 68 in 5a6ca89

   	@stash_attempted = true
   	stash_message = "Overcommit: Stash of repo state before hook run at #{Time.now}"
   	result = Overcommit::Utils.execute(
   	%w[git -c commit.gpgsign=false stash save --keep-index --quiet] + [stash_message]
   	)
   	unless result.success?
   	# Failure to stash in this case is likely due to a configuration
   	# issue (e.g. author/email not set or GPG signing key incorrect)
   	raise Overcommit::Exceptions::HookSetupFailed,
   	"Unable to setup environment for #{hook_script_name} hook run:" \
   	"\nSTDOUT:#{result.stdout}\nSTDERR:#{result.stderr}"
   	end
   	@changes_stashed = `git stash list -1`.include?(stash_message)

   into a helper method stash_changes, and in our tests validate that the helper method was never called in the case we are trying to validate.

       context 'when all changes have been staged' do
         around do |example|
           repo do
             echo('Hello World', 'tracked-file')
             echo('Hello Other World', 'other-tracked-file')
             `git add tracked-file other-tracked-file`
             example.run
           end
         end
   
         it 'does not stash changes' do
           expect(context).to_not receive(:stash_changes)
           subject
         end

   ...simliarly for cleanup_environment tests:

         it 'does not restore the stash' do
           expect(context).to_not receive(:restore_stash_changes)
           subject
         end
       end

   Does that seem reasonable?

   👀React with 👀1Reacted by Dean Radcliffe
3. sds

   added

   enhancement

   on Sep 20, 2019
4. deanrad commented on Sep 27, 2019

   

   deanrad

   on Sep 27, 2019

   Author

   More actions

   I'm definitely in agreement, but not expert with Ruby mocking and event the first assertion here doesn't pass..

         it 'does not stash changes' do
           expect(context).to receive(:setup_environment)
           expect(context).to receive(:stash_changes)
         end

   I hope this doesn't mean I'm at the end of my line trying to help, but I'm not sure I can.
5. sds

   added 2 commits that reference this issue on Sep 29, 2019

   Avoid stashing changes when all changes are staged

   ...

   70ff629

   Avoid stashing changes when all changes are staged

   ...

   b3f36ba
6. sds

   closed this as completedin #674on Sep 29, 2019
7. sds

   added a commit that references this issue on Sep 29, 2019

   Avoid stashing changes when all changes are staged

   ...

   9f82bfb
8. sds

   added a commit that references this issue on Nov 11, 2026

   Support hook side effects without stashing repository changes (#939)

   ...

   a5bdc87