RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs

Tue Oct 14 09:05:25 UTC 2014

On 14/10/14 00:20, Jonathan Gibbons wrote:
> Bengt,
>
> There may be problems with the latest version of webrev.
>
> Eric McCorkle is reporting syntax errors from his version of ksh.
>
>   patch cdiffs udiffs sdiffs frames old new
> src/jdk.javadoc/share/classes/com/sun/tools/javadoc/SeeTagImpl.java
>                  ../../tl/make/scripts/webrev.ksh[2241]:
> build_old_new[1714]: build_old_new_mercurial[1666]: [: : arithmetic
> syntax error

Hi Jon,

These line numbers do not seem to correspond to the latest version
of webrev.ksh.

line 2241 is a comment
line 1714 is '       if [ -f $F ]; then' in build_old_new_mercurial
      (an not in build_old_new)
line 1666 is '        fi' in look_for_prog
      (an not build_old_new_mercurial)

Could you maybe check that we're speaking of the same version of
webrev.ksh?

best regards,

-- daniel

>
> -- Jon
>
> On 10/03/2014 12:58 AM, Bengt Rutisson wrote:
>>
>> On 2014-10-02 23:39, John Coomes wrote:
>>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>>> On 2014-10-02 01:08, John Coomes wrote:
>>>>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> On 10/1/14 4:27 PM, Daniel Fuchs wrote:
>>>>>>> Hi Bengt,
>>>>>>>
>>>>>>> The new version looks good to me, but I am not a reviewer
>>>>>>> fore CODE_TOOLS.
>>>>>> Thanks for looking at this Daniel!
>>>>>>
>>>>>>> Hopefully someone who is will step in :-)
>>>>>> Yes, and I am not a committer in code-tools so I need a sponsor
>>>>>> for the
>>>>>> patch too.
>>>>> Looks good to me; I'll sponsor it.  Send me (privately) the result of
>>>>> "hg export --git" and I'll push that.
>>>> Thanks for looking at this, John! And thanks for sponsoring the change.
>>>> I'll send you the export in a separate email.
>>> Got it; it's been pushed.
>>
>> Thanks for pushing this, John!
>>
>> Bengt
>>
>>>
>>> -John
>>>
>>>>>>> best regards,
>>>>>>>
>>>>>>> -- daniel
>>>>>>>
>>>>>>> On 01/10/14 15:57, Bengt Rutisson wrote:
>>>>>>>> Hi Daniel,
>>>>>>>>
>>>>>>>> Thanks for looking at this!
>>>>>>>>
>>>>>>>> On 2014-10-01 15:53, Daniel Fuchs wrote:
>>>>>>>>> Hi Bengt,
>>>>>>>>>
>>>>>>>>> This is a really cool feature!
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>> I have downloaded your patch and applied it in my
>>>>>>>>> local webrev repo - and played with the new
>>>>>>>>> webrev in a couple of repositories where I have
>>>>>>>>> pending uncommitted changes.
>>>>>>>>>
>>>>>>>>> This seems to work well.
>>>>>>>>>
>>>>>>>>> I have used:
>>>>>>>>>
>>>>>>>>>     webrev -fN from top level repo
>>>>>>>>>     webrev -N  from jdk repo
>>>>>>>>>     webrev -fN from jdk repo (with changes in test/closed)
>>>>>>>>>
>>>>>>>> Great that you tired it out.
>>>>>>>>
>>>>>>>>> I also had a cursory look at your patch. The only thing
>>>>>>>>> I noticed is that 'typeset' is not consistently used in
>>>>>>>>> the original webrev file. I'm not sure whether that's
>>>>>>>>> intended or not.
>>>>>>>>> I believe that's ok - but I've been bitten by not using
>>>>>>>>> typeset before ;-( - maybe - for the sake of uniformity -
>>>>>>>>> you could use typeset at least in those functions that
>>>>>>>>> already use  it for their parameters? For instance
>>>>>>>>> lines 532-534, and possibly line 560 as well?
>>>>>>>>> Or did you intentionally not used it?
>>>>>>>> I also noticed the inconsistent use of typeset. I fixed the
>>>>>>>> lines you
>>>>>>>> pointed out. I wasn't sure which way to go. But I think your
>>>>>>>> suggestion
>>>>>>>> is good.
>>>>>>>>
>>>>>>>>> Also you should increment the version of webrev.
>>>>>>>>> IIRC that would be 25.6:
>>>>>>>>>
>>>>>>>>> - WEBREV_UPDATED=25.5-hg+openjdk.java.net
>>>>>>>>> + WEBREV_UPDATED=25.6-hg+openjdk.java.net
>>>>>>>> Thanks, didn't think of that.
>>>>>>>>
>>>>>>>> New webrev:
>>>>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.01/
>>>>>>>>
>>>>>>>> Diff compared to the first version:
>>>>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00-01.diff/
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Bengt
>>>>>>>>
>>>>>>>>> best regards,
>>>>>>>>>
>>>>>>>>> -- daniel
>>>>>>>>>
>>>>>>>>> On 01/10/14 10:56, Bengt Rutisson wrote:
>>>>>>>>>> Hi everyone,
>>>>>>>>>>
>>>>>>>>>> (This is a patch for webrev, but I am cross posting to the JDK
>>>>>>>>>> 9 dev
>>>>>>>>>> list since I'm not sure how many are on the webrev-dev list and
>>>>>>>>>> most JDK
>>>>>>>>>> developers might have opinions about this change.)
>>>>>>>>>>
>>>>>>>>>> Can I get a couple of reviews for this small enhancement to
>>>>>>>>>> the webrev
>>>>>>>>>> tool?
>>>>>>>>>>
>>>>>>>>>> CODETOOLS-7901044: Add next/prev links to navigate between diffs
>>>>>>>>>> https://bugs.openjdk.java.net/browse/CODETOOLS-7901044
>>>>>>>>>>
>>>>>>>>>> Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Background:
>>>>>>>>>> When browsing review requests that span many files it is quite
>>>>>>>>>> time
>>>>>>>>>> consuming to have to go back to the index page to view to the
>>>>>>>>>> next or
>>>>>>>>>> previous file. This patch adds links to the next and previous
>>>>>>>>>> files in
>>>>>>>>>> the diff views (including frames).
>>>>>>>>>>
>>>>>>>>>> The above webrev is generated with this new version, but since
>>>>>>>>>> it only
>>>>>>>>>> contains a single file it is not that interesting. Here is an
>>>>>>>>>> example of
>>>>>>>>>> what the new webrev looks like for a larger change (the G1 class
>>>>>>>>>> unloading change):
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/example/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Bengt
>>>>>>>>>>
>>
>