Update developer policy.

[vext01]

Summary

I've recently gone though the process of getting commit rights and have raised a few diffs already.

The developer policy is a little out of date in some places. This change updates them based upon my own personal experience.

Namely:

• diffs are not passed around on mailing lists any more.
• diffs should be -U 999999
• you don't have to respond to any automated email.

Looks good?

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

87–91

Changing this would require an RFC to see if the community wants to get rid of our requirement that patches be formatted. Personally, I'd be opposed to such a change; I think this should be kept.

112–114

I don't think we want to lose this bit either, as I don't think this advice has changed.

419–421

Rather than get rid of this, I think we might actually want to broaden it. I read this blurb as letting folks know that sometimes commit messages take a while before they show up on the commit list. It used to be the primary way that happened was when making a commit for the first time. Now it happens most often for large commits (due to the size of the email content) or a long list of CCs (often added automatically by Herald, though the moderation of these has gotten better in recent history).

I think it's kind of helpful to let people know that sometimes the automated emails get caught out by moderation. But if others don't think that's of value to mention, then we can go ahead and remove this bit.

[vext01]

llvm/docs/DeveloperPolicy.rst

112–114

Oops, you are right. My bad!

419–421

Actually, now I read it again, I realise that I don't understand what this sentence means:

Your first commit to a repository may require the autogenerated email to be approved by a moderator of the mailing list.

My first commit to a llvm repository was via github, and github doesn't discriminate. If you have write-access to the repo, then your push to main will surely go ahead. There are no automated emails involved as far as I know.

I suspect this prose is from pre-github, where the process was different?

[vext01]

llvm/docs/DeveloperPolicy.rst

87–91

Is there confusion between git format-patch and git clang-format here?

To be clear, I'm not proposing that the source code you change isn't syntactically formatted. But git format-patch does not do syntactic formatting, it just writes a diff to disk.

I don't think it matters how you generate your diff, but your changes need to have gone through git clang-format as described elsewhere in the llvm docs.

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

87–91

Oh, yes, that is confusion on my end! I was thinking git clang-format, so this part is fine by me. Thanks!

419–421

Actually, now I read it again, I realise that I don't understand what this sentence means:

Ah, I think I see where the confusion may be coming in.

We have a post-commit hook that pushes all commits to a commits email list: https://lists.llvm.org/pipermail/cfe-commits/ (as an example, there's also commits list for LLVM and others), and it's existed for a *long time*. It used to be that your commits were written to the commits list as though they came from you directly (e.g., https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130225/074838.html), and these days they come in as though from a list bot (e.g., https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20220627/424937.html); check out the from line just below the title to spot the differences. So the old issue was that when you first pushed a commit, you may not have been subscribed to cfe-commits and your commit message wouldn't make it to the lists. Now the issue is that when you push any commit, it might be caught up by moderation filters (this also used to be an issue, but it wasn't the most likely issue for people to hit).

[vext01]

llvm/docs/DeveloperPolicy.rst

419–421

OK, so what do you recommend we do? This prose is currently full of historic details that are confusing/intimidating for a newbie.

Does the sentence still apply, or can we kill it?

[vext01]

llvm/docs/DeveloperPolicy.rst

87

Woah! I just realized that -U 999999 and -U999999 are interpreted differently by git. We need the latter.

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

87

TIL I've been doing it right almost entirely by chance. :-)

You should probably fix up the patch summary as well.

419–421

Do you think something along these lines is still intimidating for a newbie?

"For external tracking purposes, committed changes are automatically reflected on a commits mailing list (link to llvm-commits archive, link to cfe-commits archive) soon after the commit lands. Note, these mailing lists are moderated and it is not unusual for a large commit to require a moderator to approve the email, so do not be concerned if a commit does not immediately appear in the archives."

Or something along these lines? Basically, I think it's useful for people to know that the commit is automatically reflected somewhere (so your commit messages are more visible than just git log/blame) and that's it's not something you need to worry about as a committer if you don't see your commit immediately because all the emails get reflected eventually.

[vext01]

llvm/docs/DeveloperPolicy.rst

419–421

That makes a lot more sense to me. I'll make that change and supply a revised diff. Thanks.

[xbolva00]

llvm/docs/DeveloperPolicy.rst

87

typo "infinte"

88

main is weird example here imho.

[vext01]

llvm/docs/DeveloperPolicy.rst

88

How comes? The docs told me to make my diffs off main, so that's what you'd be diffing against typically. No?

[xbolva00]

llvm/docs/DeveloperPolicy.rst

88

Ah, okay. I usually use it without it.

[vext01]

I've addressed the comments.

I only linked llvm-commits (reusing the already defined rst link) as the other lists are referenced elsewhere.

Typo also fixed.

After fixing my -U 999999, grepping the tree for 9999 in rst files shows that all references to -U999999 are correct.

[vext01]

llvm/docs/DeveloperPolicy.rst

88

That would mean you haven't committed your changes, and git clang-fomat wont work in that case, so committing is best IMHO.

