6 ms·

I am a firm believer that a commit should rarely be more than a couple minutes of work. When I go to git-blame, I want the commit message to be the why of this

• simonw 4y ago
  Is this example too large or about the right size for you? https://github.com/simonw/sqlite-utils/commit/ab8d4aad0c42f905640981f6f24bc1e37205ae62 https://github.com/simonw/sqlite-utils/commit/ab8d4aad0c42f9...
  • donatj 4y ago
    That's probably fine, honestly. I'd maybe take a little more context in the message, but I certainly write messages like that myself. It's always later when I'm looking into problems that I want more context. I spend a lot of time hunting down problems. Half the time I caused them. I know tone/angle of commit messages is an entirely separate bag of cats that people have strong opinions about, but I find the tone strange. I generally fond of commits that state what the commit does. Something along the lines of "Adds optional table support"
    • js2 4y ago
      > Adds optional table support Ahem, "Add optional table support" Because: imperative mood. :-) I was an early git contributor and I've modeled my commits from the git project ever since.
      • simonw 4y ago
        I can never remember which tense is recommended for git commits - I keep meaning to figure that out and try to make a habit of if.
        • mklein994 4y ago
          It's called the Imperative Mood[0]. I like to think of it as though I'm completing this statement: "If you apply this patch, this commit will [Do The Thing™]". Here's a few examples: If I were to apply this patch, this commit will… - Simplify FooWidget - Add optional table support - Remove trailing whitespace - Update fizzbuzz to 1.2.0 - Merge branch 'feature/ABC-123-foobar' - Revert "Add foo to bar" [0]: https://en.wikipedia.org/wiki/Imperative_mood https://en.wikipedia.org/wiki/Imperative_mood
          • simonw 4y ago
            Thanks, that's a really clear explanation. I'm going to try that out and see if it sticks for me.
  • fragmede 4y ago
    The commit message could stand to be the tiniest bit longer, instead of having to click through to find out what #299 is.
  • js2 4y ago
    Here's what I would have written. This took me reading the code and clicking through to #299 to figure out and could have been in the commit message: Add optional tables argument to schema command The schema of every table in the DB is shown by default. Add optional tables argument to restrict which tables are shown. Closes #299
    • thunky 4y ago
      The issue serves no purpose for me, so here's a shorter version: Add `tables` argument to schema command Optionally restricts which tables are shown
      • js2 4y ago
        I think the issue was originally an outside feature request and adding "Closes #xxx" is there just to link the commit to the issue and automatically close it.
        • thunky 4y ago
          It wasn't, actually. simonw created the issue himself and self-closed it 8 minutes later. I had a conversation with simonw yesterday about why I personally view this as an anti-pattern: https://news.ycombinator.com/item?id=33388369 https://news.ycombinator.com/item?id=33388369
          • seadan83 4y ago
            I really love this specific thread, thank you all for the thoughtful commentary. An experience that is really burned into me was working on a team that put very little into commit comments or PR descriptions. But, they did have debates and design decision conversations within PR review comments. Hence the context was in the PR comments. One fine day, I was looking for the background behind some code while tracking down a problem. After identifying about 7 commits (with pretty basic/useless messages, and no PR link!), I then had to find the corresponding PRs based on timestamps, and search the PR history for PRs merged around those timestamps. From there, i had to review the conversations, some very lengthy back and forth conversation threads within the PRs to happily finally find a conversation that had the desired background. Fhe background was something like "why did you do this? X reason. Response: ok" Given that was the main thrust of the update, had that context been in the commit, it would have saved a solid hour. From this experience and others tracking down N jira items to gain context into N possible commits, my rule of thumb is that it scales poorly if you have to go to an external tool to gain context. The above experience meant tracking down the context of any one change was generally a 15 to 60 minute exercise! No chance to grep logs, just a lot of timestamps searches and scrolling through PR conversations. My preferred approach is that a first commit is "for the history." Any further commits are for the reviewer and when squashed those commit comment will generally be removed. So things like "address concern for X" and "formatting" makes sense for a reviewer coming back to an already reviewed PR, but is deleted fir someone that will look at it squashed. The idea being the squashed commit is on master, at that point nobody cares about a "formatting" commit that is one line in a series of 10 other similar commits (that person is tracking down a bug or trying to figure out non-obvious code, they do not care about renamed variables being apart from other tweaks and formatting) Hence, there is different granularity in commits, but eventually the PR and commit granularity become the same. I thus treat PRs as ephemeral with all of the initial PR text auto-generated from the commits. So, any long work to fill out a PR description instead is captured in the commit message which then autopopulates the PR description leaving nothing extra in the PR. Then any review comments are preferably not addressed directly in the PR, but with a preference to update the code for clarity or with needed documentation. Once that is done, the response to the reviewer is a "updated in git Sha X, WDYT?" Hence, the PR is for the review process, not documentation. Of course, some updates need several commits to be grok-able in review. In this case PRs are broken up, a leading PR might be 10 refactoring commits which is then squashed together with title "refactoring X", and the lusg of commits then automatically squashed into a list of reactors done in that commit. Thus avoids a very long string of commits in the history. Then the 11th commit that is the actual bug fix or functionality changes is submitted and then exists in the history as it's own commit. So the history would be like "simplify login moodule\n[list of refactors", and commit two: "fix empty username login problem\n[description of why this is a fix]"
