RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs
John Coomes
John.Coomes at oracle.com
Tue Oct 14 22:23:26 UTC 2014
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.
-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