RFC: create-pull-request / send-pull-request updates
Between myself and others, there are several outstanding proposals to modify the pull-request scripts. Patches have been sent, but nothing has been merged due to a lack of consensus. I thought I would summarize what I see to be the current weaknesses of the scripts and my proposal to address them. I would like your feedback to ensure we have tools that meet the needs of a broad user base. Once we agree, I'll be happy to write up the patches or help review those written by others.
1) send-pull-request is too aggressive with the auto-cc feature (Khem Raj)
One of the reasons I wrote these scripts was out of frustration with git-send-email which allowed for a cover letter, but didn't include all the collected addresses on it. The current script sends every message to all the collected addresses. Khem (and others) have found this behavior to be sub-optimal, if not down right annoying.
I propose it be modified to only use the addresses local to each patch for the patches and all the collected addresses only for the cover letter.
a) Do people agree with this policy? If not, and people prefer the cover-letter only be sent to the recipients specified on the command line, then this script doesn't add any real value over 'git request-pull' and 'git send-email', and users can easily wrap those on their own.
2) create-pull-request needs to facilitate the use of multiple repositories (Tom Rini)
Some folks find gitorious or github work best for their use. It is also reasonable to want to use this script with independent layers. Tom proposed a -l option to specify the PULL_URL leaving some boiler-plate text in the cover-letter for the user to populate.
This dovetails with something I've been considering. Rather than duplicate the generation of a pull request cover letter, I'd like to see us re-use the output of 'git request-pull'. This has the added benefit of sanity checking the URL and commits. It does however remove the concept of the BROWSE_URL. We could add the BROWSE_URL back for recognized locations (git.yoctoproject.org, gitorious, and github I believe).
Some have expressed a desire for the URL to be automatically discoverable. We could try and extract this information from the current branch and a remote of a specific name, with some URL rewrites to convert ssh access to generic git access. Unfortunately, this approach breaks under several conditions. I would prefer that these scripts not be tied to any particular naming conventions for the git branches or remotes.
I propose we go with something very similar to Tom's -l PULL_URL proposal and replace the cover letter generation with the output of 'git request-pull'. The PULL_URL should also be able to be specified via an environment variable. Now that we are sending patches for review and not just the pull request itself, I feel we can drop the BROWSE_URL.
3) Rely on git-send-email exclusively (Darren Hart)
When I originally wrote these scripts, not all the users were particularly familiar with git and others may have already had a local sendmail client configured. At the time I thought it prudent to decouple the mail process from git. In retrospect, this serves only to unnecessarily complicate the send script as users must all learn git to effectively work within the OE and Yocto environments. If a local MTA is available, git can be configured to use that. There is no advantage that I can see to maintain both sending mechanisms in the scripts. They add complexity and complicate debugging and maintenance.
I propose the local sendmail mechanism be removed from send-pull- request and that it rely exclusively on 'git send-email'.
3) Rewrite the scripts in python (Tom)
While I agree that anything of any significant complexity is better written in python than bash, I feel that with the above changes, the current scripts will be smaller and remain simple enough for bash to be a viable option.
I propose we leave the scripts in bash for the time being, leaving the door open to rewrite them at a later date should their complexity increase to merit the effort.
Thoughts/Comments?
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Op 11 mei 2011, om 18:15 heeft Darren Hart het volgende geschreven:
Quoted 21 lines
Between myself and others, there are several outstanding proposals to modify the pull-request scripts. Patches have been sent, but nothing has been merged due to a lack of consensus. I thought I would summarize what I see to be the current weaknesses of the scripts and my proposal to address them. I would like your feedback to ensure we have tools that meet the needs of a broad user base. Once we agree, I'll be happy to write up the patches or help review those written by others.
1) send-pull-request is too aggressive with the auto-cc feature (Khem Raj)
One of the reasons I wrote these scripts was out of frustration with git-send-email which allowed for a cover letter, but didn't include all the collected addresses on it. The current script sends every message to all the collected addresses. Khem (and others) have found this behavior to be sub-optimal, if not down right annoying.
I propose it be modified to only use the addresses local to each patch for the patches and all the collected addresses only for the cover letter.
Quoted 5 lines
a) Do people agree with this policy? If not, and people prefer the cover-letter only be sent to the recipients specified on the command line, then this script doesn't add any real value over 'git request-pull' and 'git send-email', and users can easily wrap those on their own.
Correct me if I'm wrong, but the current script pick up address from the patches inside the patches as well. The CC: list should only contain the "OE" people involved, not random kernel hackers mentioning in the patches.
regards,
Koen
Quoted 32 lines · Koen Kooi
Op 11 mei 2011, om 18:15 heeft Darren Hart het volgende geschreven:
Quoted 23 lines
Between myself and others, there are several outstanding proposals to modify the pull-request scripts. Patches have been sent, but nothing has been merged due to a lack of consensus. I thought I would summarize what I see to be the current weaknesses of the scripts and my proposal to address them. I would like your feedback to ensure we have tools that meet the needs of a broad user base. Once we agree, I'll be happy to write up the patches or help review those written by others.
1) send-pull-request is too aggressive with the auto-cc feature (Khem Raj)
One of the reasons I wrote these scripts was out of frustration with git-send-email which allowed for a cover letter, but didn't include all the collected addresses on it. The current script sends every message to all the collected addresses. Khem (and others) have found this behavior to be sub-optimal, if not down right annoying.
I propose it be modified to only use the addresses local to each patch for the patches and all the collected addresses only for the cover letter.
Quoted 5 lines
a) Do people agree with this policy? If not, and people prefer the cover-letter only be sent to the recipients specified on the command line, then this script doesn't add any real value over 'git request-pull' and 'git send-email', and users can easily wrap those on their own.
Quoted 4 lines
Correct me if I'm wrong, but the current script pick up address from the patches inside the patches as well. The CC: list should only contain the "OE" people involved, not random kernel hackers mentioning in the patches.
There are two modes here. One is where you are adding your own changes, in this case it makes sense to harvest CC's from the patches. The other is when you are pulling in patches from other sources where the authors don't want to be receiving email. In the latter case, the user must be responsible to not use the -a option (auto cc). This is a fairly unique situation, you have to be pulling from a parallel environment. Kernel to kernel, layer to layer. But if I am adding a patch to a recipe, the CC's in that patch shouldn't get picked up because they will be part of the diff (prefaced with "+ ") and not in the commit message.
Am I missing a use case?
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Quoted 81 lines · Darren Hart
Between myself and others, there are several outstanding proposals to modify the pull-request scripts. Patches have been sent, but nothing has been merged due to a lack of consensus. I thought I would summarize what I see to be the current weaknesses of the scripts and my proposal to address them. I would like your feedback to ensure we have tools that meet the needs of a broad user base. Once we agree, I'll be happy to write up the patches or help review those written by others.
1) send-pull-request is too aggressive with the auto-cc feature (Khem Raj)
One of the reasons I wrote these scripts was out of frustration with git-send-email which allowed for a cover letter, but didn't include all the collected addresses on it. The current script sends every message to all the collected addresses. Khem (and others) have found this behavior to be sub-optimal, if not down right annoying.
I propose it be modified to only use the addresses local to each patch for the patches and all the collected addresses only for the cover letter.
a) Do people agree with this policy? If not, and people prefer the cover-letter only be sent to the recipients specified on the command line, then this script doesn't add any real value over 'git request-pull' and 'git send-email', and users can easily wrap those on their own.
2) create-pull-request needs to facilitate the use of multiple repositories (Tom Rini)
Some folks find gitorious or github work best for their use. It is also reasonable to want to use this script with independent layers. Tom proposed a -l option to specify the
PULL_URLleaving some boiler-plate text in the cover-letter for the user to populate.This dovetails with something I've been considering. Rather than duplicate the generation of a pull request cover letter, I'd like to see us re-use the output of 'git request-pull'. This has the added benefit of sanity checking the URL and commits. It does however remove the concept of the
BROWSE_URL. We could add theBROWSE_URLback for recognized locations (git.yoctoproject.org, gitorious, and github I believe).Some have expressed a desire for the URL to be automatically discoverable. We could try and extract this information from the current branch and a remote of a specific name, with some URL rewrites to convert ssh access to generic git access. Unfortunately, this approach breaks under several conditions. I would prefer that these scripts not be tied to any particular naming conventions for the git branches or remotes.
I propose we go with something very similar to Tom's -l
PULL_URLproposal and replace the cover letter generation with the output of 'git request-pull'. ThePULL_URLshould also be able to be specified via an environment variable. Now that we are sending patches for review and not just the pull request itself, I feel we can drop theBROWSE_URL.3) Rely on git-send-email exclusively (Darren Hart)
When I originally wrote these scripts, not all the users were particularly familiar with git and others may have already had a local sendmail client configured. At the time I thought it prudent to decouple the mail process from git. In retrospect, this serves only to unnecessarily complicate the send script as users must all learn git to effectively work within the OE and Yocto environments. If a local MTA is available, git can be configured to use that. There is no advantage that I can see to maintain both sending mechanisms in the scripts. They add complexity and complicate debugging and maintenance.
I propose the local sendmail mechanism be removed from send-pull- request and that it rely exclusively on 'git send-email'.
3) Rewrite the scripts in python (Tom)
While I agree that anything of any significant complexity is better written in python than bash, I feel that with the above changes, the current scripts will be smaller and remain simple enough for bash to be a viable option.
Quoted 7 lines
I propose we leave the scripts in bash for the time being, leaving the door open to rewrite them at a later date should their complexity increase to merit the effort.
Thoughts/Comments?
I would suggest to alter the process a bit and get rid of the scripts completely. Patches are sent to mailing list for review once reviewed the final patches are
sent as git pull-request. It would simplify things.
Quoted 5 lines
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
Quoted 9 lines · Khem Raj
Quoted 3 lines · Darren Hart
Thoughts/Comments?
I would suggest to alter the process a bit and get rid of the scripts completely. Patches are sent to mailing list for review once reviewed the final patches are
sent as git pull-request. It would simplify things.
I'd argue that it doesn't. It just means the requests come in different formats, sometimes with key pieces of information missing which means the people trying to handle the requests (like me) get frustrated.
I find it easiest to deal with requests that have come through those scripts.
Cheers,
Richard
Quoted 10 lines · Richard Purdie
Quoted 9 lines · Khem Raj
Quoted 3 lines · Darren Hart
Thoughts/Comments?
I would suggest to alter the process a bit and get rid of the scripts completely. Patches are sent to mailing list for review once reviewed the final patches are
sent as git pull-request. It would simplify things.
I think I know where you're coming from Khem, and I don't disagree that it would simplify things for some people. However, we have a wide range of people working on the various portions of the project. The scripts offer a means of standardizing how patches are reviewed and pulled, and I think that has improved things significantly over how things were a year ago.
With the Linux kernel, the vast majority of patches are just sent to the list as email. Pull requests are typically sent from sub-maintainers. However, the poky developers have long been using a pull model for many contributors. The problem was the patches weren't ever hitting the list. This led me to write the scripts in the first place. They attempted to maintain the pull model which worked well for the maintainers while still ensuring there was easy access to the patches for review.
Quoted 7 lines
I'd argue that it doesn't. It just means the requests come in different formats, sometimes with key pieces of information missing which means the people trying to handle the requests (like me) get frustrated.
I find it easiest to deal with requests that have come through those scripts.
Obviously we need to try and make things as easy as possible for the maintainers to merge in changes. One thing I think would be painful for maintainer with the current model, is that a pull request appears (at least to me) to be the final version of a patch series, when in fact they can be the very first iteration and still require review. The pull does make it easy to do some testing of patches in addition of review though.
I can certainly see both sides to this.
Quoted 5 lines
Cheers,
Richard
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Quoted 11 lines · Richard Purdie
Quoted 9 lines · Khem Raj
Quoted 3 lines · Darren Hart
Thoughts/Comments?
I would suggest to alter the process a bit and get rid of the scripts completely. Patches are sent to mailing list for review once reviewed the final patches are
sent as git pull-request. It would simplify things.
Quoted 6 lines
I'd argue that it doesn't. It just means the requests come in different formats, sometimes with key pieces of information missing which means the people trying to handle the requests (like me) get frustrated.
I find it easiest to deal with requests that have come through those scripts.
I think review process and pull process are different. and pull follows reviews right now they are sort of mixed. There are multiple pull requests that are sent and sometimes one patch appears in multiple pull requests. Signoffs acks etc. are not collected. Thats why I was suggesting
to use emails for reviews and there will be trail and then create pull requests
Quoted 6 lines
Cheers,
Richard
Quoted 7 lines · Darren Hart
Between myself and others, there are several outstanding proposals to modify the pull-request scripts. Patches have been sent, but nothing has been merged due to a lack of consensus. I thought I would summarize what I see to be the current weaknesses of the scripts and my proposal to address them. I would like your feedback to ensure we have tools that meet the needs of a broad user base. Once we agree, I'll be happy to write up the patches or help review those written by others.
Thanks for taking this up!
[snip]
Quoted 2 lines
2) create-pull-request needs to facilitate the use of multiple repositories (Tom Rini)
So long as (a) it's supported and (b) it's easy to use, I'm fine with however you want to implement it :) Which brings me to...
[snip]
Quoted 10 lines
3) Rewrite the scripts in python (Tom)
While I agree that anything of any significant complexity is better written in python than bash, I feel that with the above changes, the current scripts will be smaller and remain simple enough for bash to be a viable option.
I propose we leave the scripts in bash for the time being, leaving the door open to rewrite them at a later date should their complexity increase to merit the effort.
To me, dealing with some sort of prefs file means non-bash. But if you can figure out everything that's needed with a little bit of asking git and a little bit of standard-shell-magic (which it sounds like you can), yeah, keeping it in bash is fine.
Signature · 2 lines
Tom Rini Mentor Graphics Corporation
Quoted 24 lines · Darren Hart
2) create-pull-request needs to facilitate the use of multiple repositories (Tom Rini)
Some folks find gitorious or github work best for their use. It is also reasonable to want to use this script with independent layers. Tom proposed a -l option to specify the
PULL_URLleaving some boiler-plate text in the cover-letter for the user to populate.This dovetails with something I've been considering. Rather than duplicate the generation of a pull request cover letter, I'd like to see us re-use the output of 'git request-pull'. This has the added benefit of sanity checking the URL and commits. It does however remove the concept of the
BROWSE_URL. We could add theBROWSE_URLback for recognized locations (git.yoctoproject.org, gitorious, and github I believe).Some have expressed a desire for the URL to be automatically discoverable. We could try and extract this information from the current branch and a remote of a specific name, with some URL rewrites to convert ssh access to generic git access. Unfortunately, this approach breaks under several conditions. I would prefer that these scripts not be tied to any particular naming conventions for the git branches or remotes.
Quoted 6 lines
I propose we go with something very similar to Tom's -l
PULL_URLproposal and replace the cover letter generation with the output of 'git request-pull'. ThePULL_URLshould also be able to be specified via an environment variable. Now that we are sending patches for review and not just the pull request itself, I feel we can drop theBROWSE_URL.
I'd prefer to have it stored into git config backend and don't need to retype it for every call.
Signature · 3 lines
Otavio Salvador O.S. Systems E-mail: [email protected] http://www.ossystems.com.br Mobile: +55 53 9981-7854 http://projetos.ossystems.com.br