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

Tue Oct 14 20:22:43 UTC 2014

Eric says it was a fresh check out from the webrev repo, and for the 
rest, he says,

I'm not terribly familiar with ksh, but $ ksh -v gives the following:

version         sh (AT&T Research) 93v- 2014-06-06

The OS is Linux.

Oddly enough, it doesn't seem to negatively affect the output that I can
tell.

-- Jon

On 10/14/2014 12:55 PM, Daniel Fuchs wrote:
> On 10/14/14 9:12 PM, Jonathan Gibbons wrote:
>> Eric,
>>
>> Can you provide info on the OS and version of ksh you are using that 
>> caused the problems you experienced with webrev?
> And also the version of webrev please :-)
>
> -- daniel
>
>>
>> -- Jon
>>
>> On 10/14/2014 11:47 AM, Bengt Rutisson wrote:
>>> 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
>>>>>>>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>