RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Oct 14 21:29:06 UTC 2014
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.
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