Specify the developer policy around links to external resources

[aaron.ballman]

Summary

This specifies the developer policy on adding links in source & test files and commit messages, related to discussion at: https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847

The intent is to discourage adding links to resources that are not available to the community as a whole (dead links, links to internal documentation, links to internal bug trackers, etc) from source and test files, while still allowing such links to appear in other contexts as needed. It suggests to instead add sufficient context in the surrounding comments to make such links unnecessary.

It also clarifies that these links can appear in commit messages as metadata (similar to how we already have Differential Revision and Fixes metadata with links).

[arsenm]

llvm/docs/DeveloperPolicy.rst

358

I haven't quite figured out what the exact syntaxes which are automatically recognized. It seems to recognize "Fixes #Nxyz"

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

358

Yup, it does support that form as well. I had heard more than once during code review that folks seem to prefer the full link because it's easier to click on that from the commit message than it is to navigate to the fix from the number alone. That seemed like a pretty good reason to recommend the full form, but I don't have strong opinions.

[smeenai]

llvm/docs/DeveloperPolicy.rst

358

+1 for encouraging the full link

[ldionne]

llvm/docs/DeveloperPolicy.rst

358

Perhaps we could encourage using https://llvm.org/PR12345 instead? Does anybody know whether llvm.org/PRXXX is something that we intend to keep around with the Github transition or not?

358–359

Do you think this adds a useful reminder?

[mehdi_amini]

Approving in support, but don't consider this enough as-is. I suspect you should get @lattner approval here :)

llvm/docs/DeveloperPolicy.rst

356–357

More explicitly: "automate processes, including for downstream consumers"?

358

@arsenm: It's documented https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
And for linking cross-repo: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests

[lattner]

Thank you so much for raising this issue Aaron.

Documenting the policy in the developer policy is totally the right thing to do given this cross-cuts individual subprojects, but I don't think we have consensus on what the right approach is. This is a pretty important issue to a number of LLVM contributors, and I think we should have a first-principles discussion about it on the forum before instituting a policy.
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847/43?u=clattner

[aaron.ballman]

In D155081#4499046, @lattner wrote:

Thank you so much for raising this issue Aaron.

Documenting the policy in the developer policy is totally the right thing to do given this cross-cuts individual subprojects, but I don't think we have consensus on what the right approach is. This is a pretty important issue to a number of LLVM contributors, and I think we should have a first-principles discussion about it on the forum before instituting a policy.
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847/43?u=clattner

You're linking to that very discussion; I've not seen a whole lot of new input on that thread which did seem to come to a consensus, but I'm happy to wait longer for folks to weigh in before proceeding.

[tonic]

As I mentioned on the Discourse thread, but if this policy change is made, there should be some discussion about if this applies retroactively. Making a note here, because it probably should be included in the changes to Developer Policy if the radar links are not removed but not allowed going forward.

[probinson]

llvm/docs/DeveloperPolicy.rst

358

Perhaps we could encourage using https://llvm.org/PR12345 instead? Does anybody know whether llvm.org/PRXXX is something that we intend to keep around with the Github transition or not?

Currently the PRxxx links are to the old bugzillas, not the Github issues. It might be sad to lose that.

[aaron.ballman]

Updated based on review comments.

[aaron.ballman]

In D155081#4507579, @tonic wrote:

As I mentioned on the Discourse thread, but if this policy change is made, there should be some discussion about if this applies retroactively. Making a note here, because it probably should be included in the changes to Developer Policy if the radar links are not removed but not allowed going forward.

I believe this was sufficiently clarified on the Discourse thread -- this policy follows all the same procedures as our other coding style policies. If you think the existing policy is unclear in this area, I think that should be handled as a separate policy discussion instead of as an ad hoc statement for just this change.

[aaron.ballman]

Accidentally uploaded a partial diff instead of the full set of changes.

[cor3ntin]

This seems very reasonable to me

llvm/docs/DeveloperPolicy.rst

264

[aaron.ballman]

Updated based on review feedback.

[rZhBoYao]

llvm/docs/DeveloperPolicy.rst

358

If the full link is preferred, can you update the first bullet point in the Resolving/Closing bugs section of LLVM Bug Life Cycle?

[tahonermann]

This looks good to me.

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

358

Given that the RFC was specifically about links and not about bug lifecycle, I'd rather change that policy in a different patch (I suspect someone would have to make a full RFC to make the change; I don't feel strongly enough to go through that process myself). It might make more sense to link to that section of the documentation from here instead of spelling out something that may sound like a conflicting policy. e.g.,

If the patch fixes a bug in GitHub Issues, we encourage adding a reference to the issue being closed, as described `here <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`_.

