4 ms·

The commit messages are terrible. In my opinion, conventional commit messages [0] should be used for a clean commit history. [0] https://www.conventionalcommit

The commit messages are terrible. In my opinion, conventional commit messages [0] should be used for a clean commit history. [0] https://www.conventionalcommits.org/en/v1.0.0/ https://www.conventionalcommits.org/en/v1.0.0/

theyinwhy 3y ago
Something more descriptive than "Grrr" would be nice I guess.
Kwpolska 3y ago
CPython seems to use squash merges, which means only one commit will end up on the main branch after merging this PR. The history on branches is irrelevant and can be completely messy, full of merges and other experiments; the main branch has one commit per actual feature/change. And eh, conventional commits seem like pointless bureaucracy to me.
- hnfong 3y ago
  With only +1,722 lines added, even if the commits were eventually squashed upon landing, I'd consider it good etiquette to tidy up changes to maybe a handful of logical commits instead of pushing 404 raw commits. Or maybe it's another weird pun on 404 Not Found? I can't tell by now...
  - CapsAdmin 3y ago
    The end result of doing this is good, but I find it really difficult to cleanly do this before I have something that's 100% complete. I don't code linearly like "first I need feature A, then I code feature B which is needed for feature C, and so on" It's usually a bit all over the place and it's not clear what depends on what until I start reaching the end. So to do this properly I'd need to spend a day or two rewriting or making a new branch that cleanly adds everything in order. Hopefully in a way that doesn't leave master in a broken state when reverting tail commits. In addition, when doing multiple pull requests for a single high level feature, you might get some comments about pull request "C" that would require changes in pull request "A"
    - cardanome 3y ago
      How the hell is someone supposed to review your pull request if you don't take the time to clean it up? I normally go through every single individual commit when reviewing something and find the commit messages extremely helpful to understand what some change is supposed to do. Yes, cleaning up your commits takes some time butt I don't see an alternative if you don't work alone and want your code to stay maintainable.
      - Kwpolska 3y ago
        I review the pull request as a whole, looking at the diff between main and the latest commit on the branch (i.e. what GitHub/etc show by default). Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it, but you’re cluttering your mind with it. During re-reviews, I usually look at the diff between the last commit I reviewed and the newest commit.
        cardanome 3y ago
        > Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it If the commit is wrong, it shouldn't be there. I expect every commit in a Pull Request to be functional on its own or I am not going to approve it in the first place. Git has tools to rewrite your commit history and you should use them. The whole point is that I should be able to revert individual commits without code breaking. At least that is the ideal. A clean version history matters a lot of the people maintaining your code down the line.
        wirrbel 3y ago
        Yes and no. I think what you say is definitely the goal for day-to-day contributions. However, there are changes to a code base that are more "Manhattan project" in nature where not all changes can be neatly packaged into their own commits, OR the PR author kind of needs to re-do their coding on a clean room branch. Which is significant overhead. Being able to undo a commit is a means to an end, not the ultimative goal.
- da39a3ee 3y ago
  Being honest though, the guy's commit messages influence my prior on how reliable and well-designed his code will be.
  - japanman185 3y ago
    “My prior”. Give it a rest.
    - da39a3ee 3y ago
      Just saying that if I were working with this person it wouldn't make me think highly of him, and in my fairly extensive experience I can report that there's a strong correlation between silly commit messages and not great code. I didn't mean to imply that I was qualified or skilled enough to evaluate the JIT compiler for Python. I think I missed what your point was?
      - austinjp 3y ago
        I understood the point to be about using accessible language rather than jargon.
        da39a3ee 3y ago
        You think "my prior" is jargon? I'm not sure what to say to you then. You realize Bayesian thinking in statistics has been around since the 1950s if not 1930s? https://en.wikipedia.org/wiki/History_of_statistics#Bayesian_statistics https://en.wikipedia.org/wiki/History_of_statistics#Bayesian...
        japanman185 3y ago
        [dead]
        austinjp 3y ago
        Bear in mind I'm interpreting the previous comment. I'm familiar with many things Bayesian thanks :) However, I wouldn't assume everyone else is, even here. Are you familiar with the meaning(s) of the word jargon? https://dictionary.cambridge.org/dictionary/english/jargon https://dictionary.cambridge.org/dictionary/english/jargon https://en.wikipedia.org/wiki/Jargon https://en.wikipedia.org/wiki/Jargon Just to provide an example, your previous comment could have been written something like this: "Being honest though, the guy's commit messages changed my preconceptions about how reliable and well-designed his code will be." No knowledge of statistics required, Bayesian or otherwise.
