Hacker News Reader

Comment by pilif | Hacker News Reader

This commit in the PR

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.

View on HN

> 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/

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.

Also, if your commit message has parts that say "also, this", "also, that", don't be lazy and break those alsos into separate commits!

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.

> 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.

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.

Yes, makes a lot of sense. Sorry if I gave the impression that I think somehow this PR is wrong or similar. I was just replying to the fact that sometimes splitting up commits makes sense, outside of this PR.

My exception to the "also" rule of thumb is that if the split changes would cover separate areas of the code such that each piece could be easily picked out by "git add -p", then splitting them out isn't required as long as they are all associated to one logical larger story.

> ...you can't revert just one without reverting the others

That's often true in a split out patchset anyway, so I don't think that on its own it stands as a justification for doing things one way or the other.

Dubious. I prefer that style myself, but there's a lot of value in squashing multiple commits around a feature into a single, "this commit implements this feature" commit. Is it valuable to retain the entire development history of a feature, or just the feature itself? What if during the development, it was rewritten a dozen times? What if there's a dozen commits where the author meandered and only in the last three commits got to the point?

Ruthless interactive rebase is one way to fix that last point, but if you take the final result and squash that into a single changeset, there's a point to be made about that as well.

Agreed, it's a matter of preference, but I prefer logically separate changes to be in separate commits at least within the PR, where it may make reviewing the PR easier. It's basic empathy towards the reviewer(s).

Of course, when merged, it often makes sense to squash some commits together, just like you say - long-term, many of the feature development details are not important.

It's not an either-or proposition.

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.

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.

> 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.

I believe it’s the imperative (enable widget) not the infinitive (to enable widget) that is generally used.

> 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.

> This blog post is a good description of the most prevalent convention in how to write good and useful messages.

https://microsoft.github.io/code-with-engineering-playbook/c...

> 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.

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)

Ironically, the referenced article on how to write good git commit messages (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.

so true.

my personal observation: because writing down „what“ is hard. Writing down „why“ is even harder.

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

Looking forward to that: I'm going to be needing to use OBS on Apple Silicon.

I honestly thought it already was running natively, it already runs super smooth in rosetta

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.

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.

Github can point the PR at that branch and those commits won’t show up anymore.

That branch is on a different fork (PatTheMav's), so they wouldn't be able to do that in this case.

TIL. Thanks.

Yes, and the author was asked to base the PR on 5155:

https://github.com/obsproject/obs-studio/pull/5875#issuecomm...

And it looks like they're just going to set the PR to a draft until after 5155 is merged, so it will get cleaned up and be easier to review then:

https://github.com/obsproject/obs-studio/pull/5875#issuecomm...

HN is usually better about sussing out the real story.

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.

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".

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.

I think it's a double edged sword - people are also way too nitty for small commits, which only incentivizes behavior like this.

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-...

When you refactor a commonly used structure, it's easily possible to hit situations that you described. In those cases, having nicely organized commits are super important. You need to make sure that the commit that does the refactor only does the refactoring. Nothing else, then you can review the PR commit by commit and it becomes much more manageable.

When people start doing multiple things in a single commit with 1000's of changes, it does become mentally exhausting to review. Unfortunately, it requires either being extremely diligent when making the commits or being familiar with git rebase -i.

Of course, sometimes it's just thousands of lines of new code and impossible to review :).

10 lines of code = 10 issues. 500 lines of code = "looks fine."

Parkinson’s Law of Triviality

about days and a stack of notes :

this is why we have line printers, custom reading tables with balanced soft lighting (print proofing lamps) and more recently, rostrum cameras operated by foot switch.

review with fanfold might be a habit only acquired by formative experience, but once you physically manage the printout, and are comfortably seated with several hundred well spaced lines that aren't directly emitting typically exhausting spikey spectrum light forcing your brain to perform extremely complex colour management to balance your vision, which is the primary factor in concentration depletion in every attempt at testing we've tried over the years, and you mix in modern handwriting recognition and a pico projector display of feedback comments if necessary, I have only witnessed very high productivity. Anyone who is subject to audits will know the satisfaction of dumping fanfold boxes on the auditors desk. Or should.

Code reviews are interesting to watch. Especially when the author awkwardly pauses and says, "I'm not sure what this code does".

HN: big tech companies should contribute more to open source!

Also HN: no, not like that!

There's nothing weird about both of those being true.

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"

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.

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.

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

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.

A million comments and this is the only valuable one. Amazing that everyone accepted the premise that Apple proposed refactoring some other project’s CI.

>Apple probably won't have the resources to rework their PR //

Aren't Apple the richest corporation in the World, how would they not have resources; am I missing a joke? Maybe you meant 'will choose not to'?

I didn't get where I am today by employing engineers with my profits...

Before three was Dallas, there was The Rise And Fall Of Reginald Perrin, in the main arc a dark parody of 70s corporate England. If you could find a better audio-visual exposition of the transatlantic difference at the end of the nineteen seventies, you will struggle to find a funnier one.

http://www.leonardrossiter.com/reginaldperrin/CJisms.html

https://www.google.co.uk/search?q=reggie+perrin+i+didn%27t+g...

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

None of that is part of the Git repo, so any mirrors won't have the context.

It's in the commit message: https://github.com/obsproject/obs-studio/pull/5875/commits/5...

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.

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

> 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.

A fairly common occurrence when looking at old commits is that any URLs mentioned are now 404s, and ticket numbers are for an issue tracker that no longer exists.

Isn't the PR exactly where you'd go to see that information, if you're an org that uses PRs to store that information?

Unlike commit msgs, which are immutable and stored with the code, Jiras/Githubs etc. are transient.

It's in the commit message, so it will get stored: https://github.com/obsproject/obs-studio/pull/5875/commits/5...

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.

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.

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.

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.

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.

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.

Seems pretty fucking clear based on the message to me.

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.

Reply on news.ycombinator.com