[Docs] Update documentation for the new GitHub workflow #65162

tru · 2023-09-01T10:34:50Z

This describes the GitHub workflow in the documentation and marks some sections as deprecated. We should clean up these sections ASAP. I was just keen to get something on the documentation site as soon as possible.

This describes the GitHub workflow in the documentation and marks some sections as deprecated. We should clean up these sections ASAP.

llvm/docs/Contributing.rst

llvm/docs/GettingInvolved.rst

PiotrZSL

Overall ok, but raises even more questions:

Will pushing new commits to branch with open pull request in my fork require some manual "work" on pull request, or new changes to my branch will become immediately visible on pull request.
How about when committer != author on forked branch, who will be used as an author when squash + merge when someone else click "merge" (lets assume single commit pull request).
How about pushing changes without merge, just cherry-pick commit from pull request.
How about "labels" for push requests, who should manage them ?

jh7370

No objections here, other than some minor typos/grammar errors etc.

llvm/docs/GitHub.rst

llvm/docs/MyFirstTypoFix.rst

llvm/docs/GitHub.rst

mstorsjo · 2023-09-01T10:54:44Z

llvm/docs/GitHub.rst

+Landing your change
+-------------------
+When your PR has been accepted you can use the web interface to land your change.
+The button that should be used is called `Squash and merge` and after you can


When using "Squash and merge", does it preserve the commit messages, or at least give a chance to edit the final commit message? This is quite vital. With the given procedures, I would expect the first commit in the branch to have the intended commit message, and later commits to be fixups. In such a case, we'd want to maintain the full commit message of the first commit, and skip the commit messages of the later fixup commits.

Dropping the commit message of the individual commits and summarizising it into a list of commits subjects like this, would be entirely unacceptable:

Merge PR #1234

Change the frobnicator

Fixup

Fixup

Agreed, we probably need some guidance here on how to ensure a good commit message get's associated with the commit that ends up landing on main.

IIRC, github will offer an editable text box in the webui that contains the concatenation of all commit messages in the PR. Assuming the first commit contains a well-written commit message, and the follow-ons are simply fixups, it should be easy to simple remove the "fixup" commit messages.

(As an aside: if it would take too long to research the details needed for writing proper documentation, I think it would be fine to write a placeholder TODO/FIXME in the documentation, e.g. stating that we need more documentation on how to ensure a good commit message, and land this PR, so that the documentation that is ready becomes available sooner rather than later).

The default behaviour here is actually configurable and can be changed to default to "PR title + description"

You do still have the opportunity to edit the message of the squash commit right after, but with that setting it defaults to PR title + description which is what you want 99% of the time.

I'd say this should also be the convention going forward as that is effectively the same way it currently works with phabricator.

