rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
382 stars 20 forks source link

Feature: Add new hooks in Git 2.28 #147

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

Git 2.28.0 has new hooks: See changes...

Is it ok to output everything to stderr in base-template.sh? I think yes, since that is already done by Git as I experienced (can you check this?)

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 885


Changes Missing Coverage Covered Lines Changed/Added Lines %
install.sh 0 1 0.0%
<!-- Total: 19 20 95.0% -->
Files with Coverage Reduction New Missed Lines %
cli.sh 3 84.31%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 872: -0.1%
Covered Lines: 2369
Relevant Lines: 2953

💛 - Coveralls
rycus86 commented 4 years ago

Is it ok to output everything to stderr in base-template.sh?

I think I'd prefer to leave it on stdout, unless they're all exceptional messages? (update notices could perhaps also go there if that's the only other thing)

I had a quick look at the new hooks at https://git-scm.com/docs/githooks and I'm not sure if fsmonitor-watchman is something I can think of a good use-case for with Githooks to be honest. post-index-change also sounds odd for me, but I could imagine people usong that one for automation. The other two new ones look good. 👍

What do you think?

gabyx commented 4 years ago

This is true, but this project is not dependent on the hooks basically. It should work with all of them. 🤞 Eventhough fsmonitor-watchman is a funny one...

I strongly think, everything should go to stderr. I tried to contact the git mailing list about how the hooks work underneath, which descriptors get attached to what (?) it seems that git collects stdout/stderr of the hook into stderr. You can verify this in this repo by doing git commit --amend 2>/devnull which results in no output.

I would really change this, also because if there are changes in the hooks such that suddenly something is parsed on stdout by Git (which is unlikely to happen I think) its better to just report stuff on stderr.

rycus86 commented 4 years ago

This is true, but this project is not dependent on the hooks basically. It should work with all of them. 🤞 Eventhough fsmonitor-watchman is a funny one...

I think practically it's easier to say it's unsupported if no one really uses it with Githooks, than saying it's supported then dealing with odd quirks for it. 🙂

I strongly think, everything should go to stderr. I tried to contact the git mailing list about how the hooks work underneath, which descriptors get attached to what (?) it seems that git collects stdout/stderr of the hook into stderr. You can verify this in this repo by doing git commit --amend 2>/devnull which results in no output.

I would really change this, also because if there are changes in the hooks such that suddenly something is parsed on stdout by Git (which is unlikely to happen I think) its better to just report stuff on stderr.

So does this mean all the things we log to stdout currently already go to stderr? If that is the case then O don't mind if we also log to stderr if that doesn't practically change anything anyway.

gabyx commented 4 years ago

Yes, all things go to stderr. (Also with the go-port, I realized that sometimes, also on windows stderr and stdout get interleaved which is nasty..., other problem)

Whats your suggestion: Shall only drop fsmonitor-watchman and leave the output to stderr. I am bit afraid that Git reads from certain hooks (even if it doesnt, I think of the hook as a middleware connected to stdin and stdout by git, stderr is the only think Git will definitely not read from)... or drop this too...

rycus86 commented 4 years ago

I'd prefer keeping stderr changes, dropping fsmonitor-watchman changes based on your comments. 🙏

gabyx commented 4 years ago

See my latest commit.

rycus86 commented 4 years ago

@gabyx Thanks, added a couple of comments.