llvm dev - Nov 2019 - Enable Contributions Through Pull-request For LLVM

Philip Reames via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Weak -1 in general.  I'm not strongly opposed, but I see very little
> value in this migration and a lot of headache.  Others have explained
> this well, so I'll mostly skip that.  I particular dislike the assumed
> fork model; I work in patches, so that's a ton of overhead process
wise. 
Can you explain this a bit more?  The way I anticipate doing this:

1. Create fork once for all time
2. Set a remote for the fork in my local clone once for all time

while ((I am employed or financially independent) and working on LLVM)
  1. Make a commit
  2. Push to fork remote
  3. Open PR (probably right in emacs using Magit)
  4. Respond to comments, push further commits, squash as needed, etc.
  5. Declare done and request CI/merge
elihw

I would never delete the fork or the fork remote.
> One exception for me would be docs.  If we could open pull requests -
> and possibly the web-edit tool for folks with commit access? - for the
> docs subtree I could see a lot of advantage in making those easy for new
> contributors to update.  It might also be a good proving ground for the
> workflow as a whole.
That's a really great idea!

                  -David

On 11/8/19 9:29 AM, David Greene wrote:
> Philip Reames via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
>> Weak -1 in general.  I'm not strongly opposed, but I see very
little
>> value in this migration and a lot of headache.  Others have explained
>> this well, so I'll mostly skip that.  I particular dislike the
assumed
>> fork model; I work in patches, so that's a ton of overhead process
wise. 
> Can you explain this a bit more?  The way I anticipate doing this:
>
> 1. Create fork once for all time
> 2. Set a remote for the fork in my local clone once for all time
>
> while ((I am employed or financially independent) and working on LLVM)
>   1. Make a commit
>   2. Push to fork remote
>   3. Open PR (probably right in emacs using Magit)
>   4. Respond to comments, push further commits, squash as needed, etc.
>   5. Declare done and request CI/merge
> elihw
>
> I would never delete the fork or the fork remote.
In my experience, it isn't the simple workflow you've sketched where
problems arise, it's all of the cornercases. 

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. 

To be clear, my point is not that I can't do all that.  My point is
there's a lot of conceptual overhead and room for error, which doesn't
exists with the patch model.  My experience is pull requests work *very
well* for simple things, and somewhat poorly for more complicates ones. 

(Again, the above is entirely personal opinion based on my own experience.)
>
>> One exception for me would be docs.  If we could open pull requests -
>> and possibly the web-edit tool for folks with commit access? - for the
>> docs subtree I could see a lot of advantage in making those easy for
new
>> contributors to update.  It might also be a good proving ground for the
>> workflow as a whole.
> That's a really great idea!
>
>                   -David

To add to Philip's experience, and maybe I was using Github wrong somehow,
but I found that force pushing to rebase resulted in a loss of information
in the review for whatever reason (inline comments lost etc), making it
hard to trace back what happened and what still needed addressing.

On Mon, 11 Nov 2019 at 16:43, Philip Reames via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> On 11/8/19 9:29 AM, David Greene wrote:
> > Philip Reames via llvm-dev <llvm-dev at lists.llvm.org> writes:
> >
> >> Weak -1 in general.  I'm not strongly opposed, but I see very
little
> >> value in this migration and a lot of headache.  Others have
explained
> >> this well, so I'll mostly skip that.  I particular dislike the
assumed
> >> fork model; I work in patches, so that's a ton of overhead
process
> wise.
> > Can you explain this a bit more?  The way I anticipate doing this:
> >
> > 1. Create fork once for all time
> > 2. Set a remote for the fork in my local clone once for all time
> >
> > while ((I am employed or financially independent) and working on LLVM)
> >   1. Make a commit
> >   2. Push to fork remote
> >   3. Open PR (probably right in emacs using Magit)
> >   4. Respond to comments, push further commits, squash as needed, etc.
> >   5. Declare done and request CI/merge
> > elihw
> >
> > I would never delete the fork or the fork remote.
>
> In my experience, it isn't the simple workflow you've sketched
where
> problems arise, it's all of the cornercases.
>
> 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.
>
> To be clear, my point is not that I can't do all that.  My point is
> there's a lot of conceptual overhead and room for error, which
doesn't
> exists with the patch model.  My experience is pull requests work *very
> well* for simple things, and somewhat poorly for more complicates ones.
>
> (Again, the above is entirely personal opinion based on my own experience.)
>
> >
> >> One exception for me would be docs.  If we could open pull
requests -
> >> and possibly the web-edit tool for folks with commit access? - for
the
> >> docs subtree I could see a lot of advantage in making those easy
for new
> >> contributors to update.  It might also be a good proving ground
for the
> >> workflow as a whole.
> > That's a really great idea!
> >
> >                   -David
> _______________________________________________
> 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/20191111/a638d80d/attachment.html>

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