Targets' add_target(s) methods do not correctly check passed paths

[lukpueh]

Description of issue or feature request:

Judging from the docstrings and comments, add_target and add_targets are expected to update a Targets object with passed target paths, if they are relative to a targets directory base path (i.e. self._targets_directory).

However, they don't seem to take into account that Python's os.path.join ignores all path component arguments prior to a component that looks like an absolute path.

Current behavior:

• Works as expected (?) if passed target path is relative to a targets directory base path or absolute but falls into the targets directory:

# repo._targets_directory == "/path/to/repo/targets"
repo.targets.add_target("foo.txt")
repo.targets.add_target("/path/to/repo/targets/bar.txt)
# Successfully adds "foo.txt" and "bar.txt" 

• Fails if the passed target path is not absolute and does not exist relative to targets directory base path "/path/to/repo/targets".

# repo._targets_directory == "/path/to/repo/targets"
repo.targets.add_target("repo/targets/foo.txt")
# Fails with "'/path/to/repo/targets/repo/targets/foo.txt' is not a valid file in the repository's targets directory"

• Fails if the passed target path is absolute and does not exist

#repo._targets_directory == "/path/to/repo/targets"
repo.targets.add_target("/path/does/not/exist/foo.txt")
# Fails with "'/path/does/not/exist/foo.txt' is not a valid file in the repository's targets directory"

• Has unexpected behavior if the passed path is absolute and exists!

Expected behavior:
Revise intended behavior, update implementation accordingly.

[lukpueh]

IIUC target files don't get hashed right away, i.e. on add_target(). This happens when generating and writing targets metadata to disk, e.g. via writeall(). So why not completely ignore the notion of files on disk until then?

add_target could just add any paths passed to the internal representation of targets metadata, checking that they are relative (and maybe don't contain ".."?). And only later, when the corresponding files are actually needed, i.e. to generate hashes, should it fail gracefully if any of the paths can't be resolved into a file relative to the targets directory base path.

[joshuagl]

A possible disadvantage of this proposal may be that it's harder to debug if the errors come at a seemingly unrelated call site. Path related failures when you add those paths in add_target() feels a lot more intuitive than path related failures at a later time.

Is it safe to infer from the issue report that all four of the current behaviour examples should succeed? I'd be happy to have a go at implementing a fix for this.

Aside: I've worked on projects in the past where we had to implement an alternative path joining function, due to the / related behaviour of os.path.join(). That might be a way to go here, perhaps a helper in SSL? I'll need to look at the current code some before committing to that, though.

[lukpueh]

I agree with your debugging concern, @joshuagl. I am concerned with least surprise. :)

To me, and this should answer your other question, none of above cases seems fully surprise-free. Here are some concerns:

• Why can I pass an absolute path, if the spec says that targetpaths should be relative?
• Why should I pass an absolute path, if it must be relative to an "unknown" base path (i.e. self._targets_directory), whose portion in the path gets stripped anyway?
• Why must a relative path be relative to an "unknown" base path, and not relative to my cwd?

[joshuagl]

add_target could just add any paths passed to the internal representation of targets metadata, checking that they are relative (and maybe don't contain ".."?). And only later, when the corresponding files are actually needed, i.e. to generate hashes, should it fail gracefully if any of the paths can't be resolved into a file relative to the targets directory base path.

This feels like a sensible approach, that handles @lukpueh's least-surprise concerns above and we can address my debugability concern with appropriate logging.

[lukpueh]

For consistency we should adopt whatever behavior we want for add_target and add_targets to other methods of repository_tool.Targets, that allow adding target files, i.e.:

• delegate
• delegate_hashed_bin (coordinate with delegate_hashed_bins must hash a target path as it appears in metadata #995)
• add_target_to_bin and (oddly) remove_target_from_bin (coordinate with Refactor add_target_to_bin and remove_target_from_bin #994).

[lukpueh]

Fixed in #1008