jorisroovers / gitlint

Linting for your git commit messages
http://jorisroovers.github.io/gitlint
MIT License
806 stars 99 forks source link

gitlint raises an exception if run without --staged and with an ignore-by-author-name configuration #403

Closed sagittarian closed 1 year ago

sagittarian commented 1 year ago

If gitlint is run without --staged, then the author name is always None and there is no guard for that situation in the ignore-by-author-name rule. Here is a setup to reproduce it:

$ gitlint --version
gitlint, version 0.18.0
$ cat .git/hooks/commit-msg
#!/bin/bash

gitlint --msg-filename $1
$ cat .gitlint
[ignore-by-author-name]
regex=(.*)dependabot(.*)
$ git commit
hint: Waiting for your editor to close the file... Waiting for Emacs...
WARNING: I4 - ignore-by-author-name: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-by-author-name.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
Traceback (most recent call last):
  File "/home/adam/.local/bin/gitlint", line 8, in <module>
    sys.exit(cli())
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 1635, in invoke
    rv = super().invoke(ctx)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/gitlint/cli.py", line 313, in cli
    ctx.invoke(lint)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/gitlint/cli.py", line 363, in lint
    violations = linter.lint(commit)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/gitlint/lint.py", line 94, in lint
    rule.apply(self.config, commit)
  File "/home/adam/.local/pipx/venvs/gitlint-core/lib/python3.10/site-packages/gitlint/rules.py", line 479, in apply
    if regex_method(commit.author_name):
TypeError: expected string or bytes-like object
$

The fix should be very simple, just adding a None check to the offending line so it is if commit.author_name is not None and regex_method(commit.author_name):, although maybe a warning should be printed since if gitlint is run without --staged the ignore-by-author-name rule will never be able to indicate that the rule should be ignored.

jorisroovers commented 1 year ago

This makes sense - thanks for submitting.

Gitlint doesn't do warnings today (except for deprecation warnings). If we introduce them, there should probably be a way to suppress specific warnings.

I'll fix this, but not sure yet which route I'll end up going...

Thanks!

jorisroovers commented 1 year ago

So this also makes gitlint crash when passing anything in via stdin:

# Crashes
echo "foo" | gitlint
# Does not crash
echo "foo" | gitlint --staged

This is pretty bad. Working through some other things but will fix this soonish, probably without the warning since it would be unexpected for the warning to show up in this case.

edit: clarification that this only crashes when the user is using an ignore-by-author-name rule

jorisroovers commented 1 year ago

Fixed! I did end up adding a warning :) You can’t suppress it for now, maybe that’s something I’ll add in the future.

For (my own) reference: author-valid-email has a similar issue, but the rule is already silently ignored in case commit.author_name is not available. I think that’s ok (at least for now), since contrary to the ignore-by-author-name rule, the author-valid-email rule is enabled by default. Adding a warning now is likely just going to add a bunch of spam for users that are perfectly fine with the currently behavior.