modified_files and modified_lines_in_file on CommitMsg

[Quintasan]

As I promised – I did try my hand and I employed the dumbest approach that's almost not copy & paste*

Big problem

It's 2 in the morning so I'm probably cognitively impaired enough to not be able to figure out why rspec spec/overcommit/hook_context/commit_msg_spec.rb:636:705:764 are failing.

All those fails have amending in common so I suspect this is due to calling amendment? not from the Overcommit::HookContext::CommitMsg hook context but from the Overcommit::HookContext::PreCommit context which I created using Overcommit::HookContext::PreCommit.new(@config, @args, @input) but I am unable to pinpoint why would it behave differently.

Good points

1. Reuses a big chunk of Overcommit::HookContext::PreCommit so in the future you'd have to modify only that

Bad points

1. store_modified_times is copypasta
2. restore_modified_times is copypasta
3. modified_lines_in_file is quite close to being copypasta
4. Test are copypasta – can be solved by converting into shared examples*

Fixing 1, 2, 3 would require some changes in the PreCommit hook context but I didn't want to throw a huge PR right away without asking first.

@sds Please let me know if this looks reasonable or I need another approach.

Closes #726

[sds]

Thanks for the PR, @Quintasan. Supportive of the general direction.

Looking over this quickly, I don't have an obvious explanation for the failing tests, but my guess would be we'd probably have an easier time extracting the shared logic into a module that we include in both Overcommit::HookContext::CommitMsg and Overcommit::HookContext::PreCommit.

I'm also OK having the tests be duplicated rather than using shared examples. There are often subtle differences between hooks and I don't think DRY tests in this context would be worth it, even if right now we expect the same behavior.

Appreciate you proposing this general direction. If we can extract the environment setup/cleanup logic into a module and get the tests passing that way, I'd be comfortable merging. By extracting into a module, perhaps we could finally (separate from this PR, of course) consider implementing a mechanism that doesn't use the stash (see #238).

[Quintasan]

Great, I hope to have something you can work with in the upcoming week

[Quintasan]

@sds It turns out I couldn't wait this much to work on this.

I wasn't sure where to put the extracted modules so I went with Helpers module but I think SharedBehavior would work better.

Another thing I thought of was to rename setup_environment and cleanup_environment in the new module so we don't stealthily modify behavior just by including the module.

WDYT?

[sds]

Overall looks good. Some minor comments to address and then we'll happily merge. Thanks!

[sds]

Let's have just one module that defines everything rather than splitting up into a separate module for stashing changes and handling file modification time. I don't see the value in separating them as they are both necessary to provide the functionality desired. Let me know if I'm misunderstanding the intent.

[Quintasan]

@sds the intention here was to provide a way to reuse methods related to file modifications without assuming that the workflow will always be 'we must hide unstaged changes for the hook context'.

[sds]

Thanks!

[sds]

@Quintasan I'm seeing a warning once this commit is added. Can you have a look?

~/overcommit/lib/overcommit/hook/pre_commit/execute_permissions.rb:29: warning: Overcommit::Hook::PreCommit::Base#initial_commit? at ~/.rbenv/versions
/2.6.5/lib/ruby/2.6.0/forwardable.rb:158 forwarding to private method Overcommit::HookContext::PreCommit#initial_commit?

[sds]

Thank you for adding this functionality and taking it through to completion, @Quintasan.

Released in 0.56.0.