RE: Gerrit REST API Diff

[Pursehouse, David (Sony Mobile)]

> I'm not seeing consistent results across other teams instances, namely AOSP and Gerrit-Review
>
It doesn't work for me either on those sites. You'll have to wait for one of the Googlers to reply about that.

> Should I not be relying on this method to get diffs of patches?
> I know the revision based APIs for getting file diffs are coming but will that @Depreciate /revisions/current/patch ?
>
As far as I know this API will not be deprecated by the file diffs API.

> I can get a return our instance AOKP (gerrit.aokp.co; running 2.7-rc0-50-gd835996).
>

Note that in the head of master, the API has been changed to return base64 encoded data. You seem to be running a version that doesn't have that change.

When you upgrade to the latest you'll need to change the command to:

$ curl http://review/changes/1234/latest/patch | base64 -d

[Shawn Pearce]

On Mon, May 20, 2013 at 1:07 AM, Pursehouse, David (Sony Mobile)
<David.Pu...@sonymobile.com> wrote:
>> I'm not seeing consistent results across other teams instances, namely AOSP and Gerrit-Review
>>
> It doesn't work for me either on those sites. You'll have to wait for one of the Googlers to reply about that.

Confirmed it doesn't work in our build. I suspect there is a bug on
master, and that /patch is broken. I haven't confirmed it yet.

>> Should I not be relying on this method to get diffs of patches?
>> I know the revision based APIs for getting file diffs are coming but will that @Depreciate /revisions/current/patch ?
>>
> As far as I know this API will not be deprecated by the file diffs API.

There is also a new /diff API coming in addition to /patch, see
https://gerrit.googlesource.com/gerrit/+/efe7aca78d6b319e42e221876f8cc92c7270ceba

[David Pursehouse]

On 20 May 2013 22:56, "Shawn Pearce" <s...@google.com> wrote:
>
> On Mon, May 20, 2013 at 1:07 AM, Pursehouse, David (Sony Mobile)
> <David.Pu...@sonymobile.com> wrote:
> >> I'm not seeing consistent results across other teams instances, namely AOSP and Gerrit-Review
> >>
> > It doesn't work for me either on those sites.  You'll have to wait for one of the Googlers to reply about that.
>
> Confirmed it doesn't work in our build. I suspect there is a bug on
> master, and that /patch is broken. I haven't confirmed it yet.
>

It was working fine on my test server with the latest on master earlier today...

[Shawn Pearce]

OK. I still don't see the problem, but it seems to be related to our
servlet container. Its possible RestApiServlet is doing something
slightly out of spec and its confusing the container we run Gerrit in.
We will keep poking at it.

[Jon Stanford]

Thank you for the responses guys.

@David (Sorry I meant to send this response to the group not your private email, my apologies) Thank you for confirming.  I did see the Base64 return in the docs and account for the change in my app.  Why is Base64 returned there but seemingly nowhere else? Is there a benefit to using Base64 for diffs?

@Shawn I noticed that commit and it looks like a promising alternative. However so new it is not deployed to our instance yet so I was waiting to implement its usage as viewing our instance is the primary use of the app. So the revisions/current/patch API will not depreciate only change to Base64 correct? Thank you for investigating the patch issue.

I also looked for any associated Doc changes with that commit but only found the commit message for usage (not a big deal but I thought I would point it out).

Again thank you guys for your help,

Jon

[Jon Stanford]

Update on current/patch endpoint:

I'm now getting a return from gerrit-review (2.7-rc1-280-g6e6f935) using $curl https://gerrit-review.googlesource.com/changes/gerrit~stable-2.6~Id8508819ab37cd6089df0f0158ec1ef883f224f7/revisions/current/patch

However I'm not seeing a return from AOSP (2.7-rc1-223-g40acb0f) using $curl https://android-review.googlesource.com/changes/platform%2Fframeworks%2Fopt%2Ftelephony~master~Ie99239fbe02f4ee1528c0dc6e1a1750cad404b2c/revisions/current/patch

I'm also not able to get a return from our instance AOKP (2.7-rc1-3-gbcce44d) using $curl http://gerrit.aokp.co/changes/AOKP%2Fexternal_jbirdvegas_mGerrit~master~Ia32b5f6f934b1f23aec29ea09685d6ed03b1f1c6/revisions/current/patch

I hope this helps narrow your bug hunting range

Question: I've been working on using the new DiffApi but seems I'm having some trouble getting the lines of context argument to be respected. I can get the base64 return but I'm seeing the entire file returned instead of only the changed portion + lines of context.