- meowface 3y ago
  Yes, but for such a significant contribution to a huge project it's good etiquette to squash on your own before submitting the PR. (Not that it means the PR shouldn't be reviewed and accepted.) Honestly, I frequently do this for my own personal projects since I'm lazy, but if I'm submitting something to a big open source project I always clean it up first.
ltbarcly3 3y ago
I question the value of commit messages at all. Sure, at some level you need a summary of what a change is trying to do, but we have that at 5 levels now and they are completely redundant. Generally there is a ticket in some system for tracking changes, whether it's Jira or Github itself or some other system. Then you have a PR/MR that is attached to a branch which you are trying to have merged. Then there are the commit messages themselves. These are all completely redundant to each other, and nobody in their right mind should want all of these at the same time. It's too many places to look for the same exact information, there's no reason to maintain it in more than one place. Some truly awful standard for formatting commit messages, how to do something that has at best dubious value to begin with, is a fantastic way to give the appearance of work without the need for skill or ability or spending time trying to get useful work done, a true boon to incompetents and hangers on. It's also a great way to snipe someone's amazing work and put yourself in a position to critique them with 1/1000th of the effort of accomplishing something useful.
- wavemode 3y ago
  I think Jira messages are generally written from a product perspective ("here's what we want") whereas PR's are written from an implementation perspective ("here's how we did it"). And then the PR description ought to just become the squashed commit message (at least that's how my current company does it).
- etra0 3y ago
  I value the commit messages in the context where you develop a tool that has to be run widely. In particular, I had experience with Wine. Having useful commit messages allows you to do bisects and trace down regressions with more ease than cross-checking messages with some external ticket system, and when you have a lot of people contributing to a project it's easier to see what they're doing when they try to do a patch. I also believe though, that it is good practice to help your colleagues when they do need to find an issue in a project where a lot of different people can work on.
  - ltbarcly3 3y ago
    This would boil down to merge level messages. Not all projects squash commits, so what you actually care about here are the mr/pr level messages, which might be approximated by the commit message on the merge commit, but actually probably will just say merge {branchname}. When you bisect with parent=0 you only see merge commits or ff directly against main branch. Putting what im saying another way, in a project with pull requests, commit messages are redundant with the text typed into the pr and the comments on same. We should just carbon copy those onto the merge commit and forget per commit messages.
seba_dos1 3y ago
They are really bad and I would feel bad if I presented something like that to someone for a review (not just messages - most of those commits shouldn't exist at all). That said, the MR in question is marked as a draft, so anything goes at that point. Also, Conventional Commits are mostly pointless. Linux-style commit message conventions are enough.

theyinwhy 3y ago

Something more descriptive than "Grrr" would be nice I guess.

Kwpolska 3y ago

CPython seems to use squash merges, which means only one commit will end up on the main branch after merging this PR. The history on branches is irrelevant and can be completely messy, full of merges and other experiments; the main branch has one commit per actual feature/change. And eh, conventional commits seem like pointless bureaucracy to me.

hnfong 3y ago
With only +1,722 lines added, even if the commits were eventually squashed upon landing, I'd consider it good etiquette to tidy up changes to maybe a handful of logical commits instead of pushing 404 raw commits. Or maybe it's another weird pun on 404 Not Found? I can't tell by now...
- CapsAdmin 3y ago
  The end result of doing this is good, but I find it really difficult to cleanly do this before I have something that's 100% complete. I don't code linearly like "first I need feature A, then I code feature B which is needed for feature C, and so on" It's usually a bit all over the place and it's not clear what depends on what until I start reaching the end. So to do this properly I'd need to spend a day or two rewriting or making a new branch that cleanly adds everything in order. Hopefully in a way that doesn't leave master in a broken state when reverting tail commits. In addition, when doing multiple pull requests for a single high level feature, you might get some comments about pull request "C" that would require changes in pull request "A"
  - cardanome 3y ago
    How the hell is someone supposed to review your pull request if you don't take the time to clean it up? I normally go through every single individual commit when reviewing something and find the commit messages extremely helpful to understand what some change is supposed to do. Yes, cleaning up your commits takes some time butt I don't see an alternative if you don't work alone and want your code to stay maintainable.
    - Kwpolska 3y ago
      I review the pull request as a whole, looking at the diff between main and the latest commit on the branch (i.e. what GitHub/etc show by default). Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it, but you’re cluttering your mind with it. During re-reviews, I usually look at the diff between the last commit I reviewed and the newest commit.
      - cardanome 3y ago
        > Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it If the commit is wrong, it shouldn't be there. I expect every commit in a Pull Request to be functional on its own or I am not going to approve it in the first place. Git has tools to rewrite your commit history and you should use them. The whole point is that I should be able to revert individual commits without code breaking. At least that is the ideal. A clean version history matters a lot of the people maintaining your code down the line.
        wirrbel 3y ago
        Yes and no. I think what you say is definitely the goal for day-to-day contributions. However, there are changes to a code base that are more "Manhattan project" in nature where not all changes can be neatly packaged into their own commits, OR the PR author kind of needs to re-do their coding on a clean room branch. Which is significant overhead. Being able to undo a commit is a means to an end, not the ultimative goal.
da39a3ee 3y ago
Being honest though, the guy's commit messages influence my prior on how reliable and well-designed his code will be.
- japanman185 3y ago
  “My prior”. Give it a rest.
  - da39a3ee 3y ago
    Just saying that if I were working with this person it wouldn't make me think highly of him, and in my fairly extensive experience I can report that there's a strong correlation between silly commit messages and not great code. I didn't mean to imply that I was qualified or skilled enough to evaluate the JIT compiler for Python. I think I missed what your point was?
    - austinjp 3y ago
      I understood the point to be about using accessible language rather than jargon.
      - da39a3ee 3y ago
        You think "my prior" is jargon? I'm not sure what to say to you then. You realize Bayesian thinking in statistics has been around since the 1950s if not 1930s? https://en.wikipedia.org/wiki/History_of_statistics#Bayesian_statistics https://en.wikipedia.org/wiki/History_of_statistics#Bayesian...
        japanman185 3y ago
        [dead]
        austinjp 3y ago
        Bear in mind I'm interpreting the previous comment. I'm familiar with many things Bayesian thanks :) However, I wouldn't assume everyone else is, even here. Are you familiar with the meaning(s) of the word jargon? https://dictionary.cambridge.org/dictionary/english/jargon https://dictionary.cambridge.org/dictionary/english/jargon https://en.wikipedia.org/wiki/Jargon https://en.wikipedia.org/wiki/Jargon Just to provide an example, your previous comment could have been written something like this: "Being honest though, the guy's commit messages changed my preconceptions about how reliable and well-designed his code will be." No knowledge of statistics required, Bayesian or otherwise.
