8 ms·

Show HN: Git Confirm. Git hook to catch accidentally committed code (TODO/Skip)

51 points by pimterry 10y ago

samstokes 10y ago
I wish Github had a way of running these sort of checks before accepting/rejecting a push. (I can understand why they would be reluctant to run arbitrary user-supplied bash on their servers, though :))
- cheald 10y ago
  In the Github case it's probably better to run these checks as part of your CI process and fail the build if they raise alarms.
- praseodym 10y ago
  The GitHub flow [1] is to use pull requests for everything. Aside from being a great code review tool, you can execute CI and other checks on them, even making it required for those checks to pass before merging [2]. You can quite easily hook your own checks [3], like the one from the original post or even a check whether the committer signed a CLA (Spring Boot does this [4]). [1] https://guides.github.com/introduction/flow/ https://guides.github.com/introduction/flow/ [2] https://help.github.com/articles/enabling-required-status-checks/ https://help.github.com/articles/enabling-required-status-ch... [3] https://developer.github.com/v3/repos/statuses/ https://developer.github.com/v3/repos/statuses/ [4] https://github.com/spring-projects/spring-boot https://github.com/spring-projects/spring-boot
- daveguy 10y ago
  GitLab does provide this capability. They have a free CI thread even on the free accounts. GitLab is much better than GitHub. Edit: well, maybe not exactly this functionality. It is CI based and not hook based. Still free CI thread is pretty awesome and can be used in this type of application.
themckman 10y ago
There isn't a lot that peeves me more than working with developers with poor git commit habits. As someone who takes every commit they make seriously, it can really demotivate/depress me when I look at my team's git log and see a series of poorly thought out commits and messages. It's always enjoyable to look at a piece of code, wonder why it is the way it is and to find that answer in a well written commit message by the author of the change. If a tool like his can at least help developers keep the diff portion of their commits usable, then I hope more people consider using it. It'd be interesting if it could learn from a project's history and have a developer confirm that they really want to commit files that it has determined to be unrelated, together.
- nathancahill 10y ago
  Same. Last project I worked on, most people used git add ., git commit, without ever looking at what was staged. The peeves were real. I'd love a tool that didn't let you commit unrelated files.
  - jbverschoor 10y ago
    And that's how you find those lovely credentials on github.
- e40 10y ago
  What fixed this for us is Gerrit. http://code.google.com/p/gerrit/ http://code.google.com/p/gerrit/ It's been fantastic. People fear change, but after a short while, everyone was digging the benefits of commenting on other people's code and other people helping them make their code better.
coherentpony 10y ago
You can also git add -p, but that's a habit you have to develop.
- pimterry 10y ago
  I built this mostly because I really like git add -p, but afaict it doesn't work at all for adding totally new files, and it's easy to miss things in the middle of big chunks of changes. But yeah, that's exactly the inspiration.
  - abusque 10y ago
    > but afaict it doesn't work at all for adding totally new files You can use git add --intent-to-add on your new file (or -N for short), and then use git add -p as usual. You can then use the edit mode to remove hunks you don't want to commit just yet. Not quite as convenient as on an already tracked file, but it's reasonably usable.
throwawayReply 10y ago
It's a good idea but ultimately the people who don't care about this will start doing: git commit -am "" /y
- happyslobro 10y ago
  Don't forget -n (--no-verify). The last thing we need is some automated test suite getting in our way ;)
jdoliner 10y ago
This looks pretty similar to `git commit -v` which brings up a diff of changes in a text editor and prompts you for a commit message.
- nilved 10y ago
  Those things are not really at all similar.
  - jdoliner 10y ago
    I guess they differ in that one interactively prompts you for each change and one just shows you all the changes at once. But it's similar in that both are a way to confirm that you're committing only what you want to commit.
Sir_Cmpwn 10y ago
Git hooks are pretty useful for catching yourself. I recently put this in .git/hooks/pre-commit for one of my repos: #!/usr/bin/env bash npm run lint
dweinus 10y ago
Does anyone know of anything similar as a Sublime package? I already do my linting there, so this would be a great addition.
- dweinus 10y ago
  In case anyone else is interested, this looks like a way to get there: http://www.sublimelinter.com/en/latest/creating_a_linter.html http://www.sublimelinter.com/en/latest/creating_a_linter.htm...
