CODETOOLS-7900327: Change webrev to use 'trees' instead of 'forest'

[Mike Duigou]

I haven't tried the changeset (I don't use trees) but have two comments:

- The webrev version number needs to be updated. Perhaps to 26 based on this being a feature change.

- The detection of trees using help could be made more reliable by showconfig instead:

   hg showconfig extensions | grep ^extension.trees | wc -l

Perhaps Seán Coffey or one of the active users can act as actual reviewer?

Mike

On Mar 18 2014, at 06:57 , Daniel Fuchs <daniel.fuchs at oracle.com> wrote:

> Hi Jon,
> 
> On 3/18/14 2:37 AM, Jonathan Gibbons wrote:
>> Daniel,
>> 
>> There are still references to the forest extension and to fstatus in the
>> comments
>> that should probably be removed or updated.
>> 
>> For example,
>> 
>> 1365     #
>> 1366     # forest extension is still being changed. For instance the output
>> 1367     # of fstatus used to no prepend the tree path to filenames, but
>> 1368     # this has changed recently. AWK code below does try to handle
>> both
>> 1369     # cases
>> 1370     #
>> 
>> 
>> 1379     #
>> 1380     # There is a bug in the output of fstatus -aC on recent
>> versions: it
>> 1381     # inserts a space between the name of the tree and the filename
>> of the
>> 1382     # old file. e.g.:
>> 1383     #
>> 
>> Also, it looks like 1396-1411 are working around the bug described in
>> that comment.
> 
> Well - I'm not feeling bold enough to try and change that code.
> I have tested the new webrev with renamed files, both in the same dir
> and in different dirs and in subrepos, and it seems to still do
> the right thing.
> 
> Therefore - I have simply updated the comments so that someone who
> would want to attempt and cleanup this code would understand that
> part of it is historical technological debt dating from the 'forest'
> time:
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_CODETOOLS-7900327/webrev.02/
> 
> I hope that can be enough for now; if it ain't broke... ;-)
> 
> best regards,
> 
> -- daniel
> 
>> 
>> -- Jon
>> 
>> 
>> On 03/17/2014 06:26 PM, Jonathan Gibbons wrote:
>>> Daniel,
>>> 
>>> I can commit the change for you, but I'm not familiar with the webrev
>>> code
>>> so I'd like to see some general approval from folk that are.
>>> 
>>> -- Jon
>>> 
>>> On 03/17/2014 02:38 AM, Daniel Fuchs wrote:
>>>> Hi,
>>>> 
>>>> I would like to propose a patch for fixing
>>>> 
>>>> CODETOOLS-7900327: Change webrev to use 'trees' instead of 'forest'
>>>> https://bugs.openjdk.java.net/browse/CODETOOLS-7900327
>>>> 
>>>> webrev:
>>>> http://cr.openjdk.java.net/~dfuchs/webrev_CODETOOLS-7900327/webrev.00/
>>>> 
>>>> (I will need a sponsor)
>>>> 
>>>> This patch changes the behavior of the '-f' option, so that it relies
>>>> on 'trees' [1] instead of 'forest'.
>>>> For this to work your forest needs to have been configured for trees.
>>>> This happens automatically if you used 'hg tclone' to clone it,
>>>> but otherwise you can call 'hg tconfig --set --walk --depth' to
>>>> configure it (you can use 'hg tlist' to see the list of trees
>>>> that have been configured).
>>>> 
>>>> The main changes in the patch (trickiest) comes from the
>>>> fact that whereas hg f<cmd> used to print '[<relative-path>]'
>>>> hg t<cmd> now prints '[<full-path>]:'
>>>> 
>>>> 
>>>> I tested the patch with the following options:
>>>> 
>>>> [all files edited, no commit]:
>>>> 
>>>> webrev -fN
>>>> 
>>>> [all files committed, no edits]:
>>>> 
>>>> webrev -f
>>>> 
>>>> best regards,
>>>> 
>>>> -- daniel
>>>> [1] http://openjdk.java.net/projects/code-tools/trees/
>>>> 
>>>>> Hi Daniel;
>>>>> 
>>>>> Since it looks like several people have tried your patch and seem to
>>>>> find it works for them then perhaps you should just create a
>>>>> CODETOOLS issue and propose it to this list (webrev-dev) for
>>>>> sponsorship.
>>>>> 
>>>>> I haven't had a chance to try it yet but likely wouldn't have
>>>>> anything to add by my review (and I am not a committer/reviewer on
>>>>> CODETOOLS) so I would encourage you to go ahead with proposing it
>>>>> yourself.
>>>>> 
>>>>> Cheers,
>>>>> 
>>>>> Mike
>>>> 
>>>> 
>>>> 
>>> 
>> 
>