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

[Daniel Fuchs]

Hi Mike,

Thanks for restarting the  thread!

On 4/9/14 12:41 AM, Mike Duigou wrote:
> 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.

Done.

> - The detection of trees using help could be made more reliable by showconfig instead:
>
>     hg showconfig extensions | grep ^extension.trees | wc -l

Good idea. Done too. I also took the opportunity to add a warning
message when -f is used but no subtree is configured in the repository.

This could easily happen if you haven't used 'tclone' for cloning
your repository - in which case subtrees won't be configured and the
-f option will have no effect (it will not find any subtree, even
if the repository has sub repositories).

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

The new version of the webrev is there:

http://cr.openjdk.java.net/~dfuchs/webrev_CODETOOLS-7900327/webrev.03/

At the moment reviews are still attributed to duke ;-)
I believe I should list you and Jon as additional reviewers -
shouldn't I?

best regards,

-- daniel

>
> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>