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

Tue Oct 14 19:12:52 UTC 2014

Eric,

Can you provide info on the OS and version of ksh you are using that 
caused the problems you experienced with webrev?

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