[PATCH] D95018: [NFC][AMDGPU] Document target ID syntax for code object V2 to V3

Tue Jan 26 13:15:19 PST 2021

scott.linder added a comment.

In D95018#2523640 <https://reviews.llvm.org/D95018#2523640>, @MaskRay wrote:

> In D95018#2523615 <https://reviews.llvm.org/D95018#2523615>, @scott.linder wrote:
>
>> In D95018#2523019 <https://reviews.llvm.org/D95018#2523019>, @MaskRay wrote:
>>
>>> In your commit the message does not include `Reviewed by:`. Many people agree that both `Reviewed by:` & `Differential Revision:` should be present. The `Reviewed by:` list indicates people who acknowledged the patch. (The `Reviewers:` list does not necessarily mean all the people on the list have acknowledged the patch so `Reviewers:` is mostly useless.)
>>>
>>> `arc amend` can fetch the Phabricator summary and amend the local description.
>>>
>>> You can install `llvm/utils/git/pre-push.py` to prevent accidental `Summary:`, `Reviewers:`, `Subscribers:` and `Tags:` in the presence of `Differential Revision:`.
>>
>> Why is including anything besides "Differential Revision:" being permitted/required? It seems unfortunate enough that anything is needed in the commit message at all, but at least it is a single, relatively unobtrusive thing. "Denormalizing" the message by duplicating information contained in the Differential Revision doesn't seem helpful.
>>
>> If the utility is in avoiding the need to query Phabricator to get this information, then can we make the "Reviewed By:" field include full names, rather than (often opaque) usernames/handles?
>
> Sorry, the consensus is that `Differential Revision:` is needed. `Reviewed by:` is optional. Others are not useful.

Having an optional field that people are reminded to always include seems to imply it should just be required, but I guess it is a minor point. I'll just try to remember to include the "Reviewed By:" in the future. Does `arc land` work with our repo, and if so will it always update the message to the canonical one automatically?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95018/new/

https://reviews.llvm.org/D95018