Update Pre-Push Context Modified Files

[taufek]

Previously, Overcommit::HookContext::PrePush::modified_files is set as [] which
does not give us any value in applicable_files as in PreCommit context.

This change updates the method to return list of added and updated files in current branch:

1. compared between current branch and tracking branch if tracking branch is available.

2. compared between current branch with parent branch if tracking branch is not available.

This gives us above values in applicable_files in PrePush context.

One usage example is we can create a hook to scan prohibited keywords such as eval, console.log, etc. before pushing to remote. Previously we can only do it in PreCommit hook but now we could also do it in PrePush.

I think this will also allow us to reuse existing hooks for PreCommit in PrePush context that relies on applicable_files method.

[sds]

Hey @taufek, thanks for the pull request. This would be a great feature to add.

My concern here is that you are not determining the modified refs from the list passed to the pre-push hook, but are inferring them from the current state of the local repository.

I imagine this works correctly in most cases, but I'm concerned that there are issues with this implementation which are not obvious because we're obtaining the refs ourselves rather than using the refs Git passes to the pre-push hook. For example, you can run git push origin HEAD:refs/heads/name-of-your-branch from a detached head (perhaps odd, but definitely possible).

I can appreciate this implementation makes it easier to test, but I think we need to be rigorous in our solution here and utilize the refs Git passes to the hook.

[sds]

On a local repo of mine with a lot of remote branches, this can display warnings like:

warning: ignoring origin/some-remote-branch; cannot handle more than 25 refs

Would need to filter out stderr to avoid.

[taufek]

Nice catch. Will update accordingly.

[taufek]

Thank you @sds for the feedback. It does make sense. I will dig further and incorporate your suggestion here.

[taufek]

@sds ,

I've updated based on your feedback. What do you think?

[sds]

Thanks for making those changes, @taufek. We're getting close here.

[sds]

Similar to why we use the touch helper above, you should use the echo helper here. It saves us having to spawn a child process—which is slower than appending to the file directly in Ruby. A small optimization in this localized example, but when applied across the entire repository it has significant benefits.

[taufek]

TIL. Will update accordingly.

[sds]

This seems unnecessarily expensive (listing all the refs and then filtering for the one of interest). Any reason why

git rev-parse my-ref

...wouldn't suffice here?

[taufek]

Definitely will try this out.

[sds]

Another danger here is you aren't anchoring your regex with ^ / $, so a ref of foo will match foo, foobar, somefoobar, etc. Fortunately we should be able to avoid entirely with git rev-parse ....

[taufek]

👍🏻

[sds]

If we intend to have with_remote_repo be a generic spec helper, it should create the most generic possible setup, and thus should not create any files/commits/objects.

[sds]

We should not hard code some-repo here (that assumes we know how the repo helper is implemented), but obtain it from the name of the repo directory that was created.

[taufek]

My thought was to create a repo with a remote with some default files. Your comment does make sense. It could easily misinterpreted and it is not generic enough for re-use.

[sds]

Not to be overly pedantic, but when I read with_remote_repo I would expect that the block I pass would execute within the context of the remote repository, but I'll admit it's a bit hard to describe what we're trying to do concisely.

[sds]

Looking at the issues with this implementation of with_remote_repo, I'm not entirely convinced this helper is going to get you much. We should probably stick with first principles in the specs:

remote_repo = repo do
  # Create remote repository to our liking
  ...
end

local_repo = repo do
  `git remote add origin file://#{remote_repo}`
  `git fetch origin && git reset --hard origin/master`

  # Create local repository to our liking
  ...
  # Validate expectations
end

[taufek]

This will work. Thanks for the tip

[sds]

FileUtils.rm (again to avoid a child process).

[taufek]

👍🏻

[sds]

The use of some-repo here is cheating a bit as it assumes we know the name of the directory of the repository, which we don't (it is returned by the repo helper).

We can work around this by creating the remote repo in the let so we have the reference.

let(:remote_repo) do
  repo do
    ...
  end
end

let(:remote_url) { "file://#{remote_repo}" }

You can then enter the repo in the specs to perform additional custom setup work:

it ... do
  Dir.chdir remote_repo do
    ...
  end
end

[taufek]

Looks good. Will try this out.

[taufek]

@sds,

This is ready for re-review.

[sds]

You're amazing, @taufek. Thanks for writing such thorough tests!

[taufek]

Thanks @sds. Can't do it without you. I've learned so much.