On Tue, May 18, 2010 at 3:40 PM, Thomas Keller
<address@hidden> wrote:
Ok here we go - code looks very good overall, some minor questions:
1)
+ case restricted_path::required:
+ case restricted_path::excluded_required:
+ if (path_depth == 0)
+ {
+ L(FL("implicit include of nid %d path '%s'") %
current % fp);
+ return true;
+ }
+ else
+ {
+ return false;
+ }
+ return true;
If I followed the code correctly this logic is basically there to avoid
the exclusion of the root directory '', right? Would it make sense to
give this special node the correct node / path status right from the
start and therefor "move" this logic upwards? Also, the last return true
can never be reached, it should probably be
This is so that only the required parents are included but not all of their contents. Another way to think of this is that for implicit includes the depth is always 0 so that we don't end up inadvertently including more than we want to.
The last stray "return true;" shouldn't be there. I've removed it.
2) The test case restricted_diff_bug should get a better name, its no
longer a bug :)
Done.
Other than that I think its ready to get merged - I suspect Stephe has
to make his undrop command equally use explicit_includes, just like
revert, right?
Yes that's probably correct. I haven't actually looked at that code to see what it shares with revert, but it probably needs a similar restriction change. Ideally there will be a test that fails (with too much getting reverted) until it isn't including parents. It looks like undrop has not been merged yet so we can fix it before it gets merged.
I've merged the restriction changes in so we can see what needs to be done to undrop and I'll move on to the diff branch from the bugfest.
Cheers,
Derek