Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of piped data in CI environment #87

Closed
wants to merge 3 commits into from

Conversation

rogalski
Copy link
Contributor

Basically, gitlint failed to work successfully on internally-hosted QuickBuild instance.

Apparently "\n" string was piped to gitlint process, which was enough to early exit from "local repository" case. This patch attempts to avoid such case by rejecting whitespace-only piped content. Also, additional debug log is added to improve available debug info when -d switch is used.

@jorisroovers jorisroovers force-pushed the master branch 2 times, most recently from 1bb47e1 to af74eb2 Compare June 19, 2019 16:42
@jorisroovers
Copy link
Owner

Hi, thanks for your interest in gitlint and creating this PR!

So this is an interesting case, because it gets down into the semantics of what constitutes a git commit message.
While the following example might seem a bit out there, it's technically not impossible to have an white-space only commit message:

$ git commit --allow-empty-message -m "   " --cleanup=verbatim
$ echo "foo$(git log -1 --pretty=%B)bar"
foo   bar
$ # In this case, gitlint still acts as expected
$  git log -1 --pretty=%B | gitlint
1: T2 Title has trailing whitespace: "   "
1: T6 Title has leading whitespace: "   "
3: B6 Body message is missing

I would agree that most people would probably never do this, but then again there are a lot of different git and CI/CD setups (and sometimes it's not humans but machines (not) writing commit messages). The challenge with merging this PR is that there is then no way to still support this whitespace commit example.

I think we might need to look at implementing #56, which is basically a force flag for using the local repo.
Wrt logging the input data for debugging: yes, that should definitely be added :-)

@jorisroovers
Copy link
Owner

We ended up implementing the --ignore-stdin flag since v0.12.0 which can be used to deal with this scenario - see #89 and #56. Closing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants