4 ms·

Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whis

by pdovy 13y ago

Does that really work well in practice, though? I find that I might get the base functionality down to a workable state, but then as you add the bells and whistles you're likely to find and fix your own bugs, realize you need to refactor, etc. You can make all those changes in distinct, clean commits, but if the reviewer works commit by commit, they have to follow the evolution of your code rather than simply review the final state. You might argue that provides a basis for a better review, but it's also much more time consuming for the reviewer.

jakub_g 13y ago
When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this. You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.
deathanatos 13y ago
You don't have to push immediately to review: you commit a few commits locally, until you're sure of the architecture. Once you're sure, start pushing the earlier ones out to review. If you're really unsure of the design, then tackle that in a design doc first. Discuss what the API will look like, write out sample call sites to show its use. Otherwise, yeah, you'll probably end up re-writing it. Small commits have another benefit: they can be moved (cherry picked, for example to backport) or reverted easily.
taude 13y ago
When the feature is complete and ready for review, just put the final commit id in with the ticket. Compare it to some version before ticket was being worked on? I think it's easier to look at two different versions of checkins, than to hoard code and make a single huge checkin.

by pdovy 13y ago

jakub_g 13y ago

When you find an issue with previous commit while adding new things, you can make a temporary commit, fix the issue, commit once again and reorder commits, then meld the fix into the previous one. I'm pretty comfortable with doing things like that but some guys not that much. You have to like this workflow and work from the start like this. You can of course split a huge commit into smaller ones using sth like `git add -p` but it's way more work than just doing lots of small commits regularly and then squashing them into medium-sized commits before the review. I like to do temporary commits regularly as "checkpoints", meaning, "this code works", and anytime I make a stupid mistake and break something, I can analyze a short diff to find the bug I just introduced rather than trying to figure out the bug just by looking at the code. It's usually a lot faster for me.

deathanatos 13y ago

You don't have to push immediately to review: you commit a few commits locally, until you're sure of the architecture. Once you're sure, start pushing the earlier ones out to review. If you're really unsure of the design, then tackle that in a design doc first. Discuss what the API will look like, write out sample call sites to show its use. Otherwise, yeah, you'll probably end up re-writing it. Small commits have another benefit: they can be moved (cherry picked, for example to backport) or reverted easily.

taude 13y ago

When the feature is complete and ready for review, just put the final commit id in with the ticket. Compare it to some version before ticket was being worked on? I think it's easier to look at two different versions of checkins, than to hoard code and make a single huge checkin.