meowface 3y ago
Yes, but for such a significant contribution to a huge project it's good etiquette to squash on your own before submitting the PR. (Not that it means the PR shouldn't be reviewed and accepted.) Honestly, I frequently do this for my own personal projects since I'm lazy, but if I'm submitting something to a big open source project I always clean it up first.

ltbarcly3 3y ago

I question the value of commit messages at all. Sure, at some level you need a summary of what a change is trying to do, but we have that at 5 levels now and they are completely redundant. Generally there is a ticket in some system for tracking changes, whether it's Jira or Github itself or some other system. Then you have a PR/MR that is attached to a branch which you are trying to have merged. Then there are the commit messages themselves. These are all completely redundant to each other, and nobody in their right mind should want all of these at the same time. It's too many places to look for the same exact information, there's no reason to maintain it in more than one place. Some truly awful standard for formatting commit messages, how to do something that has at best dubious value to begin with, is a fantastic way to give the appearance of work without the need for skill or ability or spending time trying to get useful work done, a true boon to incompetents and hangers on. It's also a great way to snipe someone's amazing work and put yourself in a position to critique them with 1/1000th of the effort of accomplishing something useful.

wavemode 3y ago
I think Jira messages are generally written from a product perspective ("here's what we want") whereas PR's are written from an implementation perspective ("here's how we did it"). And then the PR description ought to just become the squashed commit message (at least that's how my current company does it).
etra0 3y ago
I value the commit messages in the context where you develop a tool that has to be run widely. In particular, I had experience with Wine. Having useful commit messages allows you to do bisects and trace down regressions with more ease than cross-checking messages with some external ticket system, and when you have a lot of people contributing to a project it's easier to see what they're doing when they try to do a patch. I also believe though, that it is good practice to help your colleagues when they do need to find an issue in a project where a lot of different people can work on.
- ltbarcly3 3y ago
  This would boil down to merge level messages. Not all projects squash commits, so what you actually care about here are the mr/pr level messages, which might be approximated by the commit message on the merge commit, but actually probably will just say merge {branchname}. When you bisect with parent=0 you only see merge commits or ff directly against main branch. Putting what im saying another way, in a project with pull requests, commit messages are redundant with the text typed into the pr and the comments on same. We should just carbon copy those onto the merge commit and forget per commit messages.

seba_dos1 3y ago

They are really bad and I would feel bad if I presented something like that to someone for a review (not just messages - most of those commits shouldn't exist at all). That said, the MR in question is marked as a draft, so anything goes at that point. Also, Conventional Commits are mostly pointless. Linux-style commit message conventions are enough.