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

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

github.com

51 pointspimterry45 comments

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.

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.

And that's how you find those lovely credentials on github.

What fixed this for us is 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.

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.

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 ...

Yes, do everything all at once. And never be able to release anything.

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)

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.

TODO messages are a terrible taxation on comprehension of code.

Most TODO items should be written "TODIDNT", what would we guess, 99% of TODO items never happen? What is the thinking here, "I knew I should have written the code more correctly, but I didn't -- commenting about what I failed to do here gives me a pass"?

Either do it, or don't. Don't tell everybody about what you didn't do, it reduces comprehension of the code at hand.

I'm a fan of a DESIGN.txt or other document that everyone can bitch and moan about shortcomings into. It's centralized and threaded by contributions, something like a wikimedia Talk page.

How do you reference a particular bit of code? For example, when you're writing the code you are aware that a number of lines could definitely do with improving but will get you out of the clear for the moment and past the milestone/deliverable. How do you specifically reference the code that you know is bad in this system?

By naming the file, class, method, or function name, or even variables or values discussed.

Don't you find they get updated, moved or renamed? I personally have to deal with scope creep in my projects, so things often change from a strict design. But I imagine a longer project doing the same?

The reason most people tell me they use todos is, "you can't do everything up front."

I agree, but I hate TODOs. They're the worst form of keeping track of technical debt.

Just open a ticket! I like to have a tag/label called "debt" that I put these "TODO, bad implementation" type of problems under.

That way at least it's tracked.

A preferred method for doing this at Google is to open a bug, and then leave the TODO in the code with a reference to the bug number. This has the advantage that if someone comes by to refactor the code later, they'll have the chance to go look at the bug (possibly taking it and working on it, possibly resolving it as obsolete if the original issue no longer applies, etc).

Having the two-directional link between the bug and the code is quite useful, especially for what is probably the most frequent use I've seen for TODOs: "We could do this better, but the better way is blocked on circumstances beyond our control. Once those change, revisit this."

I actually like that - perhaps that allows you to implement blocking as well, for example: "TODO: X cannot be improved until Z & Y is complete.".

The projects I work on tend to have a very quick life cycle, so there isn't even a ticketing system in place other than the occasional post it note. There just isn't the time and the team is tight.

This is my preferred method, but if there is any sort of bureaucracy in place around creating a ticket then it's not a solution that gets used.

Background: I work mostly with projects that have <1 year delivery date.

It's always better to fix the code I completely agree - in reality there simply isn't always the time. In an idealistic world products aren't released until they are ready and bugs are never produced.

But... There's deadlines and there's certainly not time allocated to fixing code unless a bug appears there. Once a milestone or deliverable is met, as far as a manager is concerned it's case closed.

Whilst I agree with you from an idealistic stand point, for myself and my line of work I need to leave TODOs to fix the code if the time permits before the release time. Corners get cut and you need some way of knowing where those corners are.

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.

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-sourcet...

  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 seems to be an option, anyone have experience with that?

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.

https://github.com/brigade/overcommit

It's wonderful.

  curl ... >> .git/hooks/pre-commit

Append instead

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.

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.

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

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

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.

> 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.

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.

Those things are not really at all similar.

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.

    git config --add hooks.confirm.match "AKIA"

would probably be a good default for AWS users.

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.

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

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 :))

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.

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/ [2] https://help.github.com/articles/enabling-required-status-ch... [3] https://developer.github.com/v3/repos/statuses/ [4] https://github.com/spring-projects/spring-boot

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.

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

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

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

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

This looks promising!

Reply on news.ycombinator.com