[xbolva00]

llvm/docs/DeveloperPolicy.rst

88

I use arc land :)

[aaron.ballman]

The changes LGTM, but you should wait a bit before landing in case other reviewers/subscribers want to chime in if they still have opinions.

llvm/docs/DeveloperPolicy.rst

88

FWIW, I also never put main explicitly on my diff lines; and I don't use arc (I just use git diff). Not everyone's workflow involves carrying around git branches for everything. That said, I don't have an issue with being explicit here, either.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

87

Using git diff like this, there are risks that main is now ahead of your branch's base. Probably safer to use HEAD~n where n is the number of commits you have on your branch.

[hubert.reinterpretcast]

llvm/docs/DeveloperPolicy.rst

398–400

"Note," => "Note that"
Add comma before "and".

[vext01]

llvm/docs/DeveloperPolicy.rst

87

The exact command you want to issue is going to vary according to circumstance and personal workflow preference. Sometimes main is fine, if it isn't HEAD~n is fine, but if you have a lot of commits then determining (i.e. manually counting) n will be impractical and you probably want to just use an absolute git hash.

I don't think we need to delve into a great level of detail. I think the prose "using something like" is adequate.

[vext01]

Fixed grammar bits.

[hubert.reinterpretcast]

LGTM!

llvm/docs/DeveloperPolicy.rst

87

okay

[vext01]

Just pushed this to main (04f6bf482b8641533274d28af5fdac7107da3344)

Thanks everyone!

llvm/docs/DeveloperPolicy.rst

#. Make your patch against git main, not a branch, and not an old version
    of LLVM.  This makes it easy to apply the patch.  For information on how to
    clone from git, please see the :ref:`Getting Started Guide
    <checkout>`.
 
 #. Similarly, patches should be submitted soon after they are generated.  Old
    patches may not apply correctly if the underlying code changes between the
    time the patch was created and the time it is applied.
 
-#. Patches should be made with ``git format-patch``, or similar (see special
-   commands for `Requesting Phabricator review via the web interface
-   <Phabricator.html#phabricator-request-review-web>`_ ). If you use a
-   different tool, make sure it uses the ``diff -u`` format and that it
-   doesn't contain clutter which makes it hard to read.
-
-Once your patch is ready, submit it by emailing it to the appropriate project's
-commit mailing list (or commit it directly if applicable). Alternatively, some
-patches get sent to the project's development list or component of the LLVM bug
-tracker, but the commit list is the primary place for reviews and should
-generally be preferred.
-
-When sending a patch to a mailing list, it is a good idea to send it as an
-*attachment* to the message, not embedded into the text of the message.  This
-ensures that your mailer will not mangle the patch when it sends it (e.g. by
-making whitespace changes or by wrapping lines).
-
-*For Thunderbird users:* Before submitting a patch, please open *Preferences >
-Advanced > General > Config Editor*, find the key
-``mail.content_disposition_type``, and set its value to ``1``. Without this
-setting, Thunderbird sends your attachment using ``Content-Disposition: inline``
-rather than ``Content-Disposition: attachment``. Apple Mail gamely displays such
-a file inline, making it difficult to work with for reviewers using that
-program.
-
-When submitting patches, please do not add confidentiality or non-disclosure
-notices to the patches themselves.  These notices conflict with the LLVM
-licensing terms and may result in your contribution being excluded.
+#. Patches should be unified diffs with "infinte context" (i.e. using something
+   like `git diff -U 999999 main`).
+
+#. Once you have created your patch, create a
+   `Phabricator review <Phabricator.html#phabricator-request-review-web>`_ for
+   it (or commit it directly if applicable).
 
 .. _code review:
 
 Code Reviews
 ------------
 
 LLVM has a code-review policy. Code review is one way to increase the quality of
 software. Please see :doc:`CodeReview` for more information on LLVM's code-review

 `Invitation Link <https://github.com/orgs/llvm/invitation>`_ directly. Once
 accept the invitation, you'll get commit access.
 
 Prior to obtaining commit access, it is common practice to request that
 someone with commit access commits on your behalf. When doing so, please
 provide the name and email address you would like to use in the Author
 property of the commit.
 
-Your first commit to a repository may require the autogenerated email to be
-approved by a moderator of the mailing list.
-This is normal and will be done when the mailing list owner has time.
-
 If you have recently been granted commit access, these policies apply:
 
 #. You are granted *commit-after-approval* to all parts of LLVM. For
    information on how to get approval for a patch, please see :doc:`CodeReview`.
    When approved, you may commit it yourself.
 
 #. You are allowed to commit patches without approval which you think are
    obvious. This is clearly a subjective decision --- we simply expect you to
    use good judgement.  Examples include: fixing build breakage, reverting
    obviously broken patches, documentation/comment changes, any other minor
    changes. Avoid committing formatting- or whitespace-only changes outside of
    code you plan to make subsequent changes to. Also, try to separate