Code review comment for lp:~vila/bzr/323111-orphan-non-versioned-files

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> Vincent Ladeuil <email address hidden> writes:

<snip/>

    >> ^- I don't think this check is actually correct. Since if the
    >> bzr-orphans directory already exists, you'd already have a file present
    >> that you have to watch out for conflicting with.
    >> So you need to check *both* that the path isn't going to be
    >> created (yet) and that it doesn't already exist.

    > *blink* Grr, trying to make the threads easier to review, I separated
    > the addition of _has_named_child which *does* the correct checks.

    > The correct check should be done here by calling _get_backup_name() not
    > osutils.available_backup_name().

    > Fixed in both threads.

Clarification: The code presented in the first submission wasn't
correct. When I realized that, I found _get_backup_name and from there
realized that we had some duplication and some dead code so I create the
3 threads.

So I've now fixed *this* thread to call _get_backup_name and the
following thread will deprecate it introducing tt._available_backup_name
both of them doing the right checks.

So 'fixed in both threads' means: this thread has now the correct fix
and the third one always had the correct fix.

Sorry for the confusion :-/

« Back to merge proposal