Allow using various git diff options for comparisons #126

timmartin19 · 2018-06-21T03:45:05Z

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.

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.

It wanted to break the arguments onto multiple line in the usage. This made it less readable IMO so I disabled the linter for that section

Can't satisfy pycodestyle and docopt at the same time without breaking it onto multiple lines. Doesn't read as well in my opinion.

timmartin19 · 2018-06-21T14:55:29Z

Addresses #60 and #110

sk- · 2018-06-21T16:46:04Z

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.

sk- · 2018-06-21T16:47:50Z

gitlint/__init__.py

+        last_commit = vcs.last_commit()
+        commits = [last_commit]
+
+    if arguments['--diff'] and vcs.__name__ != 'gitlint.git':


I think you could do vcs != gitlint.git

sk- · 2018-06-21T16:48:38Z

gitlint/git.py


    # Split as bytes, as the output may have some non unicode characters.
    blame_lines = subprocess.check_output(
        ['git', 'blame', '--porcelain', filename]).split(
            os.linesep.encode('utf-8'))
+    regex_pattern = r'^({}) (?P<line>\d+) (\d+)'.format(
+        '|'.join(commits)).encode('utf-8')


For safety I'd prefer to escape the commits.

sk- · 2018-06-21T16:49:21Z

test/unittest/test_gitlint.py

@@ -17,6 +17,7 @@
 import os
 import sys

+from docopt import DocoptExit


I follow the Google style guide, so we only import modules.

sk- · 2018-06-21T17:11:24Z

gitlint/git.py

+    command = [
+        'git', 'diff-tree', '-r', '--root', '--no-commit-id', '--name-status'
+    ]
+    command.extend(diff_target.split(' '))


In which cases is the argument going to have spaces, could you give some examples.

timmartin19 · 2018-06-21T20:07:17Z

@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.

It gets pretty hard to compare arbitrarily. Instead, it now simply compares a target branch or commit against the current head. This also simplifies `--last-commit` since it's effectively the same as `--diff HEAD^`. This did require some refactoring to keep the code base clean. Also, adding an e2e test for comparing a branch multiple commits ahead of master against master.

I really need to set up the local commit hooks

timmartin19 · 2018-06-24T15:16:50Z

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 · 2018-07-03T16:00:54Z

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

sk- · 2018-07-16T05:02:32Z

@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 · 2018-07-16T13:29:58Z

@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.

timmartin19 added 4 commits June 20, 2018 23:44

Disable pylint line length check for docopt

f56f852

It wanted to break the arguments onto multiple line in the usage. This made it less readable IMO so I disabled the linter for that section

Apparently we need to disable two types for line-too-long

10282a5

Revert to multi-line usage

bab9acf

Can't satisfy pycodestyle and docopt at the same time without breaking it onto multiple lines. Doesn't read as well in my opinion.

sk- suggested changes Jun 21, 2018

View reviewed changes

timmartin19 added 4 commits June 23, 2018 16:44

Fixing internal code styling and broken test cleanup

6baaf08

Fix docopt error

90c9d5b

I really need to set up the local commit hooks

forgot to run yapf

6405631

timmartin19 closed this Oct 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using various git diff options for comparisons #126

Allow using various git diff options for comparisons #126

timmartin19 commented Jun 21, 2018

timmartin19 commented Jun 21, 2018

sk- commented Jun 21, 2018

sk- Jun 21, 2018

sk- Jun 21, 2018

sk- Jun 21, 2018

sk- Jun 21, 2018

timmartin19 commented Jun 21, 2018

timmartin19 commented Jun 24, 2018

timmartin19 commented Jul 3, 2018

sk- commented Jul 16, 2018

timmartin19 commented Jul 16, 2018 •

edited

Loading

Allow using various git diff options for comparisons #126

Allow using various git diff options for comparisons #126

Conversation

timmartin19 commented Jun 21, 2018

timmartin19 commented Jun 21, 2018

sk- commented Jun 21, 2018

sk- Jun 21, 2018

Choose a reason for hiding this comment

sk- Jun 21, 2018

Choose a reason for hiding this comment

sk- Jun 21, 2018

Choose a reason for hiding this comment

sk- Jun 21, 2018

Choose a reason for hiding this comment

timmartin19 commented Jun 21, 2018

timmartin19 commented Jun 24, 2018

timmartin19 commented Jul 3, 2018

sk- commented Jul 16, 2018

timmartin19 commented Jul 16, 2018 • edited Loading

timmartin19 commented Jul 16, 2018 •

edited

Loading