RFC: create-pull-request / send-pull-request updates

RFC: create-pull-request / send-pull-request updates

[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_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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [poky] RFC: create-pull-request / send-pull-request updates

[Koen Kooi]

Op 11 mei 2011, om 18:15 heeft Darren Hart het volgende geschreven:

> 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.

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

Re: RFC: create-pull-request / send-pull-request updates

[Khem Raj]

On Wed, May 11, 2011 at 9:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> 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?
>

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.

> --
> Darren Hart
> Intel Open Source Technology Center
> Yocto Project - Linux Kernel
>

Re: RFC: create-pull-request / send-pull-request updates

[Richard Purdie]

On Wed, 2011-05-11 at 10:01 -0700, Khem Raj wrote:
> On Wed, May 11, 2011 at 9:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
> >
> > 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

Re: [poky] RFC: create-pull-request / send-pull-request updates

[Darren Hart]

On 05/11/2011 09:22 AM, Koen Kooi wrote:
> 
> Op 11 mei 2011, om 18:15 heeft Darren Hart het volgende geschreven:
> 
>> 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.
> 
> 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?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: RFC: create-pull-request / send-pull-request updates

[Darren Hart]

On 05/11/2011 10:40 AM, Richard Purdie wrote:
> On Wed, 2011-05-11 at 10:01 -0700, Khem Raj wrote:
>> On Wed, May 11, 2011 at 9:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
>>>
>>> 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.

> 
> 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.


> 
> Cheers,
> 
> Richard
> 

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: RFC: create-pull-request / send-pull-request updates

[Tom Rini]

On 05/11/2011 09:15 AM, Darren Hart wrote:
> 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]
> 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]
> 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.

-- 
Tom Rini
Mentor Graphics Corporation

Re: RFC: create-pull-request / send-pull-request updates

[Otavio Salvador]

On Wed, May 11, 2011 at 13:15, Darren Hart <dvhart@linux.intel.com> wrote:
> 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.

I'd prefer to have it stored into git config backend and don't need to
retype it for every call.

-- 
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

Re: RFC: create-pull-request / send-pull-request updates

[Khem Raj]

On Wed, May 11, 2011 at 10:40 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Wed, 2011-05-11 at 10:01 -0700, Khem Raj wrote:
>> On Wed, May 11, 2011 at 9:15 AM, Darren Hart <dvhart@linux.intel.com> wrote:
>> >
>> > 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.

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


>
> Cheers,
>
> Richard
>
>