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

Tue Jan 26 12:53:11 PST 2021

MaskRay added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95018