[aaron.ballman]

Tweaked wording to link to the policy about adding links to github issues in commit messages instead.

[tonic]

@lattner Please review.

[lattner]

Looks like a great improvement to the policy, thank you all for sorting through this!

[reames]

llvm/docs/DeveloperPolicy.rst

349

Removing this item seems very off topic for the change description, and certainly hasn't been discussed in the linked thread. Please add this back in a separate commit.

(To be clear, no objections to the overall change, just the removal of the phab link text.)

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

349

Hmm, I thought this was obsoleted by the new text (it is covered by "other kinds of metadata"). That said, losing that link is definitely a regression, so thank you for pointing this out! I'll find a way to add it back in (either as a stand-alone bullet point or incorporated into the new text).

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

349

I restored the link in https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d as part of the new bullet; please let me know if you have additional concerns.

[reames]

llvm/docs/DeveloperPolicy.rst

349

That 90% covers it. What's left is some minor framing. I'd suggest separating that point into two. The first should be recommended metadata (phab, issues link), and the second can be the additional metadata point. Something like:

If the patch has been reviewed, add a link to its review page, as shown
  `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`_. If the patch fixes a bug in GitHub Issues, we encourage adding a reference to the issue being closed, as described `here <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`_.

It is also acceptable to add other metadata to the commit message to automate processes, including for downstream consumers. and including links to resources that are not available to the entire community. However, such links and/or metadata should not be used in place of making the commit message self-explanatory.

All of the above is just reorganizing what you had written with some very minor copy editing. I'd separately suggest adding the following sentence at the end of the second bullet.

Note that such non-public links are *only* allowed in commit messages, and should not be included in the submitted code.

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

349

I did some minor rewording for clarity, so how about:

* If the patch has been reviewed, add a link to its review page, as shown
  `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`__.
  If the patch fixes a bug in GitHub Issues, we encourage adding a reference to
  the issue being closed, as described
  `here <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`__.

* It is also acceptable to add other metadata to the commit message to automate
  processes, including for downstream consumers. This metadata can include
  links to resources that are not available to the entire community. However,
  such links and/or metadata should not be used in place of making the commit
  message self-explanatory.

All of the above is just reorganizing what you had written with some very minor copy editing. I'd separately suggest adding the following sentence at the end of the second bullet.

Note that such non-public links are *only* allowed in commit messages, and should not be included in the submitted code.

I think this might need more wordsmithing, which is why I left out of the simple reorganization. The non-public links aren't limited to commit messages -- for example, they're fine to use in a phabricator review or github issue comment, etc. So I don't want to be too restrictive with the wording, though I agree with the intent. How about something along the lines of:

Note that such non-public links should not be included in the submitted code.

[reames]

llvm/docs/DeveloperPolicy.rst

349

LGTM to both parts. Wording is hard, and good catch on the second. :)

[aaron.ballman]

llvm/docs/DeveloperPolicy.rst

349

Thank you for the help! Because this is rearranging existing text and the additional text is a minor point of clarity, I'll land those changes shortly -- but as always, if folks have more post-commit review feedback on those changes, please speak up and I'll work with you on it.

llvm/docs/DeveloperPolicy.rst

 
 * Test cases should be written in :doc:`LLVM assembly language <LangRef>`.
 
 * Test cases, especially for regressions, should be reduced as much as possible,
   by :doc:`bugpoint <Bugpoint>` or manually. It is unacceptable to place an
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
 for coverage (correctness, performance, etc) testing, not feature or regression
 testing.
 
 Release Notes
 -------------

 
 #. Code must pass the ``llvm/test`` test suite.
 
 #. The code must not cause regressions on a reasonable subset of llvm-test,
    where "reasonable" depends on the contributor's judgement and the scope of
    the change (more invasive changes require more testing). A reasonable subset
    might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
 * The code should compile cleanly on all supported platforms.
 
 * The changes should not cause any correctness regressions in the ``llvm-test``
   suite and must not cause any major performance regressions.
 

 
 * The body, if it exists, should be separated from the title by an empty line.
 
 * The body should be concise, but explanatory, including a complete
   reasoning.  Unless it is required to understand the change, examples,
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
 * If the commit is a bug fix on top of another recently committed patch, or a
   revert or reapply of a patch, include the git commit hash of the prior
   related commit. This could be as simple as "Revert commit NNNN because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`_.
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub Issues,
+  we encourage adding a reference to the issue being closed, as described
+  `here <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and
 omissions can be handled by sending a reply to the commits mailing list.
 
 .. _revert_policy:
 
 Patch reversion policy