8 ms·
Git Things
- cesnja 3y agoDue to (async) code reviews slowing down the development, we've adopted some workflows described at https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html and it's working out pretty great. The committer decides how many approvals they need to merge a pull request, based on how confident they are about the code they wrote. This allows us to quickly merge small incremental improvements that would otherwise be bundled in a crowded feature pull request.
- kageiit 3y agoI feel like the number of reviewers should be predicated more on how well the reviewers understand the relevant part of the code review. Most code review tooling is too coarse in the sense that it expects a small number of reviewers to know the full context of the change or has too many reviewers that slow down things. Tools to show relevant parts of the change to specific reviewers and potentially break apart a large change into smaller ones automatically will go a long way especially in large scale codebases or when team sizes are large enough that the full context is not understood by everyone on the team
- ozim 3y agoThis is wrong direction IMO. That is a technical solution to something that could be handled by the team. While tooling might help people in the team still have to make same and sound decisions like unblocking team member that needs to do a quick fix.
- blep_ 3y agoThe 50-character summary thing is bizarre. As far as I can tell, what happened is someone did statistics on summary lines, found that the average was around 50, and then louder people with strong opinions missed the word "average" and declared 50 as a maximum. My hot take is that you should use exactly as many characters as you need to write a meaningful summary, and people who insist on using tiny terminal windows can deal with the consequences of their actions.
- antonvs 3y agoFor a summary line, I don’t see big problem with recommending they be kept that short. The git commit docs say this: > Though not required, it’s a good idea to begin the commit message with a single short (no more than 50 characters) line summarizing the change, followed by a blank line and then a more thorough description. It’s like the headline of an article, it doesn’t need to tell you everything. That’s the job of the “more thorough description”. And if you omit the summary line, then you’re just writing a description, and the limit no longer applies. It’s not a hard limit anyway. I’m not sure it has to do with tiny terminal windows so much as encouraging people to be concise. It allows you to skim commits without reading essays, for example.
- ufo 3y agoTo add insult to injury, that 50 char maximum sneaked its way into several syntax highlighting and linting tools, so now my computer is also complaining about my commit messages.
- cerved 3y agoThat's not what happened
- dclowd9901 3y ago> When fixing a bug, add a failing test first, as a separate commit. That way it becomes easy to verify for anyone that the test indeed fails without the follow up fix. I would genuinely like to know in what world this is useful advice. Maybe in personal projects or small teams or companies with a maniacal lead? I just can’t imagine someone in a sufficiently large organization (which most companies hope to be someday) goes sifting through feature commits to get to the bottom of a failing test.
- pjbster 3y agoI think the failing test is meant to help with code review. The commit would be rebased out prior to merging into main.
- akoboldfrying 3y agoI don't really understand your reaction. Adding a test that fails before and passes after the fix is good practice (do you disagree?), and putting that in a separate commit is a trivial effort (do you disagree?) It's basically the lowest-friction way I know to do the part of TDD that counts.
- chris_wot 3y agoYeah, but it kills a git bisect run
- akoboldfrying 3y agoHow so? Adding 1 commit adds at most 1 (but usually 0) git bisect steps.
- chris_wot 3y agoYes, but if you run git bisect and then try to run your unit tests, a failure that was intentional will tell the bisect to potentially divide the commits at the wrong point. You'd need to either exclude the test (probably not a great idea) or mark the commits as skippable. Also not a great idea.
- barbegal 3y ago>if you want to reliably record file moves during refactors in git, you should do two commits: the first commit just moves the file without any changes, the second commit applies all the required fixups. Yes this will record a file move in one of the commits but if you diff between before and after the two commits a file move might not be shown. When thinking about file moves in git it is worth remembering this comes from the diff tool not from the commits since the commits just show the state of the file system at the point it is committed.
- forrestthewoods 3y agoThe fact that Git is incapable of effectively tracking files across moves is such a failure of Git. It’s critical information that ought to be robustly tracked.
- Izkata 3y agoThe flipside is when two files are combined, git blame can track history across both sources. Can't do that when explicit moves are recorded, one of the source files would be recorded and the others treated as new lines.
- forrestthewoods 3y agoOne of these is waaaaaay more common of an operation than the other.
- vinnymac 3y agoThe fact that it was brought up early in the article, tells us which one is a more common operation as well.
- Izkata 3y agoMoving single functions or groups of lines (like extracting something common into a function) I'd say is more common than even that, and this same functionality tracks those changes across files too.
- kageiit 3y agoI feel this advice has some merit when dealing with oss projects, systems/backend or small teams. It tends to break apart when working with large teams or when working on mobile apps where one can't just rollback a change easily after it's shipped. The descriptions need to be more detailed and capture lot more information. For example, our team would require all mobile devs to add information about feature flags for each change to turn the feature off if things go wrong. This also places additional review burden to make sure the flag covers all the new code introduced and does not interact in a bad way with other existing feature flags
- mfrw 3y ago> Second, not every change needs a great commit message. If a change is really minor, I would say minor is an okay commit message! I want to slightly disagree on this for folks who are early on in career e.g me; I think there is a lot of value in learning to communicate precisely and having a habit of writing good commit message has certainly improved my skills, both verbal and written. Even though, this activity appears futile for a minor change; it might help to improve other skills :)
- karlshea 3y agoYou’re totally right, if it’s that minor then you can type in a word or two to describe it like “spacing”.
- Izkata 3y agoEven something that simple falls into what vs why: You can tell it was spacing from the diff, so the commit message should be "linting" or "alignment" or something similarly more meaningful.
- quickthrower2 3y agoThe diff may not be visible. The person might be doing a complicated cherry pick or merge and scrolling through hundreds of commits. If the message explains it, it saves them digging in. My analogy is your bookshelf books still need jackets!
- Vinnl 3y agoFirst line is for what, the rest of the commit message is for the why - if needed. As for "spacing", you can probably tell the why from the diff as well. The "what", however, tells you whether it's even worth opening the diff.
- anordal 3y agoI also want to slightly disagree, or rather state a minimum: The most important part of a commit message is the context. Good enough commit log: utils: Minor drivers: Fix assets: Update docs: Typo Hopeless commit log: Minor Fix Update Typo Context should answer the "what". Think of it as the one word you tell a new conversation participant to let them realize what you are talking about.
- forrestthewoods 3y agoI can’t help but feel that the fact that rebase and merge are treated as different commands is broken. This topic keeps coming up on HN lately and it feels like the framing is all wrong. I dunno. At the end of the day it’s all just a sequence of commits and some commit labels.
- pjbster 3y agoIt feels like there's a tiny bit of conflicting advice in this. There's the "not rocket science" rule which results in "no CI-failing commits on main" and then there's this: > Third, our review process is backwards. Review is done before code gets into main, but that’s inefficient for most of the non-mission critical projects out there. A better approach is to optimistically merge most changes as soon as not-rocket-science allows it, and then later review the code in situ, in the main branch. But the tip about adding a failing test as a separate commit on the feature branch wouldn't survive a merge and it wouldn't live long enough to be reviewed either. I like most of the advice in this article but this review one is giving me pause.
- epage 3y agoThe idea is one commit adds the test and another fixes it.
- sam_bristow 3y agoAlso, the commit adding the failing test isn't breaking anything, its demonstrating the already broken nature of the existing codebase.
- 90319080 3y agoWax
- weavejester 3y agoWhy bother keeping minor commits in feature branches? They just clutter the main branch without adding any useful information that a future maintainer could use.
- kvdveer 3y agoIf the minor commits are related to the feature branch, by all means squash it into the feature commits if you feel that makes your feature branch easier to review. However,should you, while developing your feature, find some minor thing that needs improving (e.g. adding comments, spelling fixes or tests to some existing API you encountered while developing your feature), it can be better to keep it separate, so there's fewer red herring in both the history and the review.
- cerved 3y agoBecause it's much easier to rebase ontop of and it declutters the important commits. Typically, you git log main --first-parent
- IshKebab 3y agoThe sensible alternative is squashing the commit, not doing a rebase merge.
- gverrilla 3y agoI'm a beginner dev and I find it very odd both that vscode tries to limit my commit messages to 50 or 55 characters, and also that python linters want to make my lines of code so small. What's the reason behind this? Old small monitors of the past? I also don't get why fediverse, mastodon seem to limit characters on a post, mimicking the stupid twitter, which is a niche social network. I way be wrong about the limit (never tried), but these platforms feel for ignorant outsiders like twitter-clones with a different infrastructure.
- funcDropShadow 3y ago> tries to limit my commit messages to 50 or 55 characters The tools try to limit the summary line of Git commit message to 50 characters. Not the whole message. That is a useful convention. The usual convention is described here: https://git-scm.com/docs/git-commit#_discussion https://git-scm.com/docs/git-commit#_discussion
- wodenokoto 3y agoYes, old monitors are the reason why we have line lengths as they are. The reason why we still have them is a mix between inertia, it’s hard to read long lines and it’s nice to be able to have two files side by side. Mastodon is a twitter clone, with different infrastructure.
- gverrilla 3y agoI'm only a begginer, of course, but it seems to be long lines are better than broken ones. `MyDict[family][subfamily][element][property_a][value]` is much better imo than: ``` MyDict[family][subfamily][element][property_a][ value ] ``` Probably the data format is what's wrong here in the first place, lol, but anyhow.
- wodenokoto 3y agoWhen using pandas in Python you are encouraged to keep each “chained operation “ on its own line. (MyDict [family] [subfamily] [element] [property_a] [value] ) I honestly think all suggestions here are equally bad in their own regards.
- kingkongjaffa 3y ago> That’s why for typical project it is useful to merge pull requests into the main branch — the linear sequence of merge commits is a record of successful CI runs, and is a set of commits you want to git bisect over. This is kind of obvious after reading but no one ever explained this to me until now
- kissgyorgy 3y agoInstead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear // TODO: comment explaining what would be the correct result. This prevents such tests from rotting and also catches cases where the behavior is fixed by an unrelated change. Don't do that, because they are still comments and even worse, the tests are lying. In pytest, there is a way to tell that those tests are expected to fail: xfail, so it will show in the test results every time and it's clear it's just a temporary things and should be fixed. https://docs.pytest.org/en/7.1.x/how-to/skipping.html#xfail-mark-test-functions-as-expected-to-fail https://docs.pytest.org/en/7.1.x/how-to/skipping.html#xfail-...
- lexicality 3y ago> Second, not every change needs a great commit message. If a change is really minor, I would say minor is an okay commit message! It's very hard to rank them, but very high on my list of things that make me want to send people actionable threats is when I'm searching down the history of a bug to figure out if it was a deliberate change that had unexpected consequences or just a mistake is when the git blame path ends on a commit like this. I don't care how minor you think your change is, if you're working on something collaboratively you need to express intent in your commit messages!
- bad_alloc 3y ago> if you're working on something collaboratively you need to express intent in your commit messages! 100%! And if somebody cannot communicate the intent, there is a high chance they don't understand fully what they just did.
- cerved 3y agoI think the authors point is that some changes are so minor that elaboration is not required, like changing indentation. Elaborate commit messages are great, and most people write commit messages that could be more elaborate. But most people also tend to squash several changes together, because it's easier. Being overzealous about writing elaborate commit messages for minor cookies is IMHO probably counter productive.
- lexicality 3y agoNo change is minor enough that you don't need to explain what it does. If the commit message is "fix indentation" then I will ignore it and skip over it while searching the commit history. If it's "minor" or "fix" then I have to open the commit and look to see what it actually is. These "minor cookies" add up rapidly. (side note, people that squash important changes together are also on The List)
- cerved 3y agoLet's not forget people that commit trailing whitespace, or different line-endings. Can we put those on a separate List?
- quickthrower2 3y agoI am a merger not a rebaser but it is probably a tab/space level personal quirks distinction!
- hmeh 3y agoIt’s not. They are different things and have different purposes. Saying you’re a merger is like saying you’re a left turner. You can be an ambiturner. Here’s a quick guide: Working on a feature branch with someone else and you have local commits and they have remote commits? Rebase Need to rewrite history? Rebase For just about everything else, there’s merge.
- quickthrower2 3y agoThe information preserved by a merge is greater than or equal to that of a rebase. Since you can construct the rebase from a merge. Further, it allows you to work on changes that have been committed on main since you started the work. The downside is there can be dragons in the merge commit (you can make arbitrary changes there) so it requires discipline. I only rebase if the merges conflict and therefore introduce a chance for dragons. So, yeah sometimes I turn right.
- hmeh 3y agoI tend to think it's better to have merge conflict changes be in a merge commit. That way the original commits are still present and if there was a mistake made, it'll be in its own commit. Contrast that w/ rebasing, where the original commit goes away and gets modified to account for the conflict. Compound this w/ the fact that when you rebase you may have to resolve related merge conflicts in multiple commits, and I think rebasing to deal with merge conflicts is the opposite direction of what I would recommend.
- quickthrower2 3y agoI think it depends why the conflict happened. If it happened because say someone else did a refactor then rebasing on that refactor (and that might actually mean manually doing a lot of stuff) is easy to inspect. The problem comes through as a commit with something weird unrelated to the change. It does require line by line code review by the coder, which few might want to do, but I like to do before opening any PR. Because in any case it is so easy to have an editor window open while your cat walks on the keyboard and pastes your password, or whatever :-).
- aliceryhl 3y agoRegarding the review process ... one thing that I find challenging and don't know a good solution to is documentation. I've received many PRs where the change itself is fine, but the PR is dragging out because the documentation is lacking, and getting the PR author to improve it sometimes takes a lot of review rounds. What would you do to avoid this? Sometimes the same situation comes up with tests, but it is not as common in my experience.
- seb1204 3y agoMake it clear that documentation is part of the code. Missing or poor docu = code not acceptable.
- FPGAhacker 3y agoGet people to write the docs first. Not many people like writing docs after the fact, and much of the value of working documentation is lost if you do it after the implementation. Assuming we’re not taking about user guide kind of docs, then a major benefit of writing docs first is to clarify your thinking. Being able to explain your intent in the written word is valuable because you will often uncover gaps in your thinking. This applies to a specification, or to acknowledging problem reports and updating with theories on what the cause of said problem is and an approach to confirming or fixing it. You can even reference that problem report in commits and merge requests. It pretty beneficial all around. And docs don’t have to me masterpiece works of art. Just getting people to clarify intent is a huge win. Peer reviewers don’t have time to do a super deep dive into code. If they know what you intended code to do, that’s something many reviewers can check pretty quickly without having to know much context. It’s selfish and naive to disregard basic documentation of intent.
- Vinnl 3y agoOne option would be to take an initial stab at the documentation yourself - that makes it clear to the submitter where things are unclear, because you made mistakes or omitted things, and they can just correct that, which is a lot more feasible to do than figuring out what's important while your head is in the code.
- Aeolun 3y ago> Instead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear // TODO: comment explaining what would be the correct result. This prevents such tests from rotting and also catches cases where the behavior is fixed by an unrelated change. Yuck! Tests asserting wrong behavior?! Then you might as well not have the thing. Just remove it entirely.
- taf2 3y agoI really like: [diff] colormoved = "default" colormovedws = "allow-indentation-change" That is great for understanding the actual code changes wish it was on by default.
- epage 3y ago> Within a feature branch, not every commit necessary passes the tests (or even builds), and that is a useful property! Here’s some ways this can be exploited: > When fixing a bug, add a failing test first, as a separate commit. That way it becomes easy to verify for anyone that the test indeed fails without the follow up fix. As a reviewer, I want the first commit to be passing and then the bugfix commit update the test to still pass - Makes change of behavior obvious to reviewer - Assumeing all commits are tested, ensures the test is testing the right thing
- hmeh 3y ago> However, in a typical project, landing a trivial change is slow. How long would it take you to fix it's/its typo in a comment? Probably 30 seconds to push the actual change, 30 minutes to get the CI results, and 3 hours for a review roundtrip. It doesn’t have to be like this. This is a choice. There’s no reason a team cannot work towards a world where they can push a small fix to master directly after running their 5 second test suite on their local machine. We are used to squalor, but it’s not necessary. It’s the result of a series of choices. There are other ways to do our work that maintain continuity of productivity. Old projects can feel like new projects. Any time someone writes something like this, they are normalizing slights against our fellow developers. They may not recognize it as such because it’s all they’ve ever known, but we should (collectively) know that better is possible and strive for it.
- hmeh 3y agoWhat does it say about us as an industry when someone gets downvoted for saying our work lives can be better? I always picture lobsters pulling each other back into boiling water. It’s sick, honestly.
- bornfreddy 3y ago> Instead, adjust the asserts such that they lock down the current (wrong) behavior, and add a clear // TODO: comment explaining what would be the correct result. This is awful advice. The test is there to prevent regression. Why would you change it to not only allow the wrong behavior, but to enforce it? Either fix the test or decide that you won't, in which case remove it. If neither option appeals to you then mark it as optional and let CI succeed with a warning. But changing the test to lock the wrong behavior? No, just no.
- ender7 3y ago> A better approach is to optimistically merge most changes as soon as not-rocket-science allows it, and then later review the code in situ, in the main branch. And instead of adding comments in web ui, just changint the code in-place, sending a new PR ccing the original author. This may work for small projects or open source projects that generally receive high-quality PRs, but this sounds truly infeasible for large teams or organizations. There are a couple reasons why the code review process occurs before merging. First, it helps keep the canonical version of the codebase in a "correct" state. This is, ironically, an extension of the "not rocket science" principle that the article mentions. Without this invariant, any checkout of the codebase might contain what is essentially "WIP" code that will waste other engineers' time if they have to interact with it. Second, the social pressure of blocking someone else's work is an important and necessary force for motivating code review. The idea that folks will go back and review code that's already been merged is akin to "will add tests in a followup PR". It's a pleasant lie we tell ourselves, but it rarely comes true. Relatedly, the article also seems to suggest that the code REVIEWER should be providing the changes necessary after code review. This is problematic for so many reasons: first, it places an even higher burden on code reviewers' time which no one wants; second, it encourages poor code hygiene if someone else is just going to fix up your crappy work later on; third, it robs junior engineers of what is perhaps their most valuable learning experience: getting feedback from more experienced engineers and acting upon it. (there are other issues here; the general thesis of the article leans heavily towards making code easier to _write_ rather than _read_, which again I think it just not appropriate for a codebase with a significant lifetime or number of contributors)
- RupaGupta1289 3y ago[dead]