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

Tue Oct 14 18:47:31 UTC 2014

On 10/14/14 11:05 AM, Daniel Fuchs wrote:
> 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?

Daniel, Thanks for pointing this out.

It also does not seem to be the lines that I changed.

Jon,  I'm running the webrev script with ksh and it seems to work fine. 
Do you know which version ksh and which platform Eric is using?

Thanks,
Bengt

>
> 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
>>>>>>>>>>>
>>>
>>
>