RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs
Bengt Rutisson
bengt.rutisson at oracle.com
Wed Oct 15 08:54:26 UTC 2014
Hi again Eric,
My email below was blocked because the webrev.ksh that I attached was a
too big attachement for the open lists. Can you try to apply the diff
that inlined in the message and see if it solves your problems.
Thanks,
Bengt
On 2014-10-15 10:49, Bengt Rutisson wrote:
>
> Hi Eric,
>
> Thanks for finding this issue!
>
> John Coomes pointed out that there is a missing $ on the failing line.
> Since the current version works well on my Linux machine with my ksh
> version (sh (AT&T Research) 93u+ 2012-08-01) I can't verify the issue.
>
> Could you try out the webrev version I'm attaching to this email? It
> just adds the $ that John suggested.
>
> Here's the diff:
>
> diff --git a/webrev.ksh b/webrev.ksh
> --- a/webrev.ksh
> +++ b/webrev.ksh
> @@ -2270,7 +2270,7 @@
> while read LINE
> do
> set - $LINE
> - if [[ i -lt ${#NEXT_FILES[*]} ]]
> + if [[ $i -lt ${#NEXT_FILES[*]} ]]
> then
> NEXT_FILE=${NEXT_FILES[$i]}
> i=$(($i + 1))
>
>
> Thanks,
> Bengt
>
>
> On 2014-10-14 22:57, 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
>>
>>
>>
>> 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