Stephen Neuendorffer via llvm-dev
2020-Sep-11 22:44 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
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. Steve On Fri, Sep 11, 2020 at 3:32 PM Hubert Tong < hubert.reinterpretcast at gmail.com> wrote:> On Fri, Sep 11, 2020 at 6:12 PM Renato Golin <rengolin at gmail.com> wrote: > >> On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> There seems to be a split between those who prefer to curate commits >>>> locally and present them in the PR (i.e. Method 3) as they are to be >>>> committed (i.e. squash/amend/etc from my workstation and push the result), >>>> and those who seem to feel that it is better to avoid "git rebase" and "git >>>> commit --amend" and let github handle rewriting the history with the commit. >>>> >>> Are these primarily developers or reviewers with these preferences? Are >>> the people using Method 3 handling inline comments diligently? Is GitHub >>> still "great" if Method 1 is the model with the least surprises? >>> >> >> I don't think there's a clear difference between developers and >> reviewers, we're all usually both at some point. I also tend to fluctuate >> between manually curating commits to not caring much at all. I think if we >> try to codify this with precision, no one will be satisfied. Worst still, a >> lot of people will end up doing "the wrong thing" by accident and we'll all >> get upset. >> >> On my current project, we all have very different views and styles, but >> we reached a common consensus: >> 1. No master rewrite, just like LLVM. This is madness, period. >> 2. No branch merge commit into master. Just like LLVM. We only thoroughly >> test the master branch. >> 3. The original author decides if they want to rebase or squash at the >> end. >> >> We're already lenient with commits and reverts, there's no point in being >> tough just because we have a tool that allows us to do so. >> >> We already require people to write decent commit messages. This would >> cover the review comments nicely: all comment reviews should be bundled in >> one last commit, with a tag like "address review comments" or whatever and >> a list of changes. >> >> Of course, if some review changes the series consistently, it could give >> rise to a few more meaningful commits, with decent messages on their own. >> No rules broken there. >> >> So, in a nutshell, review/approval like current phab, >> > Current phab has better diff-to-diff comparison than anything I've seen on > GitHub. It even deals with merge-from-master/rebase-on-master fairly well. > "Reviewing like current phab" matches "Method 1" more. > > >> restrict to rebase/squash >> > As in no final "merge". I'm fine with that. > I'll note that "rebase" is a departure from current phab in that a single > review creates multiple commits. Not that I am opposed to such commits, but > I am concerned with continual force pushes to PR branches. A "final history > clean-up" after reviews are done is okay in my book. > > >> , let people decide, continue encouraging decent commit messages and >> squashing final review changes. >> > From the above, I take it to mean that this "squashing" you mean last here > is about rewriting the history (if not using GitHub's "squash and merge") > before using GitHub's "rebase and merge"? > > >> cheers, >> --renato >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/5930a0c6/attachment.html>
David Blaikie via llvm-dev
2020-Sep-11 22:53 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
Is there any observable difference between "Squash and Merge" or "Rebase and Merge" when "enforce linear history" is enabled, then? 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. > > Steve > > On Fri, Sep 11, 2020 at 3:32 PM Hubert Tong < > hubert.reinterpretcast at gmail.com> wrote: > >> On Fri, Sep 11, 2020 at 6:12 PM Renato Golin <rengolin at gmail.com> wrote: >> >>> On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> There seems to be a split between those who prefer to curate commits >>>>> locally and present them in the PR (i.e. Method 3) as they are to be >>>>> committed (i.e. squash/amend/etc from my workstation and push the result), >>>>> and those who seem to feel that it is better to avoid "git rebase" and "git >>>>> commit --amend" and let github handle rewriting the history with the commit. >>>>> >>>> Are these primarily developers or reviewers with these preferences? Are >>>> the people using Method 3 handling inline comments diligently? Is GitHub >>>> still "great" if Method 1 is the model with the least surprises? >>>> >>> >>> I don't think there's a clear difference between developers and >>> reviewers, we're all usually both at some point. I also tend to fluctuate >>> between manually curating commits to not caring much at all. I think if we >>> try to codify this with precision, no one will be satisfied. Worst still, a >>> lot of people will end up doing "the wrong thing" by accident and we'll all >>> get upset. >>> >>> On my current project, we all have very different views and styles, but >>> we reached a common consensus: >>> 1. No master rewrite, just like LLVM. This is madness, period. >>> 2. No branch merge commit into master. Just like LLVM. We >>> only thoroughly test the master branch. >>> 3. The original author decides if they want to rebase or squash at the >>> end. >>> >>> We're already lenient with commits and reverts, there's no point in >>> being tough just because we have a tool that allows us to do so. >>> >>> We already require people to write decent commit messages. This would >>> cover the review comments nicely: all comment reviews should be bundled in >>> one last commit, with a tag like "address review comments" or whatever and >>> a list of changes. >>> >>> Of course, if some review changes the series consistently, it could give >>> rise to a few more meaningful commits, with decent messages on their own. >>> No rules broken there. >>> >>> So, in a nutshell, review/approval like current phab, >>> >> Current phab has better diff-to-diff comparison than anything I've seen >> on GitHub. It even deals with merge-from-master/rebase-on-master fairly >> well. "Reviewing like current phab" matches "Method 1" more. >> >> >>> restrict to rebase/squash >>> >> As in no final "merge". I'm fine with that. >> I'll note that "rebase" is a departure from current phab in that a single >> review creates multiple commits. Not that I am opposed to such commits, but >> I am concerned with continual force pushes to PR branches. A "final history >> clean-up" after reviews are done is okay in my book. >> >> >>> , let people decide, continue encouraging decent commit messages and >>> squashing final review changes. >>> >> From the above, I take it to mean that this "squashing" you mean last >> here is about rewriting the history (if not using GitHub's "squash and >> merge") before using GitHub's "rebase and merge"? >> >> >>> cheers, >>> --renato >>> >> _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/49ece510/attachment.html>
Hubert Tong via llvm-dev
2020-Sep-11 22:56 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Fri, Sep 11, 2020 at 6:53 PM David Blaikie <dblaikie at gmail.com> wrote:> Is there any observable difference between "Squash and Merge" or "Rebase > and Merge" when "enforce linear history" is enabled, then? >"Squash and Merge" will only generate one commit.> > 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 andMerge" retains merge commits.> >> Steve >> >> On Fri, Sep 11, 2020 at 3:32 PM Hubert Tong < >> hubert.reinterpretcast at gmail.com> wrote: >> >>> On Fri, Sep 11, 2020 at 6:12 PM Renato Golin <rengolin at gmail.com> wrote: >>> >>>> On Fri, 11 Sep 2020 at 19:32, Hubert Tong via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>>> There seems to be a split between those who prefer to curate commits >>>>>> locally and present them in the PR (i.e. Method 3) as they are to be >>>>>> committed (i.e. squash/amend/etc from my workstation and push the result), >>>>>> and those who seem to feel that it is better to avoid "git rebase" and "git >>>>>> commit --amend" and let github handle rewriting the history with the commit. >>>>>> >>>>> Are these primarily developers or reviewers with these preferences? >>>>> Are the people using Method 3 handling inline comments diligently? Is >>>>> GitHub still "great" if Method 1 is the model with the least surprises? >>>>> >>>> >>>> I don't think there's a clear difference between developers and >>>> reviewers, we're all usually both at some point. I also tend to fluctuate >>>> between manually curating commits to not caring much at all. I think if we >>>> try to codify this with precision, no one will be satisfied. Worst still, a >>>> lot of people will end up doing "the wrong thing" by accident and we'll all >>>> get upset. >>>> >>>> On my current project, we all have very different views and styles, but >>>> we reached a common consensus: >>>> 1. No master rewrite, just like LLVM. This is madness, period. >>>> 2. No branch merge commit into master. Just like LLVM. We >>>> only thoroughly test the master branch. >>>> 3. The original author decides if they want to rebase or squash at the >>>> end. >>>> >>>> We're already lenient with commits and reverts, there's no point in >>>> being tough just because we have a tool that allows us to do so. >>>> >>>> We already require people to write decent commit messages. This would >>>> cover the review comments nicely: all comment reviews should be bundled in >>>> one last commit, with a tag like "address review comments" or whatever and >>>> a list of changes. >>>> >>>> Of course, if some review changes the series consistently, it could >>>> give rise to a few more meaningful commits, with decent messages on their >>>> own. No rules broken there. >>>> >>>> So, in a nutshell, review/approval like current phab, >>>> >>> Current phab has better diff-to-diff comparison than anything I've seen >>> on GitHub. It even deals with merge-from-master/rebase-on-master fairly >>> well. "Reviewing like current phab" matches "Method 1" more. >>> >>> >>>> restrict to rebase/squash >>>> >>> As in no final "merge". I'm fine with that. >>> I'll note that "rebase" is a departure from current phab in that a >>> single review creates multiple commits. Not that I am opposed to such >>> commits, but I am concerned with continual force pushes to PR branches. A >>> "final history clean-up" after reviews are done is okay in my book. >>> >>> >>>> , let people decide, continue encouraging decent commit messages and >>>> squashing final review changes. >>>> >>> From the above, I take it to mean that this "squashing" you mean last >>> here is about rewriting the history (if not using GitHub's "squash and >>> merge") before using GitHub's "rebase and merge"? >>> >>> >>>> cheers, >>>> --renato >>>> >>> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200911/a705ceda/attachment-0001.html>
Renato Golin via llvm-dev
2020-Sep-12 00:13 UTC
[llvm-dev] [cfe-dev] Phabricator -> GitHub PRs?
On Fri, 11 Sep 2020 at 23:45, Stephen Neuendorffer < stephen.neuendorffer at gmail.com> wrote:> Neither "Squash and Merge" or "Rebase and Merge" results in a Merge commit > in the git history. >Exactly. Either is perfectly fine if we keep meaningful commits in the end.>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200912/78603350/attachment.html>