• strictfp 4y ago
  I've been following your school of thought as well as the one proposed by the author. I would argue that your model is a lot freer and will lead to better code quality in the long run, since you're not feeling so restricted when working. Meticulously crafting commits feel like one of those "good in theory" approaches that feels good but hurts you more than you realize. They're good for workflows where you move commits between lots of (release) branches, sure, but if you're doing trunk-based development I'm going to call it a bit formalistic.
• ilammy 4y ago
  > I care entirely about the context of the developers mind when he committed that specific line of code. In squash workflow, the PR is the context and the unit of change. It’s shoehorned onto git only because of GitHub. In some way it’s a self-fulfilling prophecy: if you disregard individual patches then individual patches will be disregarded. It could also be attributed to git being hard to use, with all these “commits” and “patches”. So some developers treat Ctrl-S as the “commit” hotkey, putting whatever state of the codebase into commits, keep stringing them. Then in the end just ship it for review as is, since it’s the path of least resistance, never even expecting anyone to review individual patches, because as far as they are concerned they are not sending in patches, they are sending the PR. Then maintainers are faced with these PRs, and then they have a choice: either enforce the quality of “commits”, or review the PR as a whole and disregard the commits. The choice often falls for the latter, because the former does not provide any practical benefits for the process (rather than theoretical “but we’ll have individual commits when 5 years later someone blames them”), while the latter does provide practical benefits of not alienating developers with subpar commit discipline.
  • u801e 4y ago
    > In squash workflow, the PR is the context and the unit of change. It’s shoehorned onto git only because of GitHub. This could be useful if a PR is essentially changing only one thing like a commit should, but since PRs usually cover a feature or a fix for a ticket, these frequently involve doing multiple things to implement the feature or fix the bug. > the [option of enforcing the quality of commits in a PR] does not provide any practical benefits for the process (rather than theoretical “but we’ll have individual commits when 5 years later someone blames them”), Your parenthetical clause is not theoretical. It's basically a thing I have to deal with on a daily basis when I'm fixing bugs or removing/adding features in multiple code bases that were written by people years ago who no longer work there. I can't really make use of git blame because the commit messages are non-informative, the diffs are just work in progress saves, and the linked PRs just have looks good to me comments and no description. But for code bases where commit quality was enforced, git blame actually becomes useful and allows me to see what was done and why it was done at the time it was written.
• js2 4y ago
  I also work this way and advocate against squash merging or putting all the context into the PR. A big reason for me is that I've built up a lot of context in my head when writing code. When I make the commit, that context is fresh in my mind and flows easily into the commit message. But say instead I make a half-dozen commits with just single sentence descriptions. At the end of my work day I push the commits to a new branch and open a PR, and start writing the PR description. What was I thinking 5 hours ago when I wrote that first commit? I've forgotten. I've lost context. So I always try to commit and document as I go. At that point I don't spend a lot of time getting the commit messages perfect, but I make sure they at least have all my thoughts. Before I push up the commits, I'll use "git rebase -i" and "reword" to edit the messages (typos, reformatting the text, maybe adding additional thoughts as I look at the diff). Then I open the PR and at that point, maybe I'll add additional context but usually my PRs are just an abstract and I ask reviewers to look at the commits themselves. It drives me crazy how hostile GitHub is to reviewing individual commits. Many teams end up with squash-merges though because frankly: (a) developers don't know how to use git beyond committing and pushing so; (b) PR feedback is addressed by adding more commits and pushing them up, as opposed to amending the original commits; combined with; (c) GitHub only grudgingly accommodating an amend+force push workflow. Frankly Gerrit is a better-designed review tool, but it's not widely used.
  • simonw 4y ago
    This is why my workflow is so issue heavy: I use issue comments to constantly capture my context as I'm working, rather than using small commits. I sometimes even run "git diff" and copy the result into an issue comment to record a potential implementation route that I'm not fully convinced of just yet.
    • davnn 4y ago
      If most of the workflow and documentation is not based on git, but instead on issues and PRs, is there an easy way to swap the underlying platform, i.e. GitHub to Gitlab?
      • simonw 4y ago
