Re: github pull requests, comments and rebase

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 8, 2013 at 1:46 PM, Sage Weil <sage@xxxxxxxxxxx> wrote:
> On Thu, 8 Aug 2013, Loic Dachary wrote:
>> Hi Sage,
>>
>> During the discussions about continuous integration at the CDS this week ( http://youtu.be/cGosx5zD4FM?t=1h16m05s ) you mentionned that github was able to keep track of the successive versions of a pull request commit, even in the case of a rebase. I just tried the following:
>>
>> a) comment on commit associated to https://github.com/ceph/ceph/pull/455 ( both inline and at the end of the commit )
>> b) Christophe rebased the associated commit against master and git push --force to publish it
>> c) The original commit  does not show up in https://github.com/ceph/ceph/pull/455  and the new commit, result of the rebase, https://github.com/kri5/ceph/commit/051b0c3e15c98714b95fca5cb7838de9614dc8e3 does not display my inline comments.
>>
>> Do you to know a different workflow that would allow rebase and keep track of the different versions / comments of the pull request ? Something similar to what you have in gerrit with https://review.openstack.org/#/c/22708/ for example where you can see and compare the patch sets.
>
> Well, blarney.  Yeah, it is not perfect.
>
> The workflow that *does* work:
>
> - open a pull
> - comment on the rolled-up diff, e.g.
> https://github.com/ceph/ceph/pull/487/files
> - re-push teh branch
>
> and it will do a cute 'so and so commented on 3 old commits' thing.
>
> This isn't always how people like to review, sadly.  I'm hopeful that
> github will suddenly get better and this problem will go way, but
> shouldn't hold my breath.
>
> In my mind this isn't a deal-breaker... whatdoes everyone else think?

It's fairly frustrating when reviewing large branches. What I've
resorted to is doing reviews in gitk on a patch-by-patch basis and
then going and finding the line in the rolled-up diff. That does work,
but it's really awkward, especially when the full patch set includes
changes that don't fix an issue but do obscure its origin. I would
love a more robust review system but the more I hear about Gerrit the
less it sounds like I could stand that either. :/
-Greg
Software Engineer #42 @ http://inktank.com | http://ceph.com
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [CEPH Users]     [Ceph Large]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux