FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 06-10-2010, 07:10 PM
Brad Figg
 
Default Update of "Kernel/Dev/HowtoReviewPullRequests" by brad-figg

On 06/10/2010 12:00 PM, Tim Gardner wrote:
> On 06/09/2010 02:52 PM, Ubuntu Wiki wrote:
>> Dear Wiki user,
>>
>> You have subscribed to a wiki page or wiki category on "Ubuntu Wiki"
>> for change notification.
>>
>> The following page has been changed by brad-figg:
>> http://wiki.ubuntu.com/Kernel/Dev/HowtoReviewPullRequests?action=diff&rev1=1&rev2=2
>>
>>
>> ------------------------------------------------------------------------------
>>
>>
>> Pull requests come into the Ubuntu Kernel Team mailing list all the
>> time. This write up is intended to give instruction on how to review
>> the patches from one such pull request.
>>
>> + 'Pull request email:'
>> + {{{
>> + The following changes since commit
>> 88b50ff9b578518170f285b92542619ee80465ff:
>> + Stefan Bader (1):
>> + UBUNTU: Start new release
>>
>> + are available in the git repository at:
>> +
>> + git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
>> +
>> + Brad Figg (1):
>> + Minor change for wiki example
>> +
>> + debian.master/NOTES | 5 +----
>> + 1 files changed, 1 insertions(+), 4 deletions(-)
>> + }}}
>> +
>> + 'xxx'
>> {{{
>> - cd<path to lucid kernel repo>
>> + git clone git://kernel.ubuntu.com/ubuntu/ubuntu-lucid.git
>> + cd ubuntu-lucid
>> git fetch
>> - git checkout -b dove-import origin/mvl-dove
>> + git checkout -b review-import origin/master
>> - git fetch git://eric lpxy
>> + git fetch git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
>> git reset --hard FETCH_HEAD
>> }}}
>>
>>
>
> When I review a pull request it generally goes something like this:
>
> git fetch git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
> git log -p HEAD..FETCH_HEAD
>
> If I like what I see I either cherry-pick the chosen commits, or in
> simple cases 'git reset --hard FETCH_HEAD', then push.
>
> If you're hunting for a merge base, then something is likely wrong. We
> generally are not dealing with wildly divergent trees.
>
> rtg

I wrote this up almost exactly as I got the instructions from Stephan.
Note, I'm not saying you are wrong here.

Brad
--
Brad Figg brad.figg@canonical.com http://www.canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 06-11-2010, 07:36 PM
Henrik Rydberg
 
Default Update of "Kernel/Dev/HowtoReviewPullRequests" by brad-figg

> Sure.
>
> But just that people don't think I'm talking through my hat, I'm a
> daily Git user as well as a Git developer.

Ah, then excuse my ignorance.

>> There is nothing wrong with using git reset --hard. In my experience, git forces
>> you to know what you are doing, which IMHO is a good thing. Compared to the
>> alternative, which could for instance be fuzzy merges, which puts your local
>> tree somewhere in in limbo between the origin and what you would like it to be,
>> using reset --hard gives you control. You know _exactly_ what sha1 applies to
>> your tree.
>
> But that's the problem. To use 'git reset --hard' you must know what
> you're doing, which is best left to expert Git users and not something
> you should expect the kind of people relying on a HowTo to do. Because
> "git reset --hard" is bypassing a lot of safety measures which would
> otherwise prevent you from unknowingly throwing commits away.

I see. But using a merge might still be wrong in that context, for said reasons.
Learning "git reflog" might not help there either, although that feature could
do with a bit more exposure.

>> In my mind, the "git pull" command was a mistake, since most contributors would
>> ever only want to do "git fetch" follows by "git rebase", to avoid those weird
>> merges.
>
> There is "git pull --rebase" for that if you wish, which is smart enough
> to find out the proper rebase boundaries in case you did a fetch more
> than once.
>
> I don't necessarily agree that "rebase" is always the good thing to do.
> Sometimes it is but not always.

For patch contributors, I would argue that a rebase from a feature branch or a
merge of the feature branch from a clean master branch are your only options,
wouldn't you say? For a patch collector, merge is of course more appropriate.

Anyways, sorry for the interruption.

Henrik


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 06-12-2010, 08:57 AM
Stefan Bader
 
Default Update of "Kernel/Dev/HowtoReviewPullRequests" by brad-figg

On 06/10/2010 09:00 PM, Tim Gardner wrote:
> On 06/09/2010 02:52 PM, Ubuntu Wiki wrote:
>> Dear Wiki user,
>>
>> You have subscribed to a wiki page or wiki category on "Ubuntu Wiki" for change notification.
>>
>> The following page has been changed by brad-figg:
>> http://wiki.ubuntu.com/Kernel/Dev/HowtoReviewPullRequests?action=diff&rev1=1&rev2=2
>>
>> ------------------------------------------------------------------------------
>>
>> Pull requests come into the Ubuntu Kernel Team mailing list all the time. This write up is intended to give instruction on how to review the patches from one such pull request.
>>
>> + 'Pull request email:'
>> + {{{
>> + The following changes since commit 88b50ff9b578518170f285b92542619ee80465ff:
>> + Stefan Bader (1):
>> + UBUNTU: Start new release
>>
>> + are available in the git repository at:
>> +
>> + git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
>> +
>> + Brad Figg (1):
>> + Minor change for wiki example
>> +
>> + debian.master/NOTES | 5 +----
>> + 1 files changed, 1 insertions(+), 4 deletions(-)
>> + }}}
>> +
>> + 'xxx'
>> {{{
>> - cd<path to lucid kernel repo>
>> + git clone git://kernel.ubuntu.com/ubuntu/ubuntu-lucid.git
>> + cd ubuntu-lucid
>> git fetch
>> - git checkout -b dove-import origin/mvl-dove
>> + git checkout -b review-import origin/master
>> - git fetch git://eric lpxy
>> + git fetch git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
>> git reset --hard FETCH_HEAD
>> }}}
>>
>>
>
> When I review a pull request it generally goes something like this:
>
> git fetch git://kernel.ubuntu.com/bradf/ubuntu-lucid-wiki lp666xxx
> git log -p HEAD..FETCH_HEAD
>

I hope I did not miss this being actually answered by Nicolas or others and
maybe I am wrong there, but IMO this can be wrong if the HEAD has not moved
ahead with new things since the time the branch of the pull request was done.
While the merge-base is exactly that point where the branch was made from.

> If I like what I see I either cherry-pick the chosen commits, or in
> simple cases 'git reset --hard FETCH_HEAD', then push.
>

For the same reason I would never do a git reset, because often there are new
things on master that were not there when the person doing the pull request has
done the branch.

I usually do a format-patch and re-apply after checking out the master branch,
but primarily to add sob's and acks or potentially doing some message header
cleanups.

Stefan


> If you're hunting for a merge base, then something is likely wrong. We
> generally are not dealing with wildly divergent trees.
>
> rtg


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 12:54 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org