bArray 10y ago
I'm a regular at leaving "TODO" in the code. It's a reminder that the implementation wasn't great and requires another look at some point. If something breaks it's the first thing I fix in that related code. It's okay to simply not have time to write every piece of code to perfection - it's important to capture that fact though. Another regular thing I use is "NOTE" for code that really is not obvious at all. If my code doesn't read easily, other programmers (or my future self) know to check it for in depth information. If at the time of writing code you recognise it's difficult to understand or not completely correct then I think it's the perfect time to capture that information into the code.
- YZF 10y ago
  What's going to happen is that TODO is going to: a) stay in the code forever. b) seen 5 years later where it doesn't even make sense any more and by a different person. If it's difficult to understand or not correct don't commit it. Just fix it. It's a false economy. Friends don't let friends TODO ...
  - btym 10y ago
    Yes, do everything all at once. And never be able to release anything.
    - YZF 10y ago
      That's not what I said. Do what needs to be done at a given point in time to release. Just don't soothe your conscience by putting a TODO in the code. 99% that TODO will never get done, it's just clutter and code smell. If your code is broken you're releasing something that's broken which is probably going to end badly. If the code is adequate then it's adequate. Often it's just perception that doing it wrong is faster than doing it right. Prefer doing it right. (EDIT- doing it right is often faster)
      - bArray 10y ago
        I'm not sure this model works, in at least my case. Where there is perhaps some circuit you need to validate actually works with some sort of code - it's often useful to write some bad display driver for example so that the hardware team knows what needs to be changed and that process is not blocked. The code may drive a very dirty clock signal that tells them: 1. Circuit works in the basic sense 2. Some part of the circuit doesn't work 3. Some other part of the circuit can now be added now that the basic circuit has been confirmed It's okay saying "build the entire thing perfectly", but spending the difference between one and two weeks validating that something at least has the chance of working is massive. You can't afford to block every team that needs to know whether the part you are writing is even a valid concept. This may come back down to design, but sometimes - especially with hardware, the manufacturer/API doesn't tell you the whole story and you need to know that as soon as possible to start compensating for it. I think building a rough implementation and re-factoring it is way more agile than trying to build the correct version first time, possibly leaving some unknown problem to remain undiscovered for longer. It seems backwards to me when we're talking about anything that has an element of risk.
web007 10y ago
curl https://.../hook.sh > .git/hooks/pre-commit This seems like a bad idea, since you may already have a commit hook that this would clobber. Does anyone use any kind of plugin manager for git hooks? I know I can just build a shellscript (and have done so extensively) but is there something like pathogen / npm / bundler for a pluggable git hook architecture? https://github.com/brigade/overcommit https://github.com/brigade/overcommit seems to be an option, anyone have experience with that?
- delinka 10y ago
  curl ... >> .git/hooks/pre-commit Append instead
  - avar 10y ago
    That's horrible advice. The existing .git/hooks/pre-commit may not even be a shellscript, you could be appending to a binary file. Even if it's a shellscript you can't just concatenate shellscripts together and expect them to work as thought you'd run them as two independent shellscripts.
    - delinka 10y ago
      Presumably, one's git hooks are not in danger of being lost to oblivion. If that's the case, neither bit of advice is ideal. If, however, one is careless enough that unix plumbing with obliterate the hook for eternity, then appending at least allows the original to exist some place.
- spenuke 10y ago
  It's hard to completely generalize for the clobbering reason you point out. Assuming that you essentially want to keep your hooks coupled with your source code (or, at least, it's ok to do so), I've found that Make works pretty well here. In the root of the project directory, you have a subdirectory containing all the hooks you need (or use a git submodule), and then you create a Make target to copy those hooks (and make them executable) to your .git/hooks directory. Make is nice because it won't clobber existing ones (unless they are older, in which case you probably want them overwritten). Then, whenever someone first clones the repo (or whenever you make a change to the hooks), they just need to run make on that target again. Obviously, not an ideal choice for everyone, but it's pretty simple and works well for me.
john_oshea 10y ago
git config --add hooks.confirm.match "AKIA" would probably be a good default for AWS users.
- carterehsmith 10y ago
  For sure. If I remember correctly, maybe a year ago, Github actually went through public repos and emailed the people that had AKIAs in the repos. Apparently there were many of them. Myself, upon reading about that, I went through our (non-public) repo and, sure enough, found like a dozen AKIAs with secret keys and all. Also found a random AKIA in some binary file, false alarm. But then I was like... wait a second. How about .pem files? Yup, found several. .cer (some SSL certs), id_rsa? - yes to all. That took a while to fix.
jquast 10y ago
You could just add this pattern to something like https://github.com/landscapeio/dodgy https://github.com/landscapeio/dodgy -- Looks at Python code to search for things which look "dodgy" such as passwords or diffs"
boardwaalk 10y ago
Honestly I think the best thing you can do towards making good commits is having a good GUI client and using it well. What I do is, after I'm done making a mess of things, I click through each new/changed file and study the diff. I might need to commit or discard or edit specific chunks, but at the end everything is staged. Then I can click through the staged files quickly to check for mistakes while formulating a good commit message. (You could also use DIFF=vimdiff or something too) I find looking at individual hunks and files and staging them piecewise makes it much easier to maintain an attention to detail than doing a 'git diff' where everything flows together and 'git commit -a' where you might miss untracked or temporary files.
- wingerlang 10y ago
  I agree, some time ago I wrote down why I liked GUIs and this was the main point. A relevant part is: This also means that for every commit, you ‘must’ skim through each file and stage only what you want to commit. This serves as a final reminder of your code and has on multiple occasions led me to moments where I had added some temp code which wasn’t supposed to be committed. Full page http://jontelang.com/blog/2016/03/02/committing-with-sourcetree.html http://jontelang.com/blog/2016/03/02/committing-with-sourcet...
amitmerchant 10y ago
This looks promising!

51 points by pimterry 10y ago

I wish Github had a way of running these sort of checks before accepting/rejecting a push. (I can understand why they would be reluctant to run arbitrary user-supplied bash on their servers, though :))

cheald 10y ago
In the Github case it's probably better to run these checks as part of your CI process and fail the build if they raise alarms.
praseodym 10y ago
The GitHub flow [1] is to use pull requests for everything. Aside from being a great code review tool, you can execute CI and other checks on them, even making it required for those checks to pass before merging [2]. You can quite easily hook your own checks [3], like the one from the original post or even a check whether the committer signed a CLA (Spring Boot does this [4]). [1] https://guides.github.com/introduction/flow/ https://guides.github.com/introduction/flow/ [2] https://help.github.com/articles/enabling-required-status-checks/ https://help.github.com/articles/enabling-required-status-ch... [3] https://developer.github.com/v3/repos/statuses/ https://developer.github.com/v3/repos/statuses/ [4] https://github.com/spring-projects/spring-boot https://github.com/spring-projects/spring-boot
daveguy 10y ago
GitLab does provide this capability. They have a free CI thread even on the free accounts. GitLab is much better than GitHub. Edit: well, maybe not exactly this functionality. It is CI based and not hook based. Still free CI thread is pretty awesome and can be used in this type of application.

themckman 10y ago

There isn't a lot that peeves me more than working with developers with poor git commit habits. As someone who takes every commit they make seriously, it can really demotivate/depress me when I look at my team's git log and see a series of poorly thought out commits and messages. It's always enjoyable to look at a piece of code, wonder why it is the way it is and to find that answer in a well written commit message by the author of the change. If a tool like his can at least help developers keep the diff portion of their commits usable, then I hope more people consider using it. It'd be interesting if it could learn from a project's history and have a developer confirm that they really want to commit files that it has determined to be unrelated, together.

