Allow using various git diff options for comparisons

sk- / git-lint

improving source code one step at a time

Apache License 2.0

236 stars 78 forks source link

Allow using various git diff options for comparisons #126

Closed timmartin19 closed 5 years ago

timmartin19 commented 6 years ago

Instead of having to do funky stuff with soft git resets, this allows the user to pass a --diff option. This option can be anything that works with both git diff-tree and git rev-list. This option does not work with mercurial yet. This is particularly helpful in CI pipelines that rely on PRs. Effectively, you run git lint --diff 'HEAD ^<target-branch>'

The change basically works by looking at all the commits that are different instead of just the --last-commit. It uses the git rev-list command to determine which commits are different. It then uses the same modified_lines function with a slight modification. modified lines now uses a regex OR condition for all of the commits instead of just one commit.

timmartin19 commented 6 years ago

Addresses #60 and #110

sk- commented 6 years ago

Thanks @timmartin19 for the PR, I like the idea, but would like to clarify and simplify some things.

With this option --last-commit could be simplified to mean --diff=HEAD^..HEAD.
We should somehow restrict the diff to be only made against HEAD, because otherwise the results are going to be (mostly) meaningless. If you need to diff a range that's not ending in HEAD you actually need to reset the repo so that the results are accurate.
Will this work in a CI environment, I know that the repo is in a weird state in which the HEAD is dettached.
We would need to have an integration test to validate that the listing of the files is working as intended.

timmartin19 commented 6 years ago

@sk- I think you're absolutely right about always comparing against HEAD. What if it was something like this git lint --diff <commit|branch> and we always compare that commit/branch against the current HEAD? For example, this PR might use git lint --diff master which would translate to git diff-tree master..HEAD. In this case, git lint --diff HEAD^ would be equivalent to git lint --last-commit

I don't think a detached HEAD will cause any problems since we aren't making any changes nor committing. That being said, I'll play with it in a branch in my fork on travis and play with it in Jenkins.

timmartin19 commented 6 years ago

I checked whether it worked with travis. Do to their use of --depth=50 when cloning, you need to do one of two things:

Tell travis not to use depth when cloning. In your .travis.yml

git: depth: false
Explicitly overwrite the configuration before running git lint

git config --replace-all remote.origin.fetch +refs/heads/:refs/remotes/origin/

timmartin19 commented 6 years ago

@sk- Is there anything else you need for this PR?

sk- commented 6 years ago

@timmartin19 Hi Tim, the fact that you still need to wrangle with the git settings in order to be able to run it in Travis makes me doubt about the general usefulness of the PR, as the motivating case was exactly to support CI (hence the mention of git reset).

timmartin19 commented 6 years ago

@sk- That's a quirk of travis as far as I can tell. We're running it in Jenkins now with no special adjustments and it's working just fine. Also, this does not break (like the current CI mechanism) when you force push. The soft reset mechanism being used in Travis is not really workable across different CI tools.