On Sat, Feb 13, 2010 at 07:39:04PM +0100, Daniel Näslund wrote:
> Hi!
Hi,
Ccing dev@ in case anyone wants to listen to our conversation.
> In definition of patch_target_t:
>
> [[[
> /* The target path, relative to the working copy directory the
> * patch is being applied to. A patch strip count applies to this
> * and only this path. This is never NULL. */
> const char *rel_path;
>
> /* The absolute path of the target on the filesystem.
> * Any symlinks the path from the patch file may contain are resolved.
> * Is not always known, so it may be NULL. */
> char *abs_path;
> ]]]
>
> Why don't we always set the abs_path? I'm reading resolve_target_path()
> but can't find the answer.
Because we can't reliably find an abspath for something that does not exist
on disk. The patch might create a new file or try to modify a non-existent
file. If we don't have a file to stat, we cannot be sure what the abs_path
really would be. Subversion supports symlinks, so the abs_path may not always
equal the concatenation of the wc_path and the (possibly stripped) rel_path.
Imagine a patch that wants to patch src/x/passwd, and src/x in the WC being
a symlink to /etc, and the wc_path being something other than /etc.
Generally, if the target isn't somewhere inside the WC, we should skip it.
If we didn't, we could end up destroying data outside the WC, which
Subversion should never, ever, do.
The idea right now is that if abs_path isn't NULL, we are 100% sure that it
is inside the WC. Any symlinks which the wc_path as well as the rel_path
might contain are resolved. We get the abspath via svn_dirent_is_under_root().
We manually reset abs_path to NULL in case svn_dirent_is_under_root()
returns false. This way, even if there was a bug, we can't accidentally end
up using an abs_path pointing outside of the WC, since we'll just segfault
instead :) And we also set target->SKIPPED in this case, which is what
actually tells the patching code what to do with the target.
> It would be nice to know that the abs_path always existed when I'm
> trying to determine if a dir is empty.
If we've deleted something, it must have existed on disk prior to patching.
And we didn't skip it. So the code checking for deletions can assume that
target->abs_path is set.
> In case you have time to spare: My approach to the 'removing empty dirs'
> problem below as pseudocode (the function collect_deleted_targets). I've
> implemented most of it. Am I on the right track?
>
> [[[
> for each target
> Check if a target is to be deleted
>
> If none were to be deleted
> Return the original targets
Instead of doing the above checks, you could init the list of deleted items
to empty, run the code below, and then check if the list of deleted items
has any elements.
> for each target in targets
> if targets is not to be deleted
> save target in not_to_be_deleted_targets
> else
> if parent_dir is key in targets_to_be_deleted
> add target to array keyed by parent_dir in targets_to_be_deleted
Now I see what you need the dirname for. It's the key into the hash
table. That's great. If you're also using the key as the path to delete,
make sure to use the dirname of target->abs_path as key, and not e.g.
something we parsed from the patch file!
> else
> create new array and add target
> save array with key parent_dir in hashtable
>
> /* Should we sort the parent dirs in hashtable after depth so that we
> * can check the longest paths first? If not, we won't be able to
> * identify when we have removed a subdir. Consider deleting theese
> * files from the greek tree where A/B/F is already deleted:
> * A/B/lambda
> * A/B/E/{alpha,beta}
> * If we checked dir B before dir E we would not detect that B was
> * empty.
Yes, we should condense the list of deleted directories, and delete
only the top-most parent of a deleted subtree.
> *
> * We would have to change the logic of the following part too. (No
> * suggestions on how to do that here) */
>
> for each parent_dir in hashtable
> Collect children of parent_dir with svn_dir_read()
> Add missing children with svn_wc__node_get_children()
> check if the patch deletes all existing children
>
> if it does
> create parent_dir target
> add parent_dir to scratch_deleted_targets
> else
> for each target in array keyed by parent_dir
> add target to scratch_deleted_targets
>
> set targets to not_to_be_deleted_targets
> set delete_all_of_this to scratch_deleted_targets
That looks good!
Thanks,
Stefan
> ]]]
>
> cheers,
> Daniel
Received on 2010-02-14 16:40:27 CET