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

Tue Oct 14 19:55:09 UTC 2014

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