Do not run stash operations when not necessary

[deanrad]

Issues #135 and #504 indicate that the stashing process, while convenient for a certain use case, introduces issues and risk of loss of files, under edge cases.

Editor flicker (VSCode on Mac, at least), the possibility that an abort (via ^C) leaves the working tree worse than before - these are issues that users need not be exposed to if they have a workflow where the hooks can and should be run on the entire working tree, because they have no unstaged changes while committing.

We discussed the desirability of this on #135 and I'm opening this issue to attach a PR to. Will need some help! But I think there will be a benefit to users who enjoy Overcommit, but who don't regularly make commits with only some of the changes staged..

I can add that my workflow, involves using stashes for the sake of applying console.log and other non-commit-worthy diffs, and manual reversion. I don't try to write the commit message and run hooks and commit until my tools very clearly show me what I'm committing and not. So I've never been in need of stashing. But perhaps by excluding use cases like mine from stashing, Overcommit can provide the best of both worlds.

[deanrad]

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!

[sds]

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?

[deanrad]

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.