Referencing rdar bugs in commit messages

Would it be possible, when referencing a rdar bug in a commit message, to additionally provide a brief description of the bug that it fixes? Those of us without access to rdar don’t have any insight into what the patches are fixing.

I understand this may not be possible when the bugs are related to issues that are not public information, but wherever possible I think this would benefit all of the non Apple people.

Seems to me the commit message should say what problem the commit fixes regardless of whether there's a Radar number associated.

Jim

We strip the actual title from the radar, but we do describe what is being fixed in each commit message. So you aren't missing any info.

Zachary might be referring to commits like 221850.

"Do not override the existing definition of addr_size when adding new properties to SBTarget. Fixes rdar://18963842”

It’s not immediately clear from that message what the original issue here was: a problem with 220372 where addr_size was erroneously repurposed to mean “size of a target byte in host bytes” where it had a different meaning (“size of a target address in bytes”) originally.

Sean

Zachary might be referring to commits like 221850.

"Do not override the existing definition of addr_size when adding new properties to SBTarget. Fixes rdar://18963842”

I see nothing wrong with the content of that comment. It's clear that it means "addr_size" was changed to mean something it shouldn't mean. The details in the Radar are "addr_size no longer returns the correct value." which was obvious from the fact that it's definition was changed and didn't add any more detail. So saying more about it wouldn't have helped.

I wouldn't put "Fixes rdar//whatever" in the comment, that does seem like it's dangling candy just out of reach. I always just put the radar number at the bottom of the comment since it is useful when you get another Radar reporting the same problem and internally we want to dup the two, but it makes it clear it isn't a vital piece of info. But other than that the comment should stand on its own.

Jim

I didn’t want to call out any specific commits, although that is the most recent example I’ve seen. I mostly just brought it up because I’ve seen similar commit messages on a number of different occasions. I’m sure it’s completely unintentional, but ideally commit messages would stand on their own with no additional context needed to understand the problem.

It’s possible i’m just advocating for better commit messages in general, and this is unspecific to rdar issues. For example, Sean’s explanation elaboration is more informative and gives some context to people who aren’t already that familiar with that section of code.

I wouldn’t put “Fixes rdar//whatever” in the comment, that does seem like it’s dangling candy just out of reach. I always just put the radar number at the bottom of the comment since it is useful when you get another Radar reporting the same problem and internally we want to dup the two, but it makes it clear it isn’t a vital piece of info. But other than that the comment should stand on its own.

If the primary reason for linking rdar issues in commit messages is just to be able to dupe the issue if they’re fixed by the same issue, then wouldn’t it work to link the commit number in the rdar issue instead of the other way around? Mostly though it’s what you said, seeing it there makes it appear that there’s additional information that’s being left out, even if there’s not.

Zachary might be referring to commits like 221850.

"Do not override the existing definition of addr_size when adding new properties to SBTarget. Fixes rdar://18963842”

I see nothing wrong with the content of that comment. It’s clear that it means “addr_size” was changed to mean something it shouldn’t mean. The details in the Radar are “addr_size no longer returns the correct value.” which was obvious from the fact that it’s definition was changed and didn’t add any more detail. So saying more about it wouldn’t have helped.

+1 - the patch seemed trivial and obvious enough that a few words suffice to explain it, regardless of radar

I wouldn’t put “Fixes rdar//whatever” in the comment, that does seem like it’s dangling candy just out of reach.

I like having the radar number somewhere in there, but as you say…

I always just put the radar number at the bottom of the comment since it is useful when you get another Radar reporting the same problem and internally we want to dup the two, but it makes it clear it isn’t a vital piece of info.

If that is a better modus operandi by community consensus, sure works for me

But other than that the comment should stand on its own.

Jim

It’s not immediately clear from that message what the original issue here was: a problem with 220372 where addr_size was erroneously repurposed to mean “size of a target byte in host bytes” where it had a different meaning (“size of a target address in bytes”) originally.

Sean

We strip the actual title from the radar, but we do describe what is being fixed in each commit message. So you aren’t missing any info.

Would it be possible, when referencing a rdar bug in a commit message, to additionally provide a brief description of the bug that it fixes? Those of us without access to rdar don’t have any insight into what the patches are fixing.

I understand this may not be possible when the bugs are related to issues that are not public information, but wherever possible I think this would benefit all of the non Apple people.


lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev


lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev


lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev


lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev

Thanks,
- Enrico
:envelope_with_arrow: egranata@.com :phone: 27683

Yea, I guess I’m just advocating for more detail in general. A few years from now, I’ll even forget what problems my own commits were trying to solve, much less someone else’s :slight_smile:

+1 - the patch seemed trivial and obvious enough that a few words suffice to explain it, regardless of radar

I agree, and sorry for using your patch to bring the issue up. It’s just something I’ve seen off and on through the months.

I probably should have just raised the issue of more descriptive commit message independently of radar, so my fault on that front. Like an “even if there’s nothing wrong, that doesn’t mean it can’t be improved” kind of thing.

I usually try to write commit messages so that someone who doesn’t understand anything about Windows can still figure out what my patch is doing.

I was actually under the impression that other LLVM projects weren’t allowed to put rdar numbers in commit messages, but someone explained to me offline that I was wrong about that, so putting it at the bottom is probably fine.

I didn't want to call out any specific commits, although that is the most recent example I've seen. I mostly just brought it up because I've seen similar commit messages on a number of different occasions. I'm sure it's completely unintentional, but ideally commit messages would stand on their own with no additional context needed to understand the problem.

It's possible i'm just advocating for better commit messages in general, and this is unspecific to rdar issues. For example, Sean's explanation elaboration is more informative and gives some context to people who aren't already that familiar with that section of code.

> I wouldn't put "Fixes rdar//whatever" in the comment, that does seem like it's dangling candy just out of reach. I always just put the radar number at the bottom of the comment since it is useful when you get another Radar reporting the same problem and internally we want to dup the two, but it makes it clear it isn't a vital piece of info. But other than that the comment should stand on its own.

If the primary reason for linking rdar issues in commit messages is just to be able to dupe the issue if they're fixed by the same issue, then wouldn't it work to link the commit number in the rdar issue instead of the other way around? Mostly though it's what you said, seeing it there makes it appear that there's additional information that's being left out, even if there's not.

Putting the Radar number in the commit message helps when you look at a new issue, and think "I fixed that earlier by touching code in function Foo, but I have no idea what the original Radar was." Then you can go to Foo, look at the commit log (svn blame is often helpful to find the right commit) and then from there go straight to the Radar. For this purpose it's really handy to have the Radar number there. Note it would similarly be handy to put the bugzilla PR in if you were working off a bugzilla report.

Jim