9 ms·

This commit in the PR https://github.com/obsproject/obs-studio/pull/5875/commits/860a20696d280402b2c3cb1ed96e4256f9dd223f https://github.com/obsproject/obs-stu

by pilif 5y ago

This commit in the PR https://github.com/obsproject/obs-studio/pull/5875/commits/860a20696d280402b2c3cb1ed96e4256f9dd223f https://github.com/obsproject/obs-studio/pull/5875/commits/8... provides an excellent example of why commit messages matter. This is more than 1000 lines diff to the build system (and thus potentially very dangerous because of possible supply chain attacks) which has the commit message of "CI: Update build scripts and Github actions workflow" Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it, especially when your commit feels unrelated or at most very tangentially related to the objective of your PR.

disintegrator 5y ago
I'm all for well presented PRs/commits but I'm not sure how a good commit message mitigates the risk of a supply chain attack in this case.
- pilif 5y ago
  It might help the reviewer to not flat-out reject the PR because it gives them a fame of mind as to what the intention of the change is.
rkangel 5y ago
> Never say what you ware doing (it's obvious this commit is updating build scripts) DEFINITELY say what you are doing. And then say why. This blog post is a good description of the most prevalent convention in how to write good and useful messages (e.g. this is what the Linux Kernel does and given the history of git there's some flow down): https://cbea.ms/git-commit/ https://cbea.ms/git-commit/ The key thing that means you should start with what is having a short one-line summary of what you're doing so you can look at a simple log and see which commit you're interested in.
- pferde 5y ago
  Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!
  - jacobmischka 5y ago
    Not necessarily. It sounds like all of the things mentioned in the commit message are interconnected (updating build scripts and updating gitignore to remove the new build directories). It would be pointless to break these into separate commits, you can't revert just one without reverting the others. The commit message is lazy, but breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it.
    - capableweb 5y ago
      > breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it Agreed, cargo-culting is always bad! But, without knowing how OBS development team works, sometimes it does make sense to split commits like this, mainly if you have a larger repository where different people are responsible for different areas in the codebase. For example, build-scripts sometimes gets managed by other people than the ones writing some core algorithms, who are also different from the people writing public APIs to be consumed by users. In those cases, it makes reviews a lot easier when you can split out to review different commits to different people, instead of having everyone navigate the same commit. But again, not familiar with the internals nor the development cycle that OBS keeps, so maybe this doesn't make sense in this case.
      - threeseed 5y ago
        As others have mentioned many times here. This commit is based off a branch that was adding support for Apple Silicon. So the entire PR is not going to look clean until that branch is merged first.
        capableweb 5y ago
- Cthulhu_ 5y ago
  Small nit: Maybe not say what YOU are doing, but what the COMMIT, when applied, will do. Git is not a personal work log, it's a source code change log. A commit may be a collaborative effort; it may be a squash of a thousand smaller commits; it may be a merge, it may be generated by a tool (like the dependencies), etc. The commit should summarize what IT does, not what YOU did. In ten years, nobody will care who you are or what you did, but they will care what the code does and why.
  - MrGilbert 5y ago
    That's the main reason we are using present tense in commit messages, not past tense. It's the commit that is applying the changes at that moment.
    - thaumasiotes 5y ago
      > That's the main reason we are using present tense in commit messages, not past tense. As a linguistic note, commit messages (at least, English commit messages) almost always use an infinitive verb and therefore may be considered to have no tense at all, though to argue that infinitives have no tense you do need to draw a distinction between tense and aspect.
      - timmb 5y ago
        I believe it’s the imperative (enable widget) not the infinitive (to enable widget) that is generally used.
        thaumasiotes 5y ago
        > I believe it’s the imperative (enable widget) not the infinitive (to enable widget) You're confusing a bare infinitive with an imperative. Infinitives are often, but not always, marked with to. Commit messages are not commands and are not capable of licensing imperative forms. Here are some examples of bare infinitives in English, with the infinitive form italicized: 1. This organization will help you apply for a loan. 2a. Q: What does he do? 2b. A: Check over the paperwork to be sure everything's right. A commit message is doing exactly the same thing as occurs in example 2b.
