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

Tue Jan 26 13:46:17 PST 2021

MaskRay added a comment.

In D95018#2523721 <https://reviews.llvm.org/D95018#2523721>, @scott.linder wrote:

> 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?

`arc amend` definitely keeps just "Reviewed by:" and `Differential Revision:`. I don't know `arc land` as I don't use it much.

(Personally I run `git pull --rebase origin main && git commit --amend --date=now --no-edit && git push origin HEAD:main`. The `--date=now` amend is to reset the author date to committer date.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95018