Example of context not being respected: curl "https://gerrit-review.googlesource.com/changes/gerrit~master~Ie14ebe062d991eb9626f7b5d78b2d193c1bcb33f/revisions/ad939136d58e57ae519ae0826829ee229beca060/files/gerrit-httpd%2Fsrc%2Fmain%2Fjava%2Fcom%2Fgoogle%2Fgerrit%2Fhttpd%2Frpc%2Faccount%2FAccountServiceImpl.java/content/b&o=context:2" | base64 -d

I've tried endpoints content/a, content/b and content/ab with the same argument "&o=context:2" each time the entire file is returned not just the changes and context lines.

Am I calling the APIs wrong?

[Shawn Pearce]

On Fri, May 24, 2013 at 12:14 PM, Jon Stanford <jbird...@gmail.com> wrote:
> Update on current/patch endpoint:
>
> I'm now getting a return from gerrit-review (2.7-rc1-280-g6e6f935) using
> $curl
> https://gerrit-review.googlesource.com/changes/gerrit~stable-2.6~Id8508819ab37cd6089df0f0158ec1ef883f224f7/revisions/current/patch
>
> However I'm not seeing a return from AOSP (2.7-rc1-223-g40acb0f) using $curl
> https://android-review.googlesource.com/changes/platform%2Fframeworks%2Fopt%2Ftelephony~master~Ie99239fbe02f4ee1528c0dc6e1a1750cad404b2c/revisions/current/patch

As you noticed, I have not promoted the version at gerrit-review to
android-review. I think commit
2012c46e264959ad8f4f79aa1747b3c2545e178e fixed the result processing
in our servers, but this hasn't made it to android-review yet.

> I hope this helps narrow your bug hunting range
>
> Question: I've been working on using the new DiffApi but seems I'm having
> some trouble getting the lines of context argument to be respected. I can
> get the base64 return but I'm seeing the entire file returned instead of
> only the changed portion + lines of context.
>
> Example of context not being respected: curl
> "https://gerrit-review.googlesource.com/changes/gerrit~master~Ie14ebe062d991eb9626f7b5d78b2d193c1bcb33f/revisions/ad939136d58e57ae519ae0826829ee229beca060/files/gerrit-httpd%2Fsrc%2Fmain%2Fjava%2Fcom%2Fgoogle%2Fgerrit%2Fhttpd%2Frpc%2Faccount%2FAccountServiceImpl.java/content/b&o=context:2"
> | base64 -d
>
> I've tried endpoints content/a, content/b and content/ab with the same
> argument "&o=context:2" each time the entire file is returned not just the
> changes and context lines.
>
> Am I calling the APIs wrong?

Yes. You are asking for /content which returns file content. Not a
diff. I think you meant:

curl https://gerrit-review.googlesource.com/changes/gerrit~stable-2.6~Id8508819ab37cd6089df0f0158ec1ef883f224f7/revisions/current/files/ReleaseNotes%2fReleaseNotes-2.6.txt/diff?context=5

but I did notice this returns the entire file anyway. It seems context
is not being honored.

[Jon Stanford]

Update I just wanted to confirm that once we updated to include the fix (2012c46e264959ad8f4f79aa1747b3c2545e178e) I'm getting a reliable return from our instance. I've also noticed Android review has been promoted as /current/patch works there now as well.

Thank you for all the help guys! Keep up the good work.

If anyone reading this has a need to check a Gerrit instance from their phone you may want to consider using mGerrit. I'm still ironing out a few bugs but some here may find it useful.

Thanks again guys

[Martin Fick]

On Thursday, May 30, 2013 09:29:12 am Jon Stanford wrote:
> Update I just wanted to confirm that once we updated to
> include the fix
> (2012c46e264959ad8f4f79aa1747b3c2545e178e) I'm getting a
> reliable return from our instance. I've also noticed
> Android review has been promoted as /current/patch works
> there now as well.
>
> Thank you for all the help guys! Keep up the good work.
>
> If anyone reading this has a need to check a Gerrit
> instance from their phone you may want to consider using

> mGerrit<https://play.google.com/store/apps/details?id=co
> m.jbirdvegas.mgerrit>. I'm still ironing out a few bugs

> but some here may find it useful.

That's cool!!!

-Martin

[Saša Živkov]

This is cool.

You should announce it on this mailing list more prominently.

 

Thanks again guys

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en
 
---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.