• deleted 4y ago
  [deleted]
• seba_dos1 4y ago
  > In the rare case you do actually care what PR a commit is from, GitHub's search will tell you.[1]. There's no need to put that information in a commit at all. Even better - if you work with linear history (like most GitHub projects do I guess), you can always require merges to be fast-forwardable, but create a merge commit anyway. This way you retain all the individual commits and they're being grouped together by pull requests, so you can easily filter the details out with flags like `--first-parent` when you're not interested in them. Squashing PRs truly is absolutely useless.
  • GauntletWizard 4y ago
    The tooling just isn't there, though, especially if you want to do something like signing. I want to have my whole merge/pull request history in one subtree, while having the 1st subtree maintain a clear linear history with MR/PR Numbers in each merge commit, and also have that one be signed by the committer. It's easy to represent. It's a little harder to get the forges to do it right.
  • howscrewedami 4y ago
    > Squashing PRs truly is absolutely useless That is quite an absolute statement. Some people just have a personal preference for having git trees arranged in a certain way. If squashing can help with that, then it serves a purpose.
    • seba_dos1 4y ago
      Squashing is an essential tool for everyday git use. Squashing pull requests wholesale at merge time as the default merging strategy? It only hurts. It's a clear case of using the wrong tool only because you haven't learned a better one yet. Maybe it is "quite an absolute statement" - but I have never seen a valid justification for its use.
      • shawabawa3 4y ago
        Have you ever worked at a company where every engineer has in depth knowledge of git? In my experience 90% of software engineers get as far as pull, push, commit, maybe merge. Rebase and amend if you're very lucky Squash merges make git easier to use for 90% of engineers and therefore unless you have significantly above average git knowledge in your company it makes sense as a default strategy
• lamontcg 4y ago
  > I was recently hunting down the reason for a single ! casting a string to a bool. The blame on the file lead to a large commit with details about the project, but nothing about why that specific line of code was changed. > Was it an accident? Did it serve an actual purpose? I might never know. You're never going to get that level of detail. Particularly if it was an accident or done without really thinking about it--the original coder will never document that because they were literally not thinking about it. New commit rule: always document what you weren't considering when you wrote the commit.
  • Izkata 4y ago
    > > Was it an accident? Did it serve an actual purpose? I might never know. > You're never going to get that level of detail. I have! I once found a bug that was caused by a typo in a linting commit, something obviously unintentional. If it was squashed it would have taken a lot longer to figure that out. I also recently found issues in a common python 2 and 3 library where the code was broken in py2to3 and first-run-of-black commits. More places where it was obvious how and why the code was wrong because the formatting was a separate commit from the feature.
• majormajor 4y ago
  Do you constantly "rewind"? E.g. getting rid of stuff like "try if this works" type commit messages, after you've finished doing whatever experimentation you need to do? The idea that I can commit every few minutes with commit messages that will make sense to you months later strikes me as incredibly unlikely, short of "write the code once to figure out how to do it, then throw it again and write it again to have a good chain of commits." And it's not worth that much - especially because the bit that's relevant when debugging may not be the bit that was relevant to me when writing the code. Non-obvious things like that string/bool thing? I use comments for that. Yes, comments can get outdated, blah blah blah, but commit messages can also be unhelpful. What if that cast wasn't what was on their mind when writing the message anyway? I would argue that if your PR was a thousand lines it's too damn big anyway. Figure out how to do multiple PRs, your reviewers will thank you. But I want commits that are cohesive, compilable, functional pieces. Not just path-dependent detritus of the dev process.
  • seba_dos1 4y ago
    > Do you constantly "rewind"? E.g. getting rid of stuff like "try if this works" type commit messages, after you've finished doing whatever experimentation you need to do? Yes. Being able to effortlessly do just that is the main value provided by git.
  • imaltont 4y ago
    > Do you constantly "rewind"? E.g. getting rid of stuff like "try if this works" type commit messages, after you've finished doing whatever experimentation you need to do? Not constantly, but will clean up the local/personal branch history before creating a PR with a rebase. Change some messages, maybe change up where some of the code is committed or even change the order if that makes more sense than the way it actually happened. It's a pretty quick and easy task to do when you get use to it.
