RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs
Bengt Rutisson
bengt.rutisson at oracle.com
Wed Oct 15 08:50:22 UTC 2014
On 2014-10-15 00:23, John Coomes wrote:
> Daniel Fuchs (daniel.fuchs at oracle.com) wrote:
>> On 10/14/14 10:57 PM, Eric McCorkle wrote:
>>> (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
>> Ahah - OK - thanks Eric - now the line numbers make sense :-)
>> I've been bitten by this kind of things before - I believe [[ ]] might
>> need to be replaced with something else.
> Here's line 2273 (plus some context):
>
> 2269 i=1;
> 2270 while read LINE
> 2271 do
> 2272 set - $LINE
> 2273 if [[ i -lt ${#NEXT_FILES[*]} ]]
> 2274 then
> 2275 NEXT_FILE=${NEXT_FILES[$i]}
>
> On line 2273, that should be $i (note the $) instead of just i.
Thanks for finding this, John!
Bengt
>
> -John
>
>> For evaluating arithmetic operations (+ - etc..) it seems better to
>> use 'expr'.
>> if [[i -lt ...]] should probably be replaced with something like
>> if [ $i -lt ... ]
>>
>> It's possible that the same kind of medicine will need to be applied
>> to i=$(($i + 1)) a few lines below - though it's not really possible
>> to tell without testing. Does the following works with your version
>> of ksh:
>>
>> ksh
>> i=1
>> i=$(($i + 1))
>> echo $i
>>
>> I see there are several occurrences of [[ ]] - not sure whether all of
>> them should be replaced... Usually string comparison don't require
>> a double [[ ]].
>>
>> Hope this helps,
>>
>> -- daniel
>>>
>>>
>>> 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