When creating a PR (at least in the web interface, I've not used gh before) with a single, it defaults to the title and description of this commit.
I have no clue what it does when you have multiple commits in a branch and opening a PR, I've not done this before and we should probably not encourage anyone doing that.

I have no clue what it does when you have multiple commits in a branch and opening a PR, I've not done this before and we should probably not encourage anyone doing that.

I think it takes the title and the body from the first commit in that branch to use as a title and commit message.

zero9178 · 2023-09-01T11:22:10Z

llvm/docs/GitHub.rst

+In general, you should avoid rebasing a Pull Request and force pushing to the
+branch that's the root of the Pull Request during the review. This action will
+make the context of the old changes and comments harder to find and read.


Isn't this too restrictive? Not rebasing your branch during review is too restrictive IMO. You might have conflicts with main at some point, so you have to rebase at some point to land it and it makes more sense for a reviewer to review the post-rebase changes in my opinion.
Additionally, in such case I am assuming CI will fail as well, as it can only run if there are no conflicts with main right? Having to accept a review that is not tested by CI seems odd to me.

Would it make more sense to say "do not invalidate the commit history by squashing and ammeding commits", but that you can rebase and then force-push as needed? Or maybe this should be in the "Updating Pull Requests" section.

Not putting much value in this (I think I agree more with you), but the closest thing to a consensus I could find was that people wanted to avoid rebases.

I don't think this is a good advice here, unless you explicitly say: "you should merge origin/main into your branch regularly".

Otherwise you may run into conflicts, or mid-air collision: any ninja check you run locally isn't testing against the latest main either.
It's not even clear what the pre-merge CI would test: would it be an outdated version of the code?

I think avoiding rebases is a good policy (though I'd encourage folks to try my diff-modulo-base tool to help in reviews), but it should be allowed.

That most closely reflects how things work with Phabricator.

llvm/docs/GitHub.rst

felipepiovezan · 2023-09-01T11:44:06Z

llvm/docs/GitHub.rst

 For pull requests, please push a branch to your
 `fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks>`_
 of the llvm-project and
 `create a pull request from the fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork>`_.

+Creating Pull Requests with GitHub CLI
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+With the CLI it's enough to create the branch locally and then run:


Should we be explicit that the original PR creation must always have a single commit?
If our advice is to squash and merge, there is an implication here that all PRs must start as a single commit.

This is the scenario I'm worried about: a well intentioned developer creates a lot of small patches (great!) but make a single PR, because they are used to other GitHub flows. The current advice will destroy all the nice work done to create the small commits.

Yes, this is also my concern. For this reason, I strongly prefer rebasing and squashing in fixes and forcepushing for each update of the branch. I know github has been rather bad at tracking old review comments across force pushes before, but I have a faint feeling it's working better these days. (And if we are going to squash at the end, or if we do allow the occasional rebase anyway when needed, we will lose context anyway.)

Personally, when submitting a PR, I would be very vocal about this, about how I want it merged (squashed, or kept as is, or if we insist on incremental fixes in separate commits, I would say that I want to do the final squashing once things are approved, and only then let someone merge it, with rebase+merge instead of squash+merge).

See my comment below, I think there is a valid discussion to have here, but not sure this is the right place to have it. We need to get the documentation in, and what's described here is the closest thing to the consensus I have found.

Just to make sure we are on the same page, I am not proposing anything new or opposing the squash and merge flow; squash&merge is what the majority of people involved in the discussion agreed upon.
The rationale for that decision (AFAICT) was to mimic the phabricator workflow as closely as possible, and that workflow starts on the premise that one unit of contribution is reviewed through a Differential review. To emphasize this intent, I believe that our new documentation should state that "PRs should be created from a single commit", it is a 1:1 match to what exists in phabricator today.

How about adding this:

Keep in mind that each pull request should generally only contain one commit. This makes it easier for reviewers to understand the introduced changes and provide feedback. It also helps maintain a clear and organized commit history for the project. If you have multiple changes you want to introduce, it's recommended to create separate pull requests for each change.

Looks great! :)

Note we also have a section that advise against amend+force-push for the PR workflow ("Updating Pull-request" below)

We currently only allow squash and merge for pull requests, I think this is a good policy because it makes it harder to make a mistake and accidentally push a series of fixup commits. Even with this setting though, it's still possible to have a pull request with multiple commits from the start, you just have to push it manually once it's reviewed rather than going through the PR interface.

, you just have to push it manually once it's reviewed rather than going through the PR interface.

This is a great point and I am glad it is a possibility, but it seems like an advanced use for the following reasons:

This is likely not what most GH users are used to doing, so I think the current wording is appropriate.

GH will not close the review automatically and there won't be any "closed by commit X" messages like we get in phab. Manual intervention is required here.

This doesn't play well with the advice of not force-pushing (on multi-commit PRs, we usually want to address comments on the commit they were mentioned, so that the log is preserved properly after merge).

CI won't be testing each commit to ensure they all compile / test.

felipepiovezan · 2023-09-01T11:59:19Z

llvm/docs/GitHub.rst

+
+  # When your PR is accepted, you can now rebase it and make sure
+  # you have all the latest changes.
+  git rebase -i origin/main


In the original discourse post, I believe you had said:

This is easiest done with git rebase --interactive origin/main and mark the commits with f for fixup

I feel we should emphasize the fixup here and remove the --squash from the merge command.
Squashing through github will create that concatenation of commit messages.

I have updated the wording in the landing fixes section now. Does it feel better to you?

llvm/docs/GitHub.rst

kbeyls

I noticed that there are a few more files in the llvm/docs directory that mention phabricator, besides the ones that you touched in this patch. I think it's fine to leave updating those for a later/separate PR. It seems you've covered the most important ones here.

llvm/docs/CodeReview.rst

llvm/docs/Contributing.rst

kbeyls · 2023-09-01T11:59:23Z

llvm/docs/Contributing.rst

-  % git log origin/main...HEAD --oneline
-  # Push to Github.
-  % git push origin HEAD:main
+Once a patch is reviewed, you can select the "Squash and merge" button in the


It was my impression that either "Squash and merge" or "Rebase and merge" are both acceptable workflows to use? If my impression is correct, maybe both options should be stated here?

We currently only have squash and merge enabled. See my comment below.

I assume this means manual pushes are still acceptable, especially in the context of multi-commit sequences. That is probably worth stating explicitly somewhere.

So the old workflow had "rebase it, re-test locally", but the new one has "click the merge button YOLO". This is fine in a world where the pre-commit testing is as good or better than what people are likely to do locally, but I'm not really sure we're there...

llvm/docs/GitHub.rst

kbeyls · 2023-09-01T12:08:30Z

llvm/docs/GitHub.rst

+Landing your change
+-------------------
+When your PR has been accepted you can use the web interface to land your change.
+The button that should be used is called `Squash and merge` and after you can


Agreed, we probably need some guidance here on how to ensure a good commit message get's associated with the commit that ends up landing on main.

IIRC, github will offer an editable text box in the webui that contains the concatenation of all commit messages in the PR. Assuming the first commit contains a well-written commit message, and the follow-ons are simply fixups, it should be easy to simple remove the "fixup" commit messages.

kbeyls · 2023-09-01T12:10:51Z

llvm/docs/GitHub.rst

+Landing your change
+-------------------
+When your PR has been accepted you can use the web interface to land your change.
+The button that should be used is called `Squash and merge` and after you can


(As an aside: if it would take too long to research the details needed for writing proper documentation, I think it would be fine to write a placeholder TODO/FIXME in the documentation, e.g. stating that we need more documentation on how to ensure a good commit message, and land this PR, so that the documentation that is ready becomes available sooner rather than later).

tru · 2023-09-01T12:33:23Z

Reading the comments here, the most controversial thing is the rebase / squash and merge flow. To be clear, this is how the project is currently configured - if you go to a PR and try to merge it, it will only allow you to use the "Squash and merge" flow - so I also used that wording in the document.

Going back and reading some of the discussion, here and here about the migration I think squash and merge was chosen for this task because it most closely mimics the workflow we are already using with Phab.

I think this policy can be discussed to allow another flow, but I am not sure this documentation (in the last second) PR is the right forum.

My suggestion is that we keep the wording about "squash and merge" but add some wording about you can either:

rebase interactively and fixup the commits and the commit message before finally landing the PR.
use squash and merge, but please remember to edit your commit message when using the convenient button in the web UI.

Would that be a good first draft of this document and then we can have the rest of the discussion on discourse to alter this policy in the future?

tru · 2023-09-01T12:36:11Z

Overall ok, but raises even more questions:

Will pushing new commits to branch with open pull request in my fork require some manual "work" on pull request, or new changes to my branch will become immediately visible on pull request.

It will be automatically updated when you push to your branch. I will add that into the document.

How about when committer != author on forked branch, who will be used as an author when squash + merge when someone else click "merge" (lets assume single commit pull request).

GitHub will handle that, you as a committer just press the button in the web-ui and github will use the right authorship, even when squashing iirc.

How about pushing changes without merge, just cherry-pick commit from pull request.

Unsure what you mean? doing the merge manually from the command line? I think that's something you can do, but I rather we try to use the flow that most people are used to (pressing buttons in the web interface).

How about "labels" for push requests, who should manage them ?

Unclear, I have no thoughts on that at this point.

pogo59

Interestingly, I see various comments marked as resolved but I don't see the updates in the PR.

llvm/docs/GitHub.rst

tru · 2023-09-01T12:51:07Z

Interestingly, I see various comments marked as resolved but I don't see the updates in the PR.

I am marking them as resolved as I am fixing them locally. I haven't pushed the update yet - lot of comments here!

Clarified how to use squash and merge and other smaller fixes.

jh7370 · 2023-09-01T12:55:29Z

Interestingly, I see various comments marked as resolved but I don't see the updates in the PR.

I am marking them as resolved as I am fixing them locally. I haven't pushed the update yet - lot of comments here!

FWIW, I think it should be up to the reviewer to mark a thing as resolved, because otherwise GitHub "conveniently" hides the review comment, and the reviewer finds it hard to double-check to see if the comment has actually been resolved (they have to go through and expand every "resolved" comment to see their previous comment, one of my many gripes about how bad GitHub is for a reviewer experience versus Phabricator). However, that's probably a discussion that needs to happen on Discourse, to determine a community-wide policy.

tru · 2023-09-01T12:57:46Z

FWIW, I think it should be up to the reviewer to mark a thing as resolved, because otherwise GitHub "conveniently" hides the review comment, and the reviewer finds it hard to double-check to see if the comment has actually been resolved (they have to go through and expand every "resolved" comment to see their previous comment, one of my many gripes about how bad GitHub is for a reviewer experience versus Phabricator). However, that's probably a discussion that needs to happen on Discourse, to determine a community-wide policy.

That's a valid discussion to have I think. It's not been the workflow I have ever used on GitHub and there is no good way to enforce it, so I am not sure I agree - but let's have that discussion on Discourse.

unterumarmung · 2023-09-01T13:38:39Z

llvm/docs/GitHub.rst

+  # When your PR is accepted, you can now rebase it and make sure
+  # you have all the latest changes.
+  git rebase -i origin/main


Is it actually a necessary step? GitHub allows merging PRs without requiring the PR to be on ToT as long as there are no merge conflicts with ToT. I'm not certain if this is also the case with GitHub CLI, but it certainly is with GitHub Web. I assume they should function in the same way.

Additionally, the original Phabricator documentation included a reminder to run tests before merging. I believe that if we suggest rebasing, we should also recommend running tests before merging.

To clarify, I'm not insisting; I'm just offering ideas on how to improve the documentation. Thank you for your time!

Is it actually a necessary step? GitHub allows merging PRs without requiring the PR to be on ToT as long as there are no merge conflicts with ToT. I'm not certain if this is also the case with GitHub CLI, but it certainly is with GitHub Web. I assume they should function in the same way.

They work the same. No it's not necessary - but I think it's a good habit to have to avoid merge conflicts when wanting to merge a PR. This is why I suggest it, most of the time it won't matter that much.

I will add a suggestion to run tests - it's something that was suggested above already.

It does not change anything about merge conflict (if the PR can be merged by Github, there isn't any), not rebasing affect the kind of test you're running locally!
(that said we're having pre-merge CI? It may do the rebase/merge when it runs?)

llvm/docs/GitHub.rst

jh7370 · 2023-09-01T13:58:01Z

FWIW, I think it should be up to the reviewer to mark a thing as resolved, because otherwise GitHub "conveniently" hides the review comment, and the reviewer finds it hard to double-check to see if the comment has actually been resolved (they have to go through and expand every "resolved" comment to see their previous comment, one of my many gripes about how bad GitHub is for a reviewer experience versus Phabricator). However, that's probably a discussion that needs to happen on Discourse, to determine a community-wide policy.

That's a valid discussion to have I think. It's not been the workflow I have ever used on GitHub and there is no good way to enforce it, so I am not sure I agree - but let's have that discussion on Discourse.

I've gone and created it as https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178.

bjope · 2023-09-01T14:04:55Z

llvm/docs/GitHub.rst

+  git rebase -i origin/main
+
+  # Now merge it
+  gh pr merge --squash --delete


So this is where I normally (in the phabricator days) would do

git pull

see what new stuff I got

if something might conflict and cause build failures etc, rebuild

if something might conflict and cause test failures etc, rerun tests

if step 3 and 4 took some time, go back to 1

git push origin HEAD:main

So my goal is to push something that has little risk of breaking things in some obvious way that would have been detected by building and rerunning lit tests.

I guess I'll still be able to do exactly the above thing. But then I guess the PR isn't updated automativally (which happened with Phabricator), or is it? Instead I need to remeber to push via my fork somehow (e.g. using "gh pr merge"). Still, I really want to keep the time here short so that I've verified my patch together with top-of-tree. So I hope that "gh pr merge" is rather quick (or that it would fail if I'm not rebased to main making the merge a fast forward).

Yeah you got it mostly right. You would push to your branch git push origin my_branch and then gh pr merge. It would never create a merge commit, it would rebase, the name "squash and merge" is actually a little weird here - check the github docs and you'll see it will actually always use fast-forwarding: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github#squashing-your-merge-commits

Does gh pr merge asks interactively about the resulting commit message? I think when I click "squash and merge" in the UI it pops-up and asks.

Our settings should be (hopefully) to use the PR description, but that would mean that amending a commit locally and setting a commit message would be ignored by gh pr merge ...

…mentation

tru · 2023-09-01T14:44:52Z

I think I have addressed most of the comments in the discussion above, so if everyone can look again, try to point out any glaring problems. I think it's better that we get this merged when we turn on PR's with some issues than to have the old documentation still on our site.

llvm/docs/GitHub.rst

joker-eph

Thanks!

llvm/docs/DeveloperPolicy.rst

llvm/docs/GettingInvolved.rst

joker-eph · 2023-09-01T18:16:58Z

llvm/docs/GitHub.rst

 For pull requests, please push a branch to your
 `fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks>`_
 of the llvm-project and
 `create a pull request from the fork <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork>`_.

+Creating Pull Requests with GitHub CLI
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+With the CLI it's enough to create the branch locally and then run:


Note we also have a section that advise against amend+force-push for the PR workflow ("Updating Pull-request" below)

joker-eph · 2023-09-01T18:17:20Z

llvm/docs/GitHub.rst

+built-in tool. See the section about landing your fix below.
+
+When pushing to your branch, make sure you push to the correct fork. Check your
+remotes with:


Can you incorporate in this section the feedback and changes from https://reviews.llvm.org/D150594 ?

joker-eph · 2023-09-01T18:18:33Z

llvm/docs/GitHub.rst

+your branch instead of force pushing. This makes it easier for GitHub to
+track the context of previous review comments. Consider using the
+`built-in support for fixups <https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupamendrewordltcommitgt>`_
+in git.


How is this making a difference for the PR / GitHub? Any link to GitHub doc?

Ping on this @tra : I'm happy to help addressing post-review comment, but here I don't know why we have this?

Ping on this @tra : I'm happy to help addressing post-review comment, but here I don't know why we have this?

I think @joker-eph meant to ping @tru

Yes, sorry @tra ! Thanks for redirecting.

joker-eph · 2023-09-01T18:21:00Z

llvm/docs/GitHub.rst

+In general, you should avoid rebasing a Pull Request and force pushing to the
+branch that's the root of the Pull Request during the review. This action will
+make the context of the old changes and comments harder to find and read.


I don't think this is a good advice here, unless you explicitly say: "you should merge origin/main into your branch regularly".

Otherwise you may run into conflicts, or mid-air collision: any ninja check you run locally isn't testing against the latest main either.
It's not even clear what the pre-merge CI would test: would it be an outdated version of the code?

joker-eph · 2023-09-01T18:22:56Z

llvm/docs/GitHub.rst

+  # When your PR is accepted, you can now rebase it and make sure
+  # you have all the latest changes.
+  git rebase -i origin/main


It does not change anything about merge conflict (if the PR can be merged by Github, there isn't any), not rebasing affect the kind of test you're running locally!
(that said we're having pre-merge CI? It may do the rebase/merge when it runs?)

joker-eph · 2023-09-01T18:24:37Z

llvm/docs/GitHub.rst

+  git rebase -i origin/main
+
+  # Now merge it
+  gh pr merge --squash --delete


Does gh pr merge asks interactively about the resulting commit message? I think when I click "squash and merge" in the UI it pops-up and asks.

Our settings should be (hopefully) to use the PR description, but that would mean that amending a commit locally and setting a commit message would be ignored by gh pr merge ...

tru · 2023-09-01T19:52:46Z

Heads up - I have no time to work on this during the weekend. So either someone can merge this now and we open up more PRs for further discussion and changes or we merge this next week. I have no strong feeling either way.

joker-eph · 2023-09-01T23:32:43Z

Thanks @tru, it's fine to follow-up on my comments and others after the week-end: having Phabricator still documented as "the way to go" is worse than having an imperfect GitHub documentation in the meantime.

llvm/docs/GitHub.rst

llvm/docs/GettingInvolved.rst

llvm/docs/GitHub.rst

nickdesaulniers · 2023-09-03T05:32:37Z

llvm/docs/Contributing.rst

-  # Check that the list of commits about to be pushed is correct.
-  % git log origin/main...HEAD --oneline
-  # Push to Github.
-  % git push origin HEAD:main


L165 below talks about a pre-push hook; but the diff removed above removes the recommended workflow about git push and the new Github doc recommends using gh pr merge.

Two thoughts:

Does the pre-push hook still run if using the gh CLI? If not, we might need to clean that up.

Can we link to the Github page's section on merging "Landing your change" from this page's "For developers to commit changes from Git" section?

hubert-reinterpretcast · 2023-09-08T03:07:44Z

llvm/docs/GitHub.rst

+`Interactive rebase <https://git-scm.com/docs/git-rebase#_interactive_mode>`_
+with fixup's. This is the recommended method since you can control the final
+commit message and inspect that the final commit looks as you expect. When
+your local state is correct, remember to force-push to your branch and press


This method does not insert the PR number into the commit subject line (unlike the other method). I thought having the PR number is considered best practice.

This is still pushing to a fork, the sentence ends with "press the merge button".

What's misleading here is that it makes you believe the commit message you have locally matters: it won't, the message is taken from the PR description (and includes the PR number)

Ah. Maybe this should say force-push to your branch and then push to main (because having control of the commit message locally can be valuable). I swear I had GitHub discard edits I made to the commit message in the web UI when "merging" the PR.

I still need to fix up this documentation. Hope to have time / energy soon :)

PR number into the commit subject line

Is this something we agreed to do / discussed? IMO this pollutes the log --one-line output, but a link at the end of the commit message (not title!) is very valuable (like what we get with arc diff).

It's just what GitHub does - automatically inserts it at the end of the line.

hubert-reinterpretcast · 2023-09-08T03:09:57Z

llvm/docs/GitHub.rst


-If you do this, you must squash and merge before committing and
+If you do this, you must squash and merge before landing the PR and
 you must use the pull request title and description as the commit message.


What about the PR number? This can be read as actively disallowing the inclusion of the PR number.

hubert-reinterpretcast · 2023-09-08T03:12:04Z

llvm/docs/GitHub.rst

+branch that's the root of the Pull Request during the review. This action will
+make the context of the old changes and comments harder to find and read.
+
+Sometimes, a rebase might be needed to update your branch with a fix for a test


Doesn't merging the updated state of main into your branch work too (and not require a force push)?

This is something I asked in https://reviews.llvm.org/D150594 and we didn't resolve about which way we should advise this!

bjope · 2023-09-08T12:16:28Z

llvm/docs/GitHub.rst

+
+  # When your PR is accepted, you can now rebase it and make sure
+  # you have all the latest changes.
+  git rebase -i origin/main


Since "gh" is changing the remotes ("origin" is made pointing at the fork) I had assumed that this step would involving "git rebase -i upstream/main" to rebase to the latest from "llvm" and not my fork. To allow running tests and ensure that my patch is working with top-of-tree (that might impact what I want to say in the commit message etc).

(Is it perhaps possible that someone else could have edited my pull request so that I want to fetch things from my fork as well?)

I think 6b6312b should make it clear

This adds first version of a GitHub workflow in the documentation and marks some sections as deprecated. We should clean up these sections ASAP. I was just keen to get something on the documentation site as soon as possible.

nickdesaulniers · 2023-09-13T15:57:52Z

Follow up on how to rebase when git pr merge fails: #66223 RFC

[Doc] Update documentation for the new GitHub workflow

ceaa087

This describes the GitHub workflow in the documentation and marks some sections as deprecated. We should clean up these sections ASAP.

tru requested review from tstellar, nickdesaulniers and kbeyls September 1, 2023 10:34

tru self-assigned this Sep 1, 2023

PiotrZSL reviewed Sep 1, 2023

View reviewed changes

llvm/docs/Contributing.rst Outdated Show resolved Hide resolved

mstorsjo reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GettingInvolved.rst Show resolved Hide resolved

PiotrZSL reviewed Sep 1, 2023

View reviewed changes

jh7370 reviewed Sep 1, 2023

View reviewed changes

bjope reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GitHub.rst Outdated Show resolved Hide resolved

mstorsjo reviewed Sep 1, 2023

View reviewed changes

zero9178 reviewed Sep 1, 2023

View reviewed changes

felipepiovezan reviewed Sep 1, 2023

View reviewed changes

karka228 reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GitHub.rst Show resolved Hide resolved

kbeyls reviewed Sep 1, 2023

View reviewed changes

pogo59 reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GitHub.rst Outdated Show resolved Hide resolved

llvm/docs/GitHub.rst Outdated Show resolved Hide resolved

llvm/docs/GitHub.rst Show resolved Hide resolved

Code review updates.

13e01d2

Clarified how to use squash and merge and other smaller fixes.

Another rephrase

45a213d

unterumarmung reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GitHub.rst Outdated Show resolved Hide resolved

bjope reviewed Sep 1, 2023

View reviewed changes

tru added 2 commits September 1, 2023 16:23

Added reminders about running tests, and links to other relevant docu…

1363885

…mentation

Add paragraph about keeping one pr == one commit

148e4f7

sdesmalen-arm reviewed Sep 1, 2023

View reviewed changes

llvm/docs/GitHub.rst Show resolved Hide resolved

joker-eph reviewed Sep 1, 2023

View reviewed changes

joker-eph merged commit 63b2911 into llvm:main Sep 1, 2023
1 check failed

tru deleted the github_docs_update branch September 2, 2023 07:51

Skgland mentioned this pull request Sep 2, 2023

Tracking Issue for merging to upstream (LLVM-57) espressif/llvm-project#4

Open

nickdesaulniers reviewed Sep 3, 2023

View reviewed changes

hubert-reinterpretcast reviewed Sep 8, 2023

View reviewed changes

bjope reviewed Sep 8, 2023

View reviewed changes

[Docs] Update documentation for the new GitHub workflow #65162

[Docs] Update documentation for the new GitHub workflow #65162

Conversation

tru commented Sep 1, 2023

PiotrZSL left a comment

Choose a reason for hiding this comment

jh7370 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero9178 Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbeyls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tru commented Sep 1, 2023

tru commented Sep 1, 2023

pogo59 left a comment

Choose a reason for hiding this comment

tru commented Sep 1, 2023

jh7370 commented Sep 1, 2023

tru commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh7370 commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tru commented Sep 1, 2023

joker-eph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tra Sep 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tru commented Sep 1, 2023

joker-eph commented Sep 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan Sep 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero9178 Sep 1, 2023 •

edited

felipepiovezan Sep 1, 2023 •

edited

felipepiovezan Sep 4, 2023 •

edited

tra Sep 8, 2023 •

edited

joker-eph commented Sep 1, 2023 •

edited

felipepiovezan Sep 8, 2023 •

edited