• raydiatian 4y ago
  This makes sense for bugs, which every feature secretly is
• MaulingMonkey 4y ago
  > I am a firm believer that a commit should rarely be more than a couple minutes of work. While I'm a proponent of small commits, this is overstating things IMO. I work on codebases that frequently take a couple of minutes to perform an incremental link, that put commits through dozens of CI hours of (useful!) integration testing, chase down C++ heisenbugs that take weeks to hunt down the root cause motivating a 1-line change with a 30-line commit message. On the right codebase, I'm more than happy to throw a single file "fix whitespace" commit at CI without even locally compiling it, confident that CI will catch any problems, because it'll help untangle the diffs of future impending commits and make them easier to review. On the same codebase, I might make a commit touching 1000 files, mechanically switching code from an old deprecated API to a new one. If individual changes are "high risk" and potentially worthy of bisecting, proper review, etc. then I might split the commit up. If the individual changes are "low risk", and basically guaranteed to work if it compiles, then splitting up the commit is just adding noise - better to make it a complete and atomic commit than a micro commit, even if it might involve hours of work (e.g. adding a categorization enumerand to allocations or logging.) The real risk would be something like changing allocation patterns breaking some uncaught edge case workflow by going out-of-memory, and the complete commit will be easier to bisect, track, and revert than dozens of scattered micro commits that individually made the crash only slightly more likely, but on the whole made the crash certain. Unless I have a huge slog of entirely mechanical commits, I probably top out at maybe 20 commits on average over a flowing, coding-focused, 8 hour workday, doing work which is straightforward and easy to carve off completed atoms of work into their own commits. Which is what - 24 minutes per commit? Bit more than a couple. And I'm an outlier compared to coworkers over multiple game companies, who trend towards less frequent, girthier commits, even if they appreciate my approach. > I was recently hunting down the reason for a single ! casting a string to a bool. The blame on the file lead to a large commit with details about the project, but nothing about why that specific line of code was changed. > > Was it an accident? Did it serve an actual purpose? I might never know. I have asked myself the same questions of plenty of tiny commits, sometimes even my own. Tiny commits don't actually solve this problem. Thorough review, clear code, and proper documentation can help, but if nothing ever slipped through the cracks, we wouldn't have bugs in the first place. And sometimes you discover it's actually a bug canceling out another bug, and that even the author of the commit didn't actually grok what was going on, even if they tricked themselves into thinking they did at the time. --- My own rule of thumb: carve off small/atomic/freestanding "complete" changes into their own commits for easier review of both it and your future commit whenever reasonably possible, if only because the change is broken up. But even this has caveats - reviewers traumatized by past coworkers adding "code for the sake of code" might push back on these for lack of concrete use cases, and it may be easier to bend to their whims than to spend the time and political capital to fight 'em and bring them around to your way of thinking. "Reasonably possible" means "stop if it hurts."
• int0x2e 4y ago
  In my experience, this only works if you have a small team of great engineers. Once you have 50+ people of various skill levels making contributions to the codebase, you'll start seeing your git history littered with variants of "fix", "fix 2", "fix broken CI test" and on and on... For me, the PR is the context I can use when trying to find an issue. Sifting through many hundreds of commits per day is painful. Sifting through tens of PRs per day is not great, but is much more manageable...
  • seba_dos1 4y ago
    > you'll start seeing your git history littered with variants of "fix", "fix 2", "fix broken CI test" and on and on... Should such commits even pass through the review in the first place?
    • m000 4y ago
      If the team hasn't agreed to not allow this kind of messages, they will get through. And if you're the only one asking for better messages, your colleagues will start seeing you as pushy, and the requested changes as personal favors. The problem is that if good commit practices aren't established very early in the project, they're unlikely to be adopted later when the team grows (assuming that the whole team has a say on the decision).
