Fangrui Song via llvm-dev
2020-Sep-12 02:34 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On 2020-09-12, Renato Golin via llvm-dev wrote:>On Fri, 11 Sep 2020 at 23:56, Hubert Tong via llvm-dev < >llvm-dev at lists.llvm.org> wrote: > >> On Fri, Sep 11, 2020 at 3:45 PM Stephen Neuendorffer via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> Just to clarify: All the LLVM incubator repositories have "enforce >>>> linear history" enabled. Neither "Squash and Merge" or "Rebase and Merge" >>>> results in a Merge commit in the git history. >>>> >>> I am not aware of/still have not experienced a case where "Rebase and >> Merge" retains merge commits. >> > >It does not. We can disable the merge commit on Github settings. Plus, the >master branch history protection will stop people from merge-commit or >force-push by hand anyway.One property of "Squash and merge" is that it will add intermediate commits as bullet points (`* `). In many cases the merger does not spend more time cleaning up the description so a commit may look like: ``` RFC: treat small negative λ as 0 for sqrt(::Hermitian) (#35057) * treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian) * typo * added tests, docs; removed rtol argument for log * don't ask for rtol so close to eps(Float64) ``` The typo and `added tests` messages should probably not be there. The circt commit demonstrates the exact same problem: https://github.com/llvm/circt/commit/9ab7d667f9ad5de06c7986a71ad5e07e19cd55e7
Stephen Neuendorffer via llvm-dev
2020-Sep-12 02:38 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Hmm, good point fangrui, I'll see what we can do to minimize that. Steve On Fri, Sep 11, 2020, 7:34 PM Fangrui Song <maskray at google.com> wrote:> On 2020-09-12, Renato Golin via llvm-dev wrote: > >On Fri, 11 Sep 2020 at 23:56, Hubert Tong via llvm-dev < > >llvm-dev at lists.llvm.org> wrote: > > > >> On Fri, Sep 11, 2020 at 3:45 PM Stephen Neuendorffer via llvm-dev < > >>> llvm-dev at lists.llvm.org> wrote: > >>> > >>>> Just to clarify: All the LLVM incubator repositories have "enforce > >>>> linear history" enabled. Neither "Squash and Merge" or "Rebase and > Merge" > >>>> results in a Merge commit in the git history. > >>>> > >>> I am not aware of/still have not experienced a case where "Rebase and > >> Merge" retains merge commits. > >> > > > >It does not. We can disable the merge commit on Github settings. Plus, the > >master branch history protection will stop people from merge-commit or > >force-push by hand anyway. > > One property of "Squash and merge" is that it will add intermediate > commits as bullet points (`* `). In many cases the merger does not spend > more time cleaning up the description so a commit may look like: > > ``` > RFC: treat small negative λ as 0 for sqrt(::Hermitian) (#35057) > > * treat small negative λ as 0 for sqrt(::Hermitian) and > log(::Hermitian) > > * typo > > * added tests, docs; removed rtol argument for log > > * don't ask for rtol so close to eps(Float64) > ``` > > The typo and `added tests` messages should probably not be there. The > circt commit demonstrates the exact same problem: > https://github.com/llvm/circt/commit/9ab7d667f9ad5de06c7986a71ad5e07e19cd55e7 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/07706348/attachment.html>
Renato Golin via llvm-dev
2020-Sep-12 13:15 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Sat, 12 Sep 2020 at 03:34, Fangrui Song <maskray at google.com> wrote:> The typo and `added tests` messages should probably not be there. The > circt commit demonstrates the exact same problem: > https://github.com/llvm/circt/commit/9ab7d667f9ad5de06c7986a71ad5e07e19cd55e7Right. The "have meaningful commit messages" requirement would encompass that, too. People should always be careful with the commit messages, regardless of how they're created. It's already common for reviewers to give hints to people how they should organise their patches and commit messages, especially if they seem new to LLVM. It's safe to assume this will continue. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200912/737bc020/attachment.html>
Daniel Martín via llvm-dev
2020-Sep-13 11:28 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Fangrui Song via cfe-dev <cfe-dev at lists.llvm.org> writes:> > One property of "Squash and merge" is that it will add intermediate > commits as bullet points (`* `). In many cases the merger does not spend > more time cleaning up the description so a commit may look like: > > ``` > RFC: treat small negative λ as 0 for sqrt(::Hermitian) (#35057) > > * treat small negative λ as 0 for sqrt(::Hermitian) and log(::Hermitian) > > * typo > > * added tests, docs; removed rtol argument for log > > * don't ask for rtol so close to eps(Float64) > ```There are some inelegant ways to workaround that problem (see, for example, https://github.com/material-foundation/github-squash-and-merge-pr-descriptions if you are using Chrome). I think I suggested GitHub some time ago that they should at least give the option to include the PR description in the squashed commit description, but I got no response yet.
Renato Golin via llvm-dev
2020-Sep-13 11:43 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Sun, 13 Sep 2020 at 12:28, Daniel Martín <mardani29 at yahoo.es> wrote:> There are some inelegant ways to workaround that problem (see, for > example, > > https://github.com/material-foundation/github-squash-and-merge-pr-descriptions > if you are using Chrome). >Don't they give you the opportunity to amend the commit message, at least? I vaguely remember it's possible. I think I suggested GitHub some time ago that they should at least> give the option to include the PR description in the squashed commit > description, but I got no response yet. >This would be a good optional feature. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200913/764d3911/attachment.html>