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

Fix stdin_has_data() #52

Closed
wants to merge 1 commit into from
Closed

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented Jan 17, 2018

When calling gitlint in a custom git commit hook on a gitlab server, stdin_has_data() will always return True regardless whether gitlint is called directly or text is piped into it. To reproduce you can call this
script:

#!/usr/bin/python3

import sys
import stat
import os

if sys.stdin.isatty():
    print("Input comes from tty.")
else:
    print("Input doesn't come from tty.")

mode = os.fstat(sys.stdin.fileno()).st_mode
if stat.S_ISFIFO(mode):
    print("stdin is piped")
elif stat.S_ISREG(mode):
    print("stdin is redirected")
else:
    print("stdin is terminal")

On my server it prints:

Input doesn't come from tty.
stdin is terminal

stdin should only be read if the user uses a pipe or a redirect to pass data in.

When calling gitlint in a custom git commit hook on a gitlab server,
stdin_has_data() will always return True regardless whether gitlint is
called directly or text is piped into it. To reproduce you can call this
script:

import sys, stat, os

if sys.stdin.isatty():
    print("Input comes from tty.")
else:
    print("Input doesn't come from tty.")
mode = os.fstat(sys.stdin.fileno()).st_mode
if stat.S_ISFIFO(mode):
    print("stdin is piped")
elif stat.S_ISREG(mode):
    print("stdin is redirected")
else:
    print("stdin is terminal")

On my server it prints:

Input doesn't come from tty.
stdin is terminal

stdin should only be read if the user uses a pipe or a redirect to pass
data in.
@jorisroovers
Copy link
Owner

I got similar feedback in #42 after I thought I fixed it with the last release. I need to make some more time to play with this, because this is a fundamental part of gitlint. I want to understand and test it properly myself before merging a PR related to it.

I don't know when I have time, but I'm going to try and make some in the next few weeks since this is a fairly big issue. Appreciate your patience!

I think the following should work as a temp workaround: git log -1 --pretty=%B | gitlint

@bdrung
Copy link
Contributor Author

bdrung commented Jan 19, 2018

My workaround is to use this patch. 😉 So there is no hurry on my side.

Some test cases you can use:

$ echo | gitlint
$ gitlint < textfile
$ gitlint

@pbregener
Copy link
Contributor

pbregener commented Feb 7, 2018

BTW: The workaround you described @jorisroovers (piping the commit message body into gitlint) doesn't work if you have rules regarding author names and/or email addresses.

@pbregener
Copy link
Contributor

As mentioned in #42, the "has-STDIN-data-check" doesn't work inside certain Jenkins and GitLab CI environments.
Unfortunately, this patch doesn't fix that problem, either (tested on GitLab CI).

@bdrung
Copy link
Contributor Author

bdrung commented Feb 21, 2018

@pbregener Have you tested my proposed patch? Can you comment how you configured GitLab CI? I am using this patch with GitLab.

@pbregener
Copy link
Contributor

Yes. I am running gitlint in a pretty standard GitLab CI config using the Docker executor and the latest gitlab-runner (v10.4.0).

@bdrung
Copy link
Contributor Author

bdrung commented Feb 21, 2018

v10.4.0 does not contain the proposed patch from this pull request. Can you apply the patch from this pull request and try again? How do you invoke gitlint?

@pbregener
Copy link
Contributor

I applied the patch from this pull request to gitlint. I'm not sure what the gitlab-runner version has to do with this. I invoke gitlint from a bash script, which is linked in the project's .gitlab-ci.yml.

@pbregener
Copy link
Contributor

You mentioned that you use it in GitLab with a commit hook which will be an entirely different thing.

@jorisroovers
Copy link
Owner

Closing this out, this got fixed in 5c93309. More context in #42.

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

Successfully merging this pull request may close these issues.

None yet

3 participants