bazaar internal error if adding file in a linked directory

I've had a look at fixing this bug and put the work on a branch of mine at https://code.launchpad.net/~declanmg/bzr/264275-fix

I'll look into reorganising the code when I get the chance (am new to bzr and new to python) and adding some tests. Any feedback/advice would be great.

Added some basic test cases around this bug as bash scripts as an attachment.

Your patch is currently failing to pass the whole test suite: 7 failures to be precise.
These failures should help you go from your bash scripts to the python counterparts.
Since you need the 'ln -s' command, you can't make use of the new shell-like facility,
but since you already introduce a modification that make test fail, you should now be able to directly
write the python tests.

Your idea sounds good but your implementation is currently making the versioning of
symlinks imposible instead of only the for the objects accessed through that symlink.

Thanks for the feedback, which I am currently using to help my updated implementation. My approach will be:

For any file/directory asked to be added
  - Get the osutils.realpath() of its parent directory and ensure that this is inside [ie. osutils.is_inside()] the osutils.realpath() of the tree.basedir
  - If not then raise an ContentsUnderSymLinkedDirectoryOutsideTreeNotSupported error

Note: This alogrithm has now been implemented in my branch but I have yet to test it properly - and it seems to be raising the error twice for some unknown reason.

I think I will be able to add a proper test to the blackbox tests for this soon.

I have updated my branch and added a python test (as opposed to bash script test) to bzrlib/tests/blackbox/test_add_symlink_wip.py however I get a return code error:

AssertionError: Unexpected return code
not equal:
a = 0
b = 3

Some questions
1) Can someone please advise how to let the test know that a non-zero error code is expected?
2) I plan to move this to a Unit test + errors.py test (rather than a Blackbox test) as per the 'Testing exceptions and errors' of http://doc.bazaar-vcs.org/developers/testing.html. Is this the correct thing to do?
3) I think the fix is pretty much on the right track at the moment - checking if the the folders above (but not including) the path being bzr add'ed are symlinks to a directory not under the working copy. I would appreciate any other feedback concerning how the fix is implemented.

Please note: I have not run the full test suite yet. I will do so later today and start to look at any failures.

Thanks again for the feedback to date,
Declan

1) pass retcode=3 to your run_bzr call
2) New errors classes should have a unit test in test_errors, yes. bzrlib/tests/blackbox is the right place for tests of the command implementations in builtins.py, such as messages that are expected to be printed and how various command-line options are handled. (It is a bit confusing because that area also contains acceptance/integration tests of the full bzrlib stack of code). So if you've made the change in builtins.py, then a "blackbox" test or three is probably appropriate... but perhaps it would be better to put the fix in the core code (rather than in the command-line UI layer), and then unit test that?
3) Sorry, I haven't looked closely enough to have an opinion on this yet, but I hope my feedback on the other parts is helpful.

Thanks for doing this work!

Update
* Firstly, thanks for the feedback Andrew
* I have refined the code and added appropriate tests to test_add.py under the blackbox tests
* I chose to leave my changes at the builtins.py level (thus making the changes subject to blackbox tests rather than unit tests) as, with my current knowledge, they fit best under the tree_files_for_add() method rather than somewhere deeper in the core of bazaar. Please advise where to move code to if this is incorrect.
* I would now like to know is there anything else I need to do? Or can this changeset be formally submitted and a start made on getting this bug resolved?

What does this fix do?
* The changes on my branch prevent the add command from adding anything (files, sym-links or directories) that lives under a sym-linked folder whose target is outside the working tree.

What code changes are there?
* When adding something via the add command, we ensure that the realpath of its parent directory lives under the basedir of the repository. If not then addition is not allowed and a user-friendly error message displayed.

Some areas to focus on when reviewing these changes
* Is there anything in there that could have a detrimental impact on performance?
* Is this the correct layer of the code to target the fix (or should it be pushed deeper)?
* Are there any extra conditions I could add to correctly cater for OSes that don't support sym-links?
* Are there any other obvious test cases not covered by the blackbox tests?

What tests are included
* 3 blackbox tests have been added
  - Adding a file under a symlinked dir inside the working copy
  - Adding a file under a symlinked dir outside the working copy
  - Adding a symlink under a symlinked dir outside the working copy

> * I would now like to know is there anything else I need to do? Or can this changeset be formally submitted and a start made on getting this bug resolved?

Yes, please submit a merge proposal for this branch: <https://code.edge.launchpad.net/~declanmg/bzr/264275-fix/+register-merge>, it definitely sounds complete enough to belong in the review queue.

Thanks Andrew. I've just submitted a merge proposal as you suggested. Thanks again for the feedback.

Hi folks, i get the same bug with my repo but i see is not fixed yet, was set it in progress in 2009-11-24, and i am using 2.0 version.
Will be fix it this?

Cristian: See the merge proposal at <https://code.launchpad.net/~declanmg/bzr/264275-fix/+merge/15175> for details about where this is up to. The summary seems to be that there's a patch, but it needs some more work before it's ready for inclusion in bzr.

Hi,

I'm the person that submitted the patch. The last activity on it was just before Christmas when I submitted a request for feedback on the merge proposal (as can be seen in the last comment of the merge proposal). As far as I can remember, I don't think I got any feedback too unfortunately I got busy with other things at the time and didn't get a chance to go back to the patch and help drive towards an acceptable fix.

I'd like to return to working on it next week (if someone else wants to do it then let me know by posting below). It would be great if my last post at https://code.edge.launchpad.net/~declanmg/bzr/264275-fix/+merge/15175 could be replied to before I start working on it.

All the best,
Declan

I think Martin and John's work on #192859 and maybe #128562 have improved the situation. Using the lp:bzr/2.0 branch (where those fixes have landed, they aren't yet on trunk or the other stable branches) the 'bzr add' now gives:

 $ bzr add testdir/one
 bzr: ERROR: bzrlib.errors.PathNotChild: Path "/tmp/testdir/one" is not a child of path "/tmp/test"

 Traceback (most recent call last):
 ...

That's still a traceback, which isn't a great result, but seems like a step in the right direction: failing with a sensible (although internal) error at the point the user requested something invalid.

Unsubscribe

Hi Andrew, good to hear that the core problem leading to this is being fixed. I presume the recent work would supersede the work I did on my branch. If there still needs to be work done to fix the problem at a higher level, closer to the command line user interface, then it may be possible for me to grab some work from my branch to give the user a better error message. (I haven't done any recent work on this fix as I didn't get any answers to my last post at https://code.edge.launchpad.net/~declanmg/bzr/264275-fix/+merge/15175) Hopefully Martin and John's work will fix this bug entirely but if not then let me know and I'll check out the lp:bzr/2.0 branch and have a look.

I'm guessing Declan didn't manage to follow up on this, so it shouldn't be considered "In Progress". It can be considered "patch available that needs some tweaking".