David Greene via llvm-dev
2019-Nov-11 19:30 UTC
[llvm-dev] Enable Contributions Through Pull-request For LLVM
Philip Reames <listmail at philipreames.com> writes:> Let's say I post a patch, and the (warranted) reviewer comment is that > it should be split. I push a couple new PRs, they get approved, and > landed. (All as new branches off master.) Then, I have a stale branch > (say it contains 2 commits) which needs rewritten. I can of course do > an interactive rebase, and a force push, but a) I've found interactive > rebases to be very error prone, and b) I *strongly* prefer to never be > in the habit of doing a force push. The alternative is to close the > original PR, and simply post a new one - which looses history of review.I would say that this is a case where a force push is not only ok, it's actually desirable. Every time we committed something via SVN we essentially did a rebase and force push. Currently with git we essentially do a rebase and force push for anything landing in master since we require fast-forward merges. In your scenario, you responded to review, splitting off some smaller commits. Now you want to go back and get review of the updated patch. But you have to update it in the context of those smaller commits you already made to master. This is exactly what rebase + force push is for. You're force pushing to your own fork, you are not rewriting history others will use directly. A lot of damage has been done by well-meaning people posting blog articles with rules like "never rebase" and "never force push." There's a reason those operations exist and a proper way to use them. -David
Fedor Sergeev via llvm-dev
2019-Nov-12 10:27 UTC
[llvm-dev] Enable Contributions Through Pull-request For LLVM
On 11/11/19 10:30 PM, David Greene via llvm-dev wrote:> Philip Reames <listmail at philipreames.com> writes: > >> Let's say I post a patch, and the (warranted) reviewer comment is that >> it should be split. I push a couple new PRs, they get approved, and >> landed. (All as new branches off master.) Then, I have a stale branch >> (say it contains 2 commits) which needs rewritten. I can of course do >> an interactive rebase, and a force push, but a) I've found interactive >> rebases to be very error prone, and b) I *strongly* prefer to never be >> in the habit of doing a force push. The alternative is to close the >> original PR, and simply post a new one - which looses history of review. > I would say that this is a case where a force push is not only ok, it's > actually desirable. Every time we committed something via SVN we > essentially did a rebase and force push. Currently with git we > essentially do a rebase and force push for anything landing in master > since we require fast-forward merges.The point is that while requiring rebase we do not require the force push into LLVM master, and should not require doing that since force push *is* dangerous when you consider pushing into public repository/branch. (and no, SVN workflow is not force-push, it is auto-rebase-on-push instead) I understand why Philip is nervous about having to even type the force push, since fingers can be tricky to control sometimes (at least for me...). I can see a straightforward solution for that problem though - just create an alias that encodes that force push, e.g. git update-pr pr1234 (where alias.update-pr is something like: 'pf() { push --force MyLLVMGihub HEAD:$1;}; pf' ) This way you free yourself from typing this --force thing manually and avoid accidental destructive pushes into a public repo. regards, Fedor.> > In your scenario, you responded to review, splitting off some smaller > commits. Now you want to go back and get review of the updated patch. > But you have to update it in the context of those smaller commits you > already made to master. This is exactly what rebase + force push is > for. You're force pushing to your own fork, you are not rewriting > history others will use directly. > > A lot of damage has been done by well-meaning people posting blog > articles with rules like "never rebase" and "never force push." There's > a reason those operations exist and a proper way to use them. > > -David > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Danila Malyutin via llvm-dev
2019-Nov-12 15:07 UTC
[llvm-dev] Enable Contributions Through Pull-request For LLVM
Aren't force-pushes disabled for the llvm master branch? If not, they should be. -- Danila> -----Original Message----- > From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Fedor > Sergeev via llvm-dev > Sent: Tuesday, November 12, 2019 13:28 > To: llvm-dev at lists.llvm.org > Subject: Re: [llvm-dev] Enable Contributions Through Pull-request For LLVM > > On 11/11/19 10:30 PM, David Greene via llvm-dev wrote: > > Philip Reames <listmail at philipreames.com> writes: > > > >> Let's say I post a patch, and the (warranted) reviewer comment is > >> that it should be split. I push a couple new PRs, they get approved, > >> and landed. (All as new branches off master.) Then, I have a stale > >> branch (say it contains 2 commits) which needs rewritten. I can of > >> course do an interactive rebase, and a force push, but a) I've found > >> interactive rebases to be very error prone, and b) I *strongly* > >> prefer to never be in the habit of doing a force push. The > >> alternative is to close the original PR, and simply post a new one - which > looses history of review. > > I would say that this is a case where a force push is not only ok, > > it's actually desirable. Every time we committed something via SVN we > > essentially did a rebase and force push. Currently with git we > > essentially do a rebase and force push for anything landing in master > > since we require fast-forward merges. > The point is that while requiring rebase we do not require the force push into > LLVM master, and should not require doing that since force push *is* > dangerous when you consider pushing into public repository/branch. > > (and no, SVN workflow is not force-push, it is auto-rebase-on-push instead) > > I understand why Philip is nervous about having to even type the force push, > since fingers can be tricky to control sometimes (at least for me...). > > I can see a straightforward solution for that problem though - just create an > alias that encodes that force push, e.g. > git update-pr pr1234 > (where alias.update-pr is something like: > 'pf() { push --force MyLLVMGihub HEAD:$1;}; pf' > ) > > This way you free yourself from typing this --force thing manually and avoid > accidental destructive pushes into a public repo. > > regards, > Fedor. > > > > > In your scenario, you responded to review, splitting off some smaller > > commits. Now you want to go back and get review of the updated patch. > > But you have to update it in the context of those smaller commits you > > already made to master. This is exactly what rebase + force push is > > for. You're force pushing to your own fork, you are not rewriting > > history others will use directly. > > > > A lot of damage has been done by well-meaning people posting blog > > articles with rules like "never rebase" and "never force push." > > There's a reason those operations exist and a proper way to use them. > > > > -David > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cg > > i-2Dbin_mailman_listinfo_llvm- > 2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg& > > r=YgdxWMcdqQPlU9EdetI- > xI79G7ouw9_Us0dFsZnFQYU&m=zgNhcK71xx2QMccUrPkIy3 > > jXQPJ-Hua6Jf3dhg9eRUQ&s=_69YJ6MQ0vj06r49okFg- > DHCZvNP3H851IrvMCZaFBw&e> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi- > 2Dbin_mailman_listinfo_llvm- > 2Ddev&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=YgdxWMcdqQPlU9Ede > tI-xI79G7ouw9_Us0dFsZnFQYU&m=zgNhcK71xx2QMccUrPkIy3jXQPJ- > Hua6Jf3dhg9eRUQ&s=_69YJ6MQ0vj06r49okFg- > DHCZvNP3H851IrvMCZaFBw&e=