This is an archive of the discontinued LLVM Phabricator instance.

[docs] avoid 'arc land' command
AbandonedPublic

Authored by trofi on Apr 25 2020, 11:55 AM.

Details

Summary

Avoid using arc land and suggest cleaning up commit
message from most ofr Phabricator tags like Summary,
Reviewers, Subscribers and Tags.

Suggest keeping Differential Revision and use
git commit --amend for that.

Diff Detail

Event Timeline

trofi created this revision.Apr 25 2020, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2020, 11:55 AM
trofi updated this revision to Diff 260159.Apr 26 2020, 3:54 AM

Minor: tweak summary to contain '[docs]' instead of 'docs:'.

are you sure? D<revision> worked for me in the past

mehdi_amini added inline comments.Apr 28 2020, 9:16 AM
llvm/docs/Phabricator.rst
232–233

I didn't know we were documenting arc land, I think we should discourage it heavily instead.

This is creating commit message with a lot of cruft inside: all the Phabricator tags should be removed before pushing IMO.

trofi marked an inline comment as done.Apr 28 2020, 10:35 AM

are you sure? D<revision> worked for me in the past

I copied arc output right from the terminal. Maybe I'm missing some configuration or arc behaviour changed.

llvm/docs/Phabricator.rst
232–233

Oh, TIL.

I can reword the section to avoid arc land. Would you be fine to review it or maybe you can suggest someone else as well?

trofi updated this revision to Diff 260704.Apr 28 2020, 11:06 AM

Instead of tweaking arc land command add a note to strip all
Phabricator tags except Differential Revision.

trofi marked 2 inline comments as done.Apr 28 2020, 11:07 AM
trofi added inline comments.
llvm/docs/Phabricator.rst
232–233

Reworded to discourage arc land.

trofi retitled this revision from docs: update 'arc land' command to [docs] avoid 'arc land' command.Apr 28 2020, 11:10 AM
trofi edited the summary of this revision. (Show Details)
trofi added a reviewer: lattner.
mehdi_amini added inline comments.Apr 28 2020, 4:59 PM
llvm/docs/Phabricator.rst
236

What about adding the same helper function as we document here: https://mlir.llvm.org/getting_started/Contributing/#using-arcanist ?

sylvestre.ledru accepted this revision.Jun 18 2020, 3:45 AM

works for me
I like "arc land" but I am not opposed to changing the doc

This revision is now accepted and ready to land.Jun 18 2020, 3:45 AM
trofi abandoned this revision.Jun 18 2020, 12:30 PM

Initially I intended to tweak the command to something that just works. I don't think I'm the right person to decide on how the changes should be landed. I'll abandon the change.

I'll pick this one up then!

hubert.reinterpretcast added inline comments.
llvm/docs/Phabricator.rst
206

I am against this. I appreciate being able to locate reviewers associated with an area from the commit message.

mehdi_amini added inline comments.Jun 19 2020, 3:14 PM
llvm/docs/Phabricator.rst
206

I think the extra tag is "Reviewed By:"?

lattner added a subscriber: klimek.Jun 19 2020, 3:27 PM

I'd recommend running this by @klimek

lattner edited reviewers, added: klimek; removed: lattner.Jun 19 2020, 3:27 PM
llvm/docs/Phabricator.rst
206

Yes; thanks. Also, I find the Summary line useful for separating the commit message body from the subject line on the buildbot.

mehdi_amini added inline comments.Jun 19 2020, 5:19 PM
llvm/docs/Phabricator.rst
206

That seems something to fix on the buildbot side. Git commit message have well known convention that define the title and the description.

hubert.reinterpretcast marked an inline comment as done.Jun 19 2020, 5:32 PM
hubert.reinterpretcast added inline comments.
llvm/docs/Phabricator.rst
206

I'm fine with that view. Just noting that newer buildbot instances I am aware of still generate the "Comment" with the subject line immediately before the first line of the body.

General note on that patch:
I am fine with both views, I know people who like to use arc land, I know people (like myself), who like to not have noisy messages.
I don't know that we have a good process to really decide that (Chris as BDFL dropped himself out of this one :)
I think giving folks a arcfilter line like Mehdi did is a great idea if we decide to go with the policy.