On Tue, May 18, 2010 at 3:40 PM, Thomas Keller <[email protected]> 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
_______________________________________________
Monotone-devel mailing list
[email protected]
http://lists.nongnu.org/mailman/listinfo/monotone-devel
