Rename README.crashpad to .md [crashpad/crashpad : main]
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
hmm this file doesn't seem to contain any metadata, it's just a placeholder so the dir is created maybe?
Compare with https://chromium.googlesource.com/crashpad/crashpad/+/main/third_party/mini_chromium/README.crashpad which does seem to contain some valid metadata.
Is this correct? Perhaps mark@ knows more background on this.
Related details
- Jordan Brown
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris added 1 comment![]( "Open in Gerrit")
hmm this file doesn't seem to contain any metadata, it's just a placeholder so the dir is created maybe?
Compare with https://chromium.googlesource.com/crashpad/crashpad/+/main/third_party/mini_chromium/README.crashpad which does seem to contain some valid metadata.
Is this correct? Perhaps mark@ knows more background on this.
Will HarrisYep, that's why we need to rename this one to .md
this discussion also took place in https://chromium-review.googlesource.com/c/chromium/src/+/6966151
I'm going to recuse myself until mark@ has lgtmed then I will happily rs lgtm to get the second lgtm
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Brown (Gerrit)
Jordan Brown added 2 comments![]( "Open in Gerrit")
Jordan Browncan you explain why this is needed or link a bug?
Done
Renaming README.crashpad to .mdJordan Brownwe prefer imperative mood for CL titles
can you change to "Rename ..." instead of "Renaming ..."
Done
Related details
- Mark Mentovai
- Will Harris
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Brown (Gerrit)
Jordan Brown added 1 comment![]( "Open in Gerrit")
Jordan Brownhmm this file doesn't seem to contain any metadata, it's just a placeholder so the dir is created maybe?
Compare with https://chromium.googlesource.com/crashpad/crashpad/+/main/third_party/mini_chromium/README.crashpad which does seem to contain some valid metadata.
Is this correct? Perhaps mark@ knows more background on this.
Yep, that's why we need to rename this one to .md
Related details
- Mark Mentovai
- Will Harris
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Brown (Gerrit)
Jordan Brown added 1 comment![]( "Open in Gerrit")
Jordan Brownhmm this file doesn't seem to contain any metadata, it's just a placeholder so the dir is created maybe?
Compare with https://chromium.googlesource.com/crashpad/crashpad/+/main/third_party/mini_chromium/README.crashpad which does seem to contain some valid metadata.
Is this correct? Perhaps mark@ knows more background on this.
Yep, that's why we need to rename this one to .md
this discussion also took place in https://chromium-review.googlesource.com/c/chromium/src/+/6966151
I'm going to recuse myself until mark@ has lgtmed then I will happily rs lgtm to get the second lgtm
Acknowledged
Related details
- Mark Mentovai
- Will Harris
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 4 comments![]( "Open in Gerrit")
Rename README.crashpad to .mdThis repository is full of README.crashpad files. In this change, you’re only working on one.
In fact: is it even correct for you to only work on one? Are any other README.crashpad files going to be a problem for your licenses.py? If so, please just deal with them all together at once.
metadata filetype (there are about a dozen of them [1]).A dozen supported metadata filetypes? A dozen README.crashpad files?
Neither of those points are aids to understanding this change, so you should just remove this.
https://source.chromium.org/chromium/chromium/src/+/main:tools/licenses/licenses.py;l=47Links in long-lived stable locations like commit messages should be pinned to a version. Otherwise, when licenses.py changes tomorrow, this ;l=47 becomes useless, and when it’s renamed next week, the entire link becomes useless. Pin it to a hash and what you’re referring to will be unambiguous for all time.
R=w...@chromium.orgPlease don’t use R= lines anymore. Gerrit will automatically insert the appropriate review information such as Reviewed-By: into the commit message’s footer.
Related details
- Jordan Brown
- Will Harris
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jordan Brown (Gerrit)
Jordan Brown added 5 comments![]( "Open in Gerrit")
This is now blocking better vulnerability coverage for chromium, crashpad and the other repositories chrome depends on. Cross timezone reviews like are challenging, in the future, if you only have nits consider marking as LGTM with nits that can be addressed before submitting.
This repository is full of README.crashpad files. In this change, you’re only working on one.
In fact: is it even correct for you to only work on one? Are any other README.crashpad files going to be a problem for your licenses.py? If so, please just deal with them all together at once.
This is the only problematic one.
metadata filetype (there are about a dozen of them [1]).A dozen supported metadata filetypes? A dozen README.crashpad files?
Neither of those points are aids to understanding this change, so you should just remove this.
A dozen README.crashpad files. I updated the description to more clearly reflect this, and how it relates to the context of this change.
Links in long-lived stable locations like commit messages should be pinned to a version. Otherwise, when licenses.py changes tomorrow, this ;l=47 becomes useless, and when it’s renamed next week, the entire link becomes useless. Pin it to a hash and what you’re referring to will be unambiguous for all time.
I removed it.
Please don’t use R= lines anymore. Gerrit will automatically insert the appropriate review information such as Reviewed-By: into the commit message’s footer.
I used `git cl upload --r-owners`, I can file a bug for you if you'd like? What component does git cl belong to?
Related details
- Mark Mentovai
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
This is now blocking better vulnerability coverage for chromium, crashpad and the other repositories chrome depends on. Cross timezone reviews like are challenging, in the future, if you only have nits consider marking as LGTM with nits that can be addressed before submitting.
This is now blocking better vulnerability coverage for chromium, crashpad and the other repositories chrome depends on. Cross timezone reviews like are challenging, in the future, if you only have nits consider marking as LGTM with nits that can be addressed before submitting.
I sense that this is frustrating for you. I apologize, but allow me to explain.
I can’t LGTM if I’m not convinced that a change won‘t land without uncorrectable defects.
A misleading or confusing first line of a commit message becomes a permanent part of the git shortlog. If a change lands with a defective first line of its commit message, it’s an uncorrectable defect.
A commit message devoid of context altogether is also an uncorrectable defect, and that’s how this change was originally proposed, both in the predecessor change https://chromium-review.googlesource.com/c/6966151/ where I first asked for better commentary, and in the first upload of this change at https://chromium-review.googlesource.com/c/6975426/1.
Why is it uncharacteristically important to get it right the first time in a commit message? Contrast it with code: if you goof in the code itself, or in a comment, sure, you can fix it up in a follow-up, no problem. The intermediate state of the code, although perhaps imperfect, is ephemeral. We trust each other to land a follow-up to correct those problems, right? Commit messages are different: the commit message isn’t ephemeral. It’s a fixed, permanent description. If you goof a commit message, there’s no opportunity to recover. We won’t rewrite git history, and although you might consider landing a follow-up with a better message, it’s not going to be obvious to anyone, including existing tools, or be a part of anyone’s workflow, to look at nearby changes to try to understand the context of one with a defective commit message. If something lands with a bad message, you‘ve doomed all future engineers—yourself included!—attempting to make sense of the change. That’s not a fair position to put your coworkers or future self in.
Cross-time zone reviews _are_ challenging—for your reviewer too!—and if you want to help meet that challenge (really, a good idea even when things don’t need to circle the planet), please consider review feedback more diligently, and perhaps even approach your own changes as a reviewer and not just author. As part of that, I recommend that you give thought to long-term durability of your work product, commit messages included.
Rename README.crashpad to .mdJordan BrownThis repository is full of README.crashpad files. In this change, you’re only working on one.
In fact: is it even correct for you to only work on one? Are any other README.crashpad files going to be a problem for your licenses.py? If so, please just deal with them all together at once.
This is the only problematic one.
This is the only problematic one.
Then say so in the commit message.
The commit message isn’t just a scratchpad to communicate ideas with your reviewers at review time. It’s at least as important to leave a long-term and accurate record of the change and what motivated it.
Jordan BrownPlease don’t use R= lines anymore. Gerrit will automatically insert the appropriate review information such as Reviewed-By: into the commit message’s footer.
I used `git cl upload --r-owners`, I can file a bug for you if you'd like? What component does git cl belong to?
You can peruse https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/refs/heads/main/git_cl.py. It looks like Chromium→Infra→SDK.
Related details
- Jordan Brown
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |