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

Eric McCorkle eric.mccorkle at oracle.com
Tue Oct 14 20:57:16 UTC 2014 

(Sorry, I forgot to hit "reply all")

Actually, the results may be from a version I had stashed in my home dir
(which I'd forgotten about).

Here is from a fresh checkout:

$ ksh -p ../../webrev/webrev.ksh
   SCM detected: mercurial

 No outgoing, perhaps you haven't commited.
      Workspace: /usr/home/emccorkl/src/enum_convert/langtools
Compare against: ssh://hg.openjdk.java.net/jdk9/dev/langtools
      Output to: /usr/home/emccorkl/src/enum_convert/langtools/webrev
   Output Files:
../../webrev/webrev.ksh: line 2273: i: arithmetic syntax error



Version: WEBREV_UPDATED=25.6-hg+openjdk.java.net





Here is from the one in my home dir:


webrev
   SCM detected: mercurial

 No outgoing, perhaps you haven't commited.
      Workspace: /usr/home/emccorkl/src/enum_convert/langtools
Compare against: ssh://hg.openjdk.java.net/jdk9/dev/langtools
      Output to: /usr/home/emccorkl/src/enum_convert/langtools/webrev
   Output Files:
        make/build.properties
                /usr/home/emccorkl//bin/webrev[2241]:
build_old_new[1714]: build_old_new_mercurial[1666]: [: : arithmetic
syntax error



Version: WEBREV_UPDATED=25.1-hg+openjdk.java.net


So it seems the output from earlier is from the version I had stashed.
Sorry about that.

On 10/14/14 16:22, Jonathan Gibbons wrote:
> 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
>>>>>>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the jdk9-dev mailing list