- ignoramous 5y ago
  > This blog post is a good description of the most prevalent convention in how to write good and useful messages. See also: https://google.github.io/eng-practices/review/developer/cl-descriptions.html https://google.github.io/eng-practices/review/developer/cl-d... https://microsoft.github.io/code-with-engineering-playbook/code-reviews/pull-requests/ https://microsoft.github.io/code-with-engineering-playbook/c...
- jldugger 5y ago
  > DEFINITELY say what you are doing. And then say why. My favored github PR template has 3 sections: - What does this PR do? - Why is that valuable? - Link to bug tracker.
  - ihaveajob 5y ago
    We have a similar template at work, and since we made it the default (on Bitbucket you can pre-fill it), it's saved a ton of back and forth asking questions: * Description: (What this does, and why) * Ref: (Ticket/task link) * To test: (Specific instructions to make sure things are working as expected) * To deploy: (What specific servers/services to restart, and operations to perform)
- zestyping 5y ago
  Ironically, the referenced article on how to write good git commit messages (https://cbea.ms/git-commit/ https://cbea.ms/git-commit/) gives no "why" for most of its points. It lists 7 rules, of which 2, 3, 4, 5, 6 are simply declared without rationale.
  - pixiemaster 5y ago
    so true. my personal observation: because writing down „what“ is hard. Writing down „why“ is even harder.
ff133 5y ago
Question from a naive kiddo; When the PRs are this big, what are the steps taken to "get it in" the master ? Just blindly accept based on CD/CI jobs, or break it down to walk through every change ? "Ref: supply chain attacks"
- pilif 5y ago
  If I was in the position of having to review that PR, I would probably flat-out reject it and ask for just the minimal changes required. I don't see how adding an additional capture framework backend would require meaningful changes to the build system. With some likelihood, in this case, Apple probably won't have the resources to rework their PR and thus the PR will stay un-merged until somebody of the project has time to merge/reimplement the relevant parts. Which of course is a total shame and I wonder why Apple didn't have "mergeability" in mind when they decided to create their PR. They must have known that "rework the build system" can't be part of a PR adding a capture backend if they wanted any chance of this being merged.
  - spockz 5y ago
    It is not that uncommon to have make changes to a build system in many places in order to support a new architecture. Especially if there was previously only support for a single one.
  - Diskutant 5y ago
    No, the PR is based on another PR [0] which adds Apple Silicon and Universal Binary builds, the mentioned commit is part of that PR. [0] https://github.com/obsproject/obs-studio/pull/5155/commits https://github.com/obsproject/obs-studio/pull/5155/commits
  - madmoose 5y ago
    The Apple developer's commit builds upon another pull request which contains the build system work by OBS developers, the build system changes are not part of Apple's commit.
    - 5y ago
n3_ 5y ago
In this case it looks their work is based on another branch not yet merged (https://github.com/obsproject/obs-studio/pull/5155 https://github.com/obsproject/obs-studio/pull/5155 which adds support for Apple Silicon) - and the commit is from there.
- Applejinx 5y ago
  Looking forward to that: I'm going to be needing to use OBS on Apple Silicon.
  - mikkelam 5y ago
    I honestly thought it already was running natively, it already runs super smooth in rosetta
    - ziml77 5y ago
      Rosetta is incredibly impressive. And I guess it needed to be. The switch to ARM was going to be a hard sell without a fast x86 translation layer. I'm hoping though that devs don't use it as an excuse to never release ARM builds of their software. It would be good to have ARM builds across all the major operating systems.
      - dagmx 5y ago
        I fully expect devs to drag their feet. This happens with every transition in Apple history. It takes pulling the rug out completely before many devs will do it Look at the Python transition, the carbon to cocoa transition, even the original Rosetta transition.
      - PostThisTooFast 5y ago
- krallja 5y ago
  Github can point the PR at that branch and those commits won’t show up anymore.
steerablesafe 5y ago
Just to chip in what should the commit message say: it should express the intent of the change and the reason of the change (latter could be reference to ticket). I don't think expressing how the intended change is achieved is important, that's what the diff is. If it's not clear in the diff how the intended change is achieved, try adding some comments.
michaelt 5y ago
Doesn't the gigantic pull request description [1] already provide ample description of why the changes are being made? [1] https://github.com/obsproject/obs-studio/pull/5875 https://github.com/obsproject/obs-studio/pull/5875
- progbits 5y ago
  None of that is part of the Git repo, so any mirrors won't have the context.
  - madmoose 5y ago
    It's in the commit message: https://github.com/obsproject/obs-studio/pull/5875/commits/551c54ba8440fcdf4cdc221f5d50a30a68b87a7e https://github.com/obsproject/obs-studio/pull/5875/commits/5...
- kzzzznot 5y ago
  That doesn’t get stored in git, you won’t be able to see that information on the clone of the repository you will be developing on locally.
  - gwd 5y ago
    Just to expand on that, I always say there are three audiences for the commit message: 1. The reviewer 2. Someone scanning commit logs for patches of interest 3. Archaeologists Archaeologists are people a year, five years, 10 years down the road who are trying to figure out how the code got to be the way that it is, so they can feel confident changing it. (See also "Chesterton's Fence"[1].) Those people should not have to dig out the conversation from some random PR to figure out what's going on. [1] https://news.ycombinator.com/item?id=30070757 https://news.ycombinator.com/item?id=30070757
    - whimsicalism 5y ago
      > Those people should not have to dig out the conversation from some random PR to figure out what's going on. Not knowing to look to an associated PR thread for a commit would be the equivalent of modern day archaeologists not knowing what "carbon dating" was.
i386 5y ago
Seems pretty fucking clear based on the message to me.
donatj 5y ago
We have a rule internally that a PR can’t be more than 1,000 loc. I personally find a 1,000 loc PR almost unreviewable. It takes me multiple days and a stack of notes, and still almost always ends up with effects I didn’t anticipate. Trying to understand the changes and their full implications is nearly impossible. 100 loc is a better top end imho.
- dlsa 5y ago
  Code reviews are interesting to watch. Especially when the author awkwardly pauses and says, "I'm not sure what this code does".
- coldcode 5y ago
  Last place I worked (big company, not FAANG) I remember a PR for our main app that affected 500 files and occasionally dozens of places in some of them. All the reviews were "LGTM".
  - ascagnel_ 5y ago
    I forget where I first saw it, but there's a joke of "ten lines, ten issues; 1,000 lines, looks good". There's a human limit to how much you can hold in your head when reviewing code.
    - whimsicalism 5y ago
      I think it's a double edged sword - people are also way too nitty for small commits, which only incentivizes behavior like this.
    - thetallstick 5y ago
      Looks like the programmer's version of: "If you owe the bank $100, that's your problem. If you owe the bank $100 million, that's the bank's problem.” https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-100-that-s-your-problem-if https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-...
    - sockpuppet69 5y ago
ricardobeat 5y ago
Note that those commits seem to have been added by an OBS contributor on top of Apple's patch. The fact they touch the entire build system, in a PR that is 100% macOS focused, is much more concerning than the commit messages.
emptybottle 5y ago
Yes, a good commit message should be a few concise paragraphs. The subject describes what is being affected (think easily greppable keywords) Then, then body should go into detail about the contents and rationale for the changes. Use specific terms like “add” or “remove” and avoid subjective descriptions like “prepare” “clean” and “fix” For example this patch says: > Clean up consistency of tabs/spaces in include_directories. Well, what does clean up tabs/spaces actually mean? Saying “remove tabs in favor of spaces” is much clearer.
rlt 5y ago
HN: big tech companies should contribute more to open source! Also HN: no, not like that!
- dimator 5y ago
  There's nothing weird about both of those being true.
fnord77 5y ago
in the description of the PR, there's almost 1000 words of text describing in detail what is being done. I don't see the problem.
Joeri 5y ago
Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it Don’t say what you were doing, say what the commit does. A good commit message completes the sentence “when merged this will …”. Adding a more detailed description on the second line including why things were done is also not bad, but I have a strong opinion that design notes do not belong in the commit log but in code comments or markdown files that are in the repo, so that they can be found more easily and updated.

by pilif 5y ago

disintegrator 5y ago

I'm all for well presented PRs/commits but I'm not sure how a good commit message mitigates the risk of a supply chain attack in this case.

pilif 5y ago
It might help the reviewer to not flat-out reject the PR because it gives them a fame of mind as to what the intention of the change is.

rkangel 5y ago

> Never say what you ware doing (it's obvious this commit is updating build scripts) DEFINITELY say what you are doing. And then say why. This blog post is a good description of the most prevalent convention in how to write good and useful messages (e.g. this is what the Linux Kernel does and given the history of git there's some flow down): https://cbea.ms/git-commit/ https://cbea.ms/git-commit/ The key thing that means you should start with what is having a short one-line summary of what you're doing so you can look at a simple log and see which commit you're interested in.

pferde 5y ago
Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!
- jacobmischka 5y ago
  Not necessarily. It sounds like all of the things mentioned in the commit message are interconnected (updating build scripts and updating gitignore to remove the new build directories). It would be pointless to break these into separate commits, you can't revert just one without reverting the others. The commit message is lazy, but breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it.
  - capableweb 5y ago
    > breaking these into separate commits because of some "also" rule of thumb is just blindly following a rule for the sake of it Agreed, cargo-culting is always bad! But, without knowing how OBS development team works, sometimes it does make sense to split commits like this, mainly if you have a larger repository where different people are responsible for different areas in the codebase. For example, build-scripts sometimes gets managed by other people than the ones writing some core algorithms, who are also different from the people writing public APIs to be consumed by users. In those cases, it makes reviews a lot easier when you can split out to review different commits to different people, instead of having everyone navigate the same commit. But again, not familiar with the internals nor the development cycle that OBS keeps, so maybe this doesn't make sense in this case.
    - threeseed 5y ago
      As others have mentioned many times here. This commit is based off a branch that was adding support for Apple Silicon. So the entire PR is not going to look clean until that branch is merged first.
      - capableweb 5y ago
Cthulhu_ 5y ago
Small nit: Maybe not say what YOU are doing, but what the COMMIT, when applied, will do. Git is not a personal work log, it's a source code change log. A commit may be a collaborative effort; it may be a squash of a thousand smaller commits; it may be a merge, it may be generated by a tool (like the dependencies), etc. The commit should summarize what IT does, not what YOU did. In ten years, nobody will care who you are or what you did, but they will care what the code does and why.
- MrGilbert 5y ago
  That's the main reason we are using present tense in commit messages, not past tense. It's the commit that is applying the changes at that moment.
  - thaumasiotes 5y ago
    > That's the main reason we are using present tense in commit messages, not past tense. As a linguistic note, commit messages (at least, English commit messages) almost always use an infinitive verb and therefore may be considered to have no tense at all, though to argue that infinitives have no tense you do need to draw a distinction between tense and aspect.
    - timmb 5y ago
      I believe it’s the imperative (enable widget) not the infinitive (to enable widget) that is generally used.
      - thaumasiotes 5y ago
        > I believe it’s the imperative (enable widget) not the infinitive (to enable widget) You're confusing a bare infinitive with an imperative. Infinitives are often, but not always, marked with to. Commit messages are not commands and are not capable of licensing imperative forms. Here are some examples of bare infinitives in English, with the infinitive form italicized: 1. This organization will help you apply for a loan. 2a. Q: What does he do? 2b. A: Check over the paperwork to be sure everything's right. A commit message is doing exactly the same thing as occurs in example 2b.
ignoramous 5y ago
> This blog post is a good description of the most prevalent convention in how to write good and useful messages. See also: https://google.github.io/eng-practices/review/developer/cl-descriptions.html https://google.github.io/eng-practices/review/developer/cl-d... https://microsoft.github.io/code-with-engineering-playbook/code-reviews/pull-requests/ https://microsoft.github.io/code-with-engineering-playbook/c...
jldugger 5y ago
> DEFINITELY say what you are doing. And then say why. My favored github PR template has 3 sections: - What does this PR do? - Why is that valuable? - Link to bug tracker.
- ihaveajob 5y ago
  We have a similar template at work, and since we made it the default (on Bitbucket you can pre-fill it), it's saved a ton of back and forth asking questions: * Description: (What this does, and why) * Ref: (Ticket/task link) * To test: (Specific instructions to make sure things are working as expected) * To deploy: (What specific servers/services to restart, and operations to perform)
zestyping 5y ago
Ironically, the referenced article on how to write good git commit messages (https://cbea.ms/git-commit/ https://cbea.ms/git-commit/) gives no "why" for most of its points. It lists 7 rules, of which 2, 3, 4, 5, 6 are simply declared without rationale.
- pixiemaster 5y ago
  so true. my personal observation: because writing down „what“ is hard. Writing down „why“ is even harder.

ff133 5y ago

Question from a naive kiddo; When the PRs are this big, what are the steps taken to "get it in" the master ? Just blindly accept based on CD/CI jobs, or break it down to walk through every change ? "Ref: supply chain attacks"

pilif 5y ago
If I was in the position of having to review that PR, I would probably flat-out reject it and ask for just the minimal changes required. I don't see how adding an additional capture framework backend would require meaningful changes to the build system. With some likelihood, in this case, Apple probably won't have the resources to rework their PR and thus the PR will stay un-merged until somebody of the project has time to merge/reimplement the relevant parts. Which of course is a total shame and I wonder why Apple didn't have "mergeability" in mind when they decided to create their PR. They must have known that "rework the build system" can't be part of a PR adding a capture backend if they wanted any chance of this being merged.
- spockz 5y ago
  It is not that uncommon to have make changes to a build system in many places in order to support a new architecture. Especially if there was previously only support for a single one.
- Diskutant 5y ago
  No, the PR is based on another PR [0] which adds Apple Silicon and Universal Binary builds, the mentioned commit is part of that PR. [0] https://github.com/obsproject/obs-studio/pull/5155/commits https://github.com/obsproject/obs-studio/pull/5155/commits
- madmoose 5y ago
  The Apple developer's commit builds upon another pull request which contains the build system work by OBS developers, the build system changes are not part of Apple's commit.
  - 5y ago

n3_ 5y ago

In this case it looks their work is based on another branch not yet merged (https://github.com/obsproject/obs-studio/pull/5155 https://github.com/obsproject/obs-studio/pull/5155 which adds support for Apple Silicon) - and the commit is from there.

Applejinx 5y ago
Looking forward to that: I'm going to be needing to use OBS on Apple Silicon.
- mikkelam 5y ago
  I honestly thought it already was running natively, it already runs super smooth in rosetta
  - ziml77 5y ago
    Rosetta is incredibly impressive. And I guess it needed to be. The switch to ARM was going to be a hard sell without a fast x86 translation layer. I'm hoping though that devs don't use it as an excuse to never release ARM builds of their software. It would be good to have ARM builds across all the major operating systems.
    - dagmx 5y ago
      I fully expect devs to drag their feet. This happens with every transition in Apple history. It takes pulling the rug out completely before many devs will do it Look at the Python transition, the carbon to cocoa transition, even the original Rosetta transition.
    - PostThisTooFast 5y ago
krallja 5y ago
Github can point the PR at that branch and those commits won’t show up anymore.

steerablesafe 5y ago

Just to chip in what should the commit message say: it should express the intent of the change and the reason of the change (latter could be reference to ticket). I don't think expressing how the intended change is achieved is important, that's what the diff is. If it's not clear in the diff how the intended change is achieved, try adding some comments.

michaelt 5y ago

Doesn't the gigantic pull request description [1] already provide ample description of why the changes are being made? [1] https://github.com/obsproject/obs-studio/pull/5875 https://github.com/obsproject/obs-studio/pull/5875

progbits 5y ago
None of that is part of the Git repo, so any mirrors won't have the context.
- madmoose 5y ago
  It's in the commit message: https://github.com/obsproject/obs-studio/pull/5875/commits/551c54ba8440fcdf4cdc221f5d50a30a68b87a7e https://github.com/obsproject/obs-studio/pull/5875/commits/5...
kzzzznot 5y ago
That doesn’t get stored in git, you won’t be able to see that information on the clone of the repository you will be developing on locally.
- gwd 5y ago
  Just to expand on that, I always say there are three audiences for the commit message: 1. The reviewer 2. Someone scanning commit logs for patches of interest 3. Archaeologists Archaeologists are people a year, five years, 10 years down the road who are trying to figure out how the code got to be the way that it is, so they can feel confident changing it. (See also "Chesterton's Fence"[1].) Those people should not have to dig out the conversation from some random PR to figure out what's going on. [1] https://news.ycombinator.com/item?id=30070757 https://news.ycombinator.com/item?id=30070757
  - whimsicalism 5y ago
    > Those people should not have to dig out the conversation from some random PR to figure out what's going on. Not knowing to look to an associated PR thread for a commit would be the equivalent of modern day archaeologists not knowing what "carbon dating" was.

i386 5y ago

Seems pretty fucking clear based on the message to me.

donatj 5y ago

We have a rule internally that a PR can’t be more than 1,000 loc. I personally find a 1,000 loc PR almost unreviewable. It takes me multiple days and a stack of notes, and still almost always ends up with effects I didn’t anticipate. Trying to understand the changes and their full implications is nearly impossible. 100 loc is a better top end imho.

dlsa 5y ago
Code reviews are interesting to watch. Especially when the author awkwardly pauses and says, "I'm not sure what this code does".
coldcode 5y ago
Last place I worked (big company, not FAANG) I remember a PR for our main app that affected 500 files and occasionally dozens of places in some of them. All the reviews were "LGTM".
- ascagnel_ 5y ago
  I forget where I first saw it, but there's a joke of "ten lines, ten issues; 1,000 lines, looks good". There's a human limit to how much you can hold in your head when reviewing code.
  - whimsicalism 5y ago
    I think it's a double edged sword - people are also way too nitty for small commits, which only incentivizes behavior like this.
  - thetallstick 5y ago
    Looks like the programmer's version of: "If you owe the bank $100, that's your problem. If you owe the bank $100 million, that's the bank's problem.” https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-100-that-s-your-problem-if https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-...
  - sockpuppet69 5y ago

ricardobeat 5y ago

Note that those commits seem to have been added by an OBS contributor on top of Apple's patch. The fact they touch the entire build system, in a PR that is 100% macOS focused, is much more concerning than the commit messages.

emptybottle 5y ago

Yes, a good commit message should be a few concise paragraphs. The subject describes what is being affected (think easily greppable keywords) Then, then body should go into detail about the contents and rationale for the changes. Use specific terms like “add” or “remove” and avoid subjective descriptions like “prepare” “clean” and “fix” For example this patch says: > Clean up consistency of tabs/spaces in include_directories. Well, what does clean up tabs/spaces actually mean? Saying “remove tabs in favor of spaces” is much clearer.

rlt 5y ago

HN: big tech companies should contribute more to open source! Also HN: no, not like that!

dimator 5y ago
There's nothing weird about both of those being true.

fnord77 5y ago

in the description of the PR, there's almost 1000 words of text describing in detail what is being done. I don't see the problem.

Joeri 5y ago

Never say what you ware doing (it's obvious this commit is updating build scripts). Say why you are doing it Don’t say what you were doing, say what the commit does. A good commit message completes the sentence “when merged this will …”. Adding a more detailed description on the second line including why things were done is also not bad, but I have a strong opinion that design notes do not belong in the commit log but in code comments or markdown files that are in the repo, so that they can be found more easily and updated.