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

Tue Apr 29 11:07:19 UTC 2014

Hi guys,

I discovered another issue with webrev -fN:

The AWK code that attempts to determine whether the modified/added
files are prepended with the subtree path use the following logic:

n=index($2,tree)
if (n == 0)
     printf("A %s/%s\n", tree, $2);
else
     printf("A %s\n", $2);

As I was dismayed to discover this does not work if you attempt
to generate a webrev from the root of the forest and your modification
contains a file in the jdk subtree whose name also contains the string
"jdk" (for instance, all files under src/share/classes/jdk/ fall into
that category - because in that case n=19).

The consequence is that the 'tree' path is not prepended as it should,
and the symptom is that the modified/added file can no longer be found
and doesn't get copied inside the webrev - and you get a message saying
'abort: cannot follow file not in parent revision: <file>'
(and you can see in the webrev/file.list that the file is missing its
jdk/ prefix)

An easy fix for that issue is to change

if (n == 0)

into

if (n != 1)

because what we want to check is whether the file name starts with
the tree path or not (not whether it merely contains it elsewhere)

Here is a new revision that incorporates this additional change:

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

However, I am unsure whether using 'tree' guarantees us that
the bug/feature in hg that this 'if' condition checks will no
longer ever happen, and that the subtree will never be printed
out - in which case a better fix could be to simplify the code
and assume that we will always be in the case where (n != 1)
[which is what I am observing on my machine with my version
of hg] and remove that check altogether...
I haven't felt bold enough to hack it all away - but maybe I should...

Hope this helps,

-- daniel
PS: there's a bug logged against 'abort: cannot follow file
not in parent revision: <file>' - which also happens in other
different circumstances:
https://bugs.openjdk.java.net/browse/CODETOOLS-7900880
I have added a note in that bug as well.

On 4/9/14 11:35 AM, Daniel Fuchs wrote:
> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>