[PATCH] D128645: Update developer policy.

Tue Jun 28 07:59:18 PDT 2022

aaron.ballman added inline comments.

================
Comment at: llvm/docs/DeveloperPolicy.rst:87

-#. 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`).
----------------
vext01 wrote:
> aaron.ballman wrote:
> > vext01 wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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.
> > Oh, yes, that is confusion on my end! I was thinking `git clang-format`, so this part is fine by me. Thanks!
> Woah! I just realized that `-U 999999` and `-U999999` are interpreted differently by `git`. We need the latter.
TIL I've been doing it right almost entirely by chance. :-)

You should probably fix up the patch summary as well.

================
Comment at: llvm/docs/DeveloperPolicy.rst:419-421
-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.
----------------
vext01 wrote:
> aaron.ballman wrote:
> > vext01 wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > 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?
> > > 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).
> 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? 
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.

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

https://reviews.llvm.org/D128645