22 Jul
2025
22 Jul
'25
2:44 p.m.
On Tue, Jul 22, 2025 at 7:33 AM Chet Ramey via COFF <coff(a)tuhs.org> wrote:
On 7/21/25 6:26 PM, josh wrote:
As someone who has been on a project for the last 30 years (FreeBSD)
that's encouraged long, complete commit messages, I have to strongly
disagree. Separate change log are useless in comparison. If I go to
the gnu binutils and find a changelog entry that says something about
fixing some mips symbol type thing, even with specifics, I don't know
the code that it affected. With that same information in the commit
log, when I git-blame the code, I can find why a line of code changed
over time, and even the other changes associated with it. That
metadata is quite useful to understanding how we got here with the
code. Having had to look at projects that only added it to the commit
log, it's a night and day difference.
When I've looked at other projects and see messages like 'fix, ok
deraat' I find myself angry. Why was it a bug? why was it the right
fix? etc.
For example, how do you put this into a change log such that someone
could find it years from now:
commit 73e1bd71427794ee5496fdeb2bdaa04b05b0c35b
Author: Warner Losh <imp(a)FreeBSD.org>
Date: Mon Jul 21 20:50:50 2025 -0600
cam/mmc: Remove stray xpt_path_inq
Turns out, we don't use the results of xpt_path_inq here at all. And it
also causes problems. Since it calls xpt_cam_inq to do this useless
XPT_PATH_INQ, it loses the original priority we had for the CCB. This
priority should be CAM_PRIORITY_XPT, but was originally set to
CAM_PRIORITY_NORMAL. This worked to enumerate the device because no
normal priority CCBs were queued by anything doing the
enumeration. However, when I changed xpt_path_inq to use the more proper
PRIORITY_NONE, it exposed this bug because queued CCBs with
PRIORITY_NONE sometimes won't run. This caused the probe device to stop
after its first operation. Removing the xpt_path_inq means we no longer
step on the important fields we get from xpt_schedule, allowing probing
to work correctly.
Noticed by: bz@
Sponsored by: Netflix
I'll admit I could have worded parts of that a little better. Now git
blame will show that I removed this call, and give a good background
for why (and yes, this condition was turned into an KASSERT after
fixing the other bugs that the KASSERT found).
I could have made my future self or future colleges mad by just saying
'remove xpt_path_inq call' since they'd have no idea why, or the
background. How the heck would I have found this in a changelog in 5
or 10 years time? Especially if I was looking for all commits that
touched xpt_path_inq because I suspected one elsewhere in the code of
causing problems too?
Warner
On Monday, July 21, 2025, Chet Ramey via COFF
<coff(a)tuhs.org
<mailto:coff@tuhs.org>> wrote:
On 7/21/25 11:27 AM, Paul Winalski wrote:
When writing all but the most trivial bug fixes I always put in a
comment referring to the bug report number. This helps with what
can otherwise be a perplexing problem: "why is this bit of code
there?"
I put those in the change log entries.
Does anyone else feel like this is still an unsolved problem?
It seems git blame continues to be the state of the art for connecting a
section of code to the “commit” (or analogous concept) in which it was
added, which is where one would include context about why the change was
made and connect it to the wider world (bug tracker, etc).
I don't like commit messages that are paragraph length. That seems more
appropriate for a separate change log.