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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
