[ANN] Please use Gerrit from now on for Patch Review

classic Classic list List threaded Threaded
90 messages Options
12345 « Prev
David Ostrovsky David Ostrovsky
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On 02.07.2012 18:44, Bjoern Michaelsen wrote:
On Mon, Jul 02, 2012 at 06:13:50PM +0200, Lionel Elie Mamane wrote:
When I click "Diff All Side-by-Side" (or "Diff All Unified"), it shows
> me only the first file with a link to the (diff) of the next file. I
> expected to see the diff for *all* files on one page.
There is gitweb to see all changes on one page. We might patch the diff-all
button to link there too (although you cannot do inline comments there).
done: https://gerrit-review.googlesource.com/#/c/36600/
I wonder why our gerrit version is outdated: current is 2.4.2, we have 2.3

Regards
David


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lionel Elie Mamane Lionel Elie Mamane
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Lionel Elie Mamane
On Mon, Jul 02, 2012 at 06:13:50PM +0200, Lionel Elie Mamane wrote:
> On Mon, Jun 18, 2012 at 12:09:49PM +0200, Bjoern Michaelsen wrote:

>> gerrit is documented and ready to go. Please use it for code review
>> as much as possible now as it simplifies things a lot over manual
>> patch fiddling on mailing lists.

Now I went to https://gerrit.libreoffice.org/#/c/267/; there are at
this time three patchsets, but it seems there is no easy / convenient
way to see the differences (interdiff) between them.

--
Lionel
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

diff between patch revisions (was: Please use Gerrit from now on for Patch Review)

On Sun, Jul 08, 2012 at 10:56:55PM +0200, Lionel Elie Mamane wrote:
> Now I went to https://gerrit.libreoffice.org/#/c/267/; there are at
> this time three patchsets, but it seems there is no easy / convenient
> way to see the differences (interdiff) between them.

Find the text "Old Version History:" and the drop down saying "Base" next to
it. If you select "Patch Set 1" instead, you will see the diff to the first
patch.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lionel Elie Mamane Lionel Elie Mamane
Reply | Threaded
Open this post in threaded view
|

Re: diff between patch revisions (was: Please use Gerrit from now on for Patch Review)

On Sun, Jul 08, 2012 at 11:17:31PM +0200, Bjoern Michaelsen wrote:
> On Sun, Jul 08, 2012 at 10:56:55PM +0200, Lionel Elie Mamane wrote:
>> Now I went to https://gerrit.libreoffice.org/#/c/267/; there are at
>> this time three patchsets, but it seems there is no easy / convenient
>> way to see the differences (interdiff) between them.

> Find the text "Old Version History:" and the drop down saying "Base" next to
> it. If you select "Patch Set 1" instead, you will see the diff to the first
> patch.

Ah, great, I see now. Thanks!

--
Lionel
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

In reply to this post by Lionel Elie Mamane
\> Now I went to https://gerrit.libreoffice.org/#/c/267/; there are at
> this time three patchsets, but it seems there is no easy / convenient
> way to see the differences (interdiff) between them.

QT have patched their gerrit to do the 'right thing'
See https://codereview.qt-project.org/#patch,all,25593,2
for example

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
David Ostrovsky David Ostrovsky
Reply | Threaded
Open this post in threaded view
|

Re: [ANN] Please use Gerrit from now on for Patch Review

On 09.07.2012 08:53, Norbert Thiebaud wrote:
> QT have patched their gerrit to do the 'right thing'
we too and we are even better: we pushed it upstream ;-)

https://gerrit-review.googlesource.com/#/c/36640/

Regards
David

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Jan Holesovsky Jan Holesovsky
Reply | Threaded
Open this post in threaded view
|

Patching our gerrit instance [was Re: [ANN] Please use Gerrit from now on for Patch Review]

In reply to this post by Bjoern Michaelsen
Hi Bjoern,

On 2012-07-02 at 18:44 +0200, Bjoern Michaelsen wrote:

> > When I click "Diff All Side-by-Side" (or "Diff All Unified"), it shows
> > me only the first file with a link to the (diff) of the next file. I
> > expected to see the diff for *all* files on one page. That is much
> > more convenient to check if the changes in one file match the changes
> > in another file. For example: the signature of a function is called in
> > foo.h and everywhere it is called (in bar.cxx and qux.cxx), the call
> > is adapted correctly.
>
> There is gitweb to see all changes on one page. We might patch the diff-all
> button to link there too (although you cannot do inline comments there).

The [Diff All Unified] is completely unusable at the very moment, I have
no idea who might want to see the patch split into several open
windows ;-)  Luckily, the Qt guys apparently had the same problem, and
have a solution (the entire diff on one page, without abandoning the
inline commenting):

http://qt.gitorious.org/qtqa/gerrit/commit/737400d1bad4fa8bfd39cb326636a0307014901f

So - what to do about that?  Norbert had the right concerns that we
probably shouldn't patch our instance, but do we have another
possibility?

Another thing are the mail templates - can you please commit the current
mail templates that we are using on gerrit.libreoffice.org to
dev-tools/gerrit/gerrit_site/etc/mail, so that we can tweak them [eg. to
get rid of the ..... line], in a version-controlled way, and deploy them
back there easily?

Also, can you please make

http://cgit.freedesktop.org/libreoffice/contrib/dev-tools/commit/?id=72d5bdff8d4bae14f6a63fd67d988f637b0bbc07

live on the server?

Thank you,
Kendy

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
David Ostrovsky David Ostrovsky
Reply | Threaded
Open this post in threaded view
|

Re: Patching our gerrit instance [was Re: [ANN] Please use Gerrit from now on for Patch Review]

Hi Kendy,

On 18.07.2012 19:11, Jan Holesovsky wrote:
> Another thing are the mail templates - can you please commit the current
> mail templates that we are using on gerrit.libreoffice.org to
> dev-tools/gerrit/gerrit_site/etc/mail, so that we can tweak them [eg. to
> get rid of the ..... line], in a version-controlled way
https://gerrit.libreoffice.org/#/c/328/

Regards
David
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Lionel Elie Mamane Lionel Elie Mamane
Reply | Threaded
Open this post in threaded view
|

Re: Patching our gerrit instance [was Re: [ANN] Please use Gerrit from now on for Patch Review]

On Wed, Jul 18, 2012 at 10:10:38PM +0200, David Ostrovsky wrote:

> https://gerrit.libreoffice.org/#/c/328/

The "gitweb" link on that page says "404 no such project".

--
Lionel
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: Patching our gerrit instance [was Re: [ANN] Please use Gerrit from now on for Patch Review]

In reply to this post by Jan Holesovsky
Hi,

On Wed, Jul 18, 2012 at 07:11:53PM +0200, Jan Holesovsky wrote:
> Luckily, the Qt guys apparently had the same problem, and
> have a solution (the entire diff on one page, without abandoning the
> inline commenting):
>
> http://qt.gitorious.org/qtqa/gerrit/commit/737400d1bad4fa8bfd39cb326636a0307014901f
>
> So - what to do about that?  Norbert had the right concerns that we
> probably shouldn't patch our instance, but do we have another
> possibility?

David pointed out there might be some issues with the patch IIRC and it cant be
upstreamed for licensing reasons (doh, yeah, awesome!). Since David has been
investgating there, I would trust his judgement how we can best solve this one.
@David: Any suggestion what the least sucking alternative is?

Personally, I wouldnt use that feature at all and simply navigate with the
keybindings through changes -- but as always, workflows differ.

> Another thing are the mail templates - can you please commit the current
> mail templates that we are using on gerrit.libreoffice.org to
> dev-tools/gerrit/gerrit_site/etc/mail, so that we can tweak them [eg. to
> get rid of the ..... line], in a version-controlled way, and deploy them
> back there easily?

Done (David uploaded to gerrit, I pushed).

> Also, can you please make
> http://cgit.freedesktop.org/libreoffice/contrib/dev-tools/commit/?id=72d5bdff8d4bae14f6a63fd67d988f637b0bbc07
> live on the server?

Im already in firefighting mode on the distro-level today, so I better wont
tweak on another production system. Maybe Robert or Norbert can, the thing
seems trivial enough.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
12345 « Prev