nathancahill 10y ago
Same. Last project I worked on, most people used git add ., git commit, without ever looking at what was staged. The peeves were real. I'd love a tool that didn't let you commit unrelated files.
- jbverschoor 10y ago
  And that's how you find those lovely credentials on github.
e40 10y ago
What fixed this for us is Gerrit. http://code.google.com/p/gerrit/ http://code.google.com/p/gerrit/ It's been fantastic. People fear change, but after a short while, everyone was digging the benefits of commenting on other people's code and other people helping them make their code better.

coherentpony 10y ago

You can also git add -p, but that's a habit you have to develop.

pimterry 10y ago
I built this mostly because I really like git add -p, but afaict it doesn't work at all for adding totally new files, and it's easy to miss things in the middle of big chunks of changes. But yeah, that's exactly the inspiration.
- abusque 10y ago
  > but afaict it doesn't work at all for adding totally new files You can use git add --intent-to-add on your new file (or -N for short), and then use git add -p as usual. You can then use the edit mode to remove hunks you don't want to commit just yet. Not quite as convenient as on an already tracked file, but it's reasonably usable.

throwawayReply 10y ago

It's a good idea but ultimately the people who don't care about this will start doing: git commit -am "" /y

happyslobro 10y ago
Don't forget -n (--no-verify). The last thing we need is some automated test suite getting in our way ;)

jdoliner 10y ago

This looks pretty similar to `git commit -v` which brings up a diff of changes in a text editor and prompts you for a commit message.

nilved 10y ago
Those things are not really at all similar.
- jdoliner 10y ago
  I guess they differ in that one interactively prompts you for each change and one just shows you all the changes at once. But it's similar in that both are a way to confirm that you're committing only what you want to commit.

Sir_Cmpwn 10y ago

Git hooks are pretty useful for catching yourself. I recently put this in .git/hooks/pre-commit for one of my repos: #!/usr/bin/env bash npm run lint

dweinus 10y ago

Does anyone know of anything similar as a Sublime package? I already do my linting there, so this would be a great addition.

dweinus 10y ago
In case anyone else is interested, this looks like a way to get there: http://www.sublimelinter.com/en/latest/creating_a_linter.html http://www.sublimelinter.com/en/latest/creating_a_linter.htm...

bArray 10y ago

I'm a regular at leaving "TODO" in the code. It's a reminder that the implementation wasn't great and requires another look at some point. If something breaks it's the first thing I fix in that related code. It's okay to simply not have time to write every piece of code to perfection - it's important to capture that fact though. Another regular thing I use is "NOTE" for code that really is not obvious at all. If my code doesn't read easily, other programmers (or my future self) know to check it for in depth information. If at the time of writing code you recognise it's difficult to understand or not completely correct then I think it's the perfect time to capture that information into the code.

YZF 10y ago
What's going to happen is that TODO is going to: a) stay in the code forever. b) seen 5 years later where it doesn't even make sense any more and by a different person. If it's difficult to understand or not correct don't commit it. Just fix it. It's a false economy. Friends don't let friends TODO ...
- btym 10y ago
  Yes, do everything all at once. And never be able to release anything.
  - YZF 10y ago
    That's not what I said. Do what needs to be done at a given point in time to release. Just don't soothe your conscience by putting a TODO in the code. 99% that TODO will never get done, it's just clutter and code smell. If your code is broken you're releasing something that's broken which is probably going to end badly. If the code is adequate then it's adequate. Often it's just perception that doing it wrong is faster than doing it right. Prefer doing it right. (EDIT- doing it right is often faster)
    - bArray 10y ago
      I'm not sure this model works, in at least my case. Where there is perhaps some circuit you need to validate actually works with some sort of code - it's often useful to write some bad display driver for example so that the hardware team knows what needs to be changed and that process is not blocked. The code may drive a very dirty clock signal that tells them: 1. Circuit works in the basic sense 2. Some part of the circuit doesn't work 3. Some other part of the circuit can now be added now that the basic circuit has been confirmed It's okay saying "build the entire thing perfectly", but spending the difference between one and two weeks validating that something at least has the chance of working is massive. You can't afford to block every team that needs to know whether the part you are writing is even a valid concept. This may come back down to design, but sometimes - especially with hardware, the manufacturer/API doesn't tell you the whole story and you need to know that as soon as possible to start compensating for it. I think building a rough implementation and re-factoring it is way more agile than trying to build the correct version first time, possibly leaving some unknown problem to remain undiscovered for longer. It seems backwards to me when we're talking about anything that has an element of risk.

web007 10y ago

curl https://.../hook.sh > .git/hooks/pre-commit This seems like a bad idea, since you may already have a commit hook that this would clobber. Does anyone use any kind of plugin manager for git hooks? I know I can just build a shellscript (and have done so extensively) but is there something like pathogen / npm / bundler for a pluggable git hook architecture? https://github.com/brigade/overcommit https://github.com/brigade/overcommit seems to be an option, anyone have experience with that?

delinka 10y ago
curl ... >> .git/hooks/pre-commit Append instead
- avar 10y ago
  That's horrible advice. The existing .git/hooks/pre-commit may not even be a shellscript, you could be appending to a binary file. Even if it's a shellscript you can't just concatenate shellscripts together and expect them to work as thought you'd run them as two independent shellscripts.
  - delinka 10y ago
    Presumably, one's git hooks are not in danger of being lost to oblivion. If that's the case, neither bit of advice is ideal. If, however, one is careless enough that unix plumbing with obliterate the hook for eternity, then appending at least allows the original to exist some place.
spenuke 10y ago
It's hard to completely generalize for the clobbering reason you point out. Assuming that you essentially want to keep your hooks coupled with your source code (or, at least, it's ok to do so), I've found that Make works pretty well here. In the root of the project directory, you have a subdirectory containing all the hooks you need (or use a git submodule), and then you create a Make target to copy those hooks (and make them executable) to your .git/hooks directory. Make is nice because it won't clobber existing ones (unless they are older, in which case you probably want them overwritten). Then, whenever someone first clones the repo (or whenever you make a change to the hooks), they just need to run make on that target again. Obviously, not an ideal choice for everyone, but it's pretty simple and works well for me.

john_oshea 10y ago

git config --add hooks.confirm.match "AKIA" would probably be a good default for AWS users.

carterehsmith 10y ago
For sure. If I remember correctly, maybe a year ago, Github actually went through public repos and emailed the people that had AKIAs in the repos. Apparently there were many of them. Myself, upon reading about that, I went through our (non-public) repo and, sure enough, found like a dozen AKIAs with secret keys and all. Also found a random AKIA in some binary file, false alarm. But then I was like... wait a second. How about .pem files? Yup, found several. .cer (some SSL certs), id_rsa? - yes to all. That took a while to fix.

jquast 10y ago

You could just add this pattern to something like https://github.com/landscapeio/dodgy https://github.com/landscapeio/dodgy -- Looks at Python code to search for things which look "dodgy" such as passwords or diffs"

boardwaalk 10y ago

Honestly I think the best thing you can do towards making good commits is having a good GUI client and using it well. What I do is, after I'm done making a mess of things, I click through each new/changed file and study the diff. I might need to commit or discard or edit specific chunks, but at the end everything is staged. Then I can click through the staged files quickly to check for mistakes while formulating a good commit message. (You could also use DIFF=vimdiff or something too) I find looking at individual hunks and files and staging them piecewise makes it much easier to maintain an attention to detail than doing a 'git diff' where everything flows together and 'git commit -a' where you might miss untracked or temporary files.

wingerlang 10y ago
I agree, some time ago I wrote down why I liked GUIs and this was the main point. A relevant part is: This also means that for every commit, you ‘must’ skim through each file and stage only what you want to commit. This serves as a final reminder of your code and has on multiple occasions led me to moments where I had added some temp code which wasn’t supposed to be committed. Full page http://jontelang.com/blog/2016/03/02/committing-with-sourcetree.html http://jontelang.com/blog/2016/03/02/committing-with-sourcet...

amitmerchant 10y ago

This looks promising!