• DeathArrow 4y ago
  I see the purpose of code versioning in being able to save the code fast, being able to reverse code changes fast and doing code merged with ease. If some philosophy stays against someone's productivity I don't believe it is a good thing. I would let developers commit in every state they find easy, provided the code builds and the commit comment describes what is being committed. I think it is the PR which we can push more demands on but not commits. I, for example, dislike having to track any small change I have made and commit it. I like committing larger parts which contain a functionality.
  • imaltont 4y ago
    > I, for example, dislike having to track any small change I have made and commit it. I like committing larger parts which contain a functionality. This is pretty much the suggested way by the creator of git too. A commit should contain the necessary changes for one bit of functionality/bug fix. This imo makes the history pretty neat and tidy, while at the same time making it easy to search through with blame/bisect whenever you need to. Giant commits (multiple functionality/whole project/extension squashed into one) makes both of those hard to use and in some cases pretty much useless outside of finding who did it and hope they still work at the company and remember their state of mind when they did the change.
• bennysonething 4y ago
  Git squash into main works well for my team. Lots a commits, often undoing then redoing things really doesnt add anything for me. In fact I find it harder to understand lots of small commits because the overall goal / change change is so fragmented. Also it's it's easier to revert a single squash.
• gorgoiler 4y ago
  You wouldn’t publish the chapters of your book with a complete edit history and all of the margin notes and email discussions you had with your editor. At some point you have to draw a line under your creative process and present a finished piece of work that stands up on its own. That means one diff with a meaningful commit message. It might seem like a reasonable point that you want the unsquashed commit history in order to see into the mind of the developer. In practice, anything of consequence mentioned in those commits should be in the squashed message. I don’t need to see your thought process as you get something wrong three times then spend another three commits fighting with the linter all before you present your final thesis. I just need to see what you’ve finally settled on. (See also: changes to a code base that land and are then immediately changed again. Developers like this put up code too soon and too persuasively. Try to get them to slow down.) PS: Most projects I work on are mature code bases with between tens and thousands of developers. Most of the changes are iterative, adding features on the long march to revenue. There’s an exception to this process — which the OP alludes to — where large new features land as blocks of hundreds or thousands of lines of code. When that happens, if the author spent a long time in modelling hell trying to find the best way to lay out their code, then it’s going to have a horrible commit history. If there’s anything they can do to land it in stages then they should — including rebuilding their history with pretend commits as if they have one clear idea after another. That is hard to simulate, so it’s often easier to break the code up on the orthogonal axis to time: space. With version controlled code, the dimensions of space are modules, and if a large change is broken into clearly defined modules then that provides the same benefit as a series of changes broken into clearly defined ideas / diffs / patches / PRs.
  • trashburger 4y ago
    > I don’t need to see your thought process as you get something wrong three times then spend another three commits fighting with the linter all before you present your final thesis. I just need to see what you’ve finally settled on. I don't think parent is advocating for a commit to stay the same before a merge. You can simply clean up those WIP commits via rebase + fixups and put them in a coherent order and structure (i.e. make them atomic as the article mentions). This and rebase-merge workflows make git history a delight to work with.
• bmitc 4y ago
  If you keep your commits so small, what do you put as the commit message? Also, this showcases why Git is a terrible system. It contains no semantic information in it and is just a dumb line and text change keeper.
• darekkay 4y ago
  > Blames are easy. There's zero need to try to keep your tree "looking nice". I don't care about how the tree looks like. But I do care about keeping the number of commits low _because_ it makes blames easier. > In the future when I'm hunting down a problem I almost never care about what feature or fix this was a part of - e.g. the PR - I care entirely about the context of the developers mind when he committed that specific line of code. I've mostly made the opposite experience. Many developers don't have the knowledge or confidence to use "amend" etc. consciously, so it leads to a mass of useless "fix error", "forgot something", "add tests" and "implement PR reviews" commits. This makes blame more difficult. I also agree: sometimes a squashed commit message doesn't help me understand the code of line I'm blaming. But not having squashed the commit mostly wouldn't help me anyway. Instead, I try to avoid this problem beforehand, by making sure that the PR itself contains all the information (via PR description and/or code comments). Recently I've reviewed a "Fix scrollbar" PR. The "what" is clear. The "why" (= "it's broken") _sounds_ sufficient, but I was missing information like "how exactly is it broken" and "why does this change fix it". That's the kind of stuff I might blame in a year from now, and now it's part of the Git history.
  • b3morales 4y ago
    > Many developers don't have the knowledge or confidence to use "amend" etc. consciously, The secondary problem here is that GitHub (just to throw another complaint on the pile about GitHub) makes a dog's breakfast of tracking PR comments when things have changed. Amending and force pushing only seems to make it worse, so even though it should be the better workflow, it causes friction in a different place.