facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-11 01:07:15 +03:00
Code Issues Projects Releases Wiki Activity

eden: improve validation for eden redirect add

Browse Source 
Summary:
A user ran into an issue where some buck/build related automation had
added a redirection named `../../../Users/username/reponame/.lsp-buck-out`.

The valid name for this is `.lsp-buck-out` because redirections must always be
repo-root-relative.

This commit adds missing validation for this, and makes it slightly more
convenient for consuming tools to work with by allowing an absolute path to the
redirection to be specified.

Why not simply resolve the path from the cwd and use that?  The command has
always been defined in terms of repo-root-relative paths, and there are a
number of cases where CI and other build tools can start in unexpected
subdirectories and that means that paths like `../buck-out` can have an
ambiguous or surprising resolution.  There are tools that follow the
documentation and correctly specify repo-root-relative paths that would either
be broken outright or left with newly ambiguous behavior if we suddenly started
to accept and resolve relative paths for ourselves.

I'm not opposed to changing the behavior in that way in the future, but it
requires a bit more of a coordinated effort than I have time to wrangle right
now, so this diff is aimed at surfacing breakage rather than magically
resolving it.

This commit fixes up the behavior so that paths like `../foo` are explicitly
checked for and generate an error, but absolute paths like
`/Users/username/reponame/foo` (which are unambiguous) are now resolved to the
appropriate repo-root-relative path automatically.

That makes it a bit easier for consuming tools to swing through a behavior
change; they can now simply pass the absolute path rather than messing around
with trying relativize the paths to the repo root.

Reviewed By: kmancini

Differential Revision: D25261490

fbshipit-source-id: 1706b54fc15c2dad334cdf6c75cca5e6e44ed97a
This commit is contained in:
Wez Furlong 2020-12-04 14:24:46 -08:00 committed by Facebook GitHub Bot
parent 2bfdf6f481
commit 52076f801f
1 changed files with 72 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

72
eden/fs/cli/redirect.py
View File

@ -748,6 +748,66 @@ class FixupCmd(Subcmd):
return 0 if ok else 1
def resolve_repo_relative_path(checkout_path: Path, repo_rel_path: Path) -> Path:
"""Given a path, verify that it is an appropriate repo-root-relative path
and return the resolved form of that path.
The ideal is that they pass in `foo` and we return `foo`, but we also
allow for the path to be absolute path to `foo`, in which case we resolve
it and verify that it falls with the repo and then return the relative
path to `foo`."""
if repo_rel_path.is_absolute():
# Well, the original intent was to only interpret paths as relative
# to the repo root, but it's a bit burdensome to require the caller
# to correctly relativize for that case, so we'll allow an absolute
# path to be specified.
try:
canonical_repo_path = repo_rel_path.resolve()
return canonical_repo_path.relative_to(checkout_path)
except ValueError:
raise RuntimeError(
(
f"The redirection path `{repo_rel_path}` doesn't resolve "
f"to a path inside the repo `{checkout_path}`"
)
)
# Otherwise, the path must be interpreted as being relative to the repo
# root, so let's resolve that and verify that it lies within the repo
candidate = (checkout_path / repo_rel_path).resolve()
try:
relative = candidate.relative_to(checkout_path)
except ValueError:
raise RuntimeError(
(
f"The repo-root-relative redirection path `{repo_rel_path}` "
f"doesn't resolve to a path inside the repo `{checkout_path}`. "
"Specify either a canonical absolute path to the redirection, or "
"a canonical (without `..` components) path relative to the "
f"repository root at `{checkout_path}`."
)
)
# If the resolved and relativized path doesn't match the user-specified
# path then it means that they either used `..` or a path that resolved
# through a symlink. The former is ambiguous, especially because it likely
# implies that the user is assuming that the path is current working directory
# relative instead of repo root relative, and the latter is problematic for
# all of the usual symlink reasons.
if relative != repo_rel_path:
raise RuntimeError(
(
f"The redirection path `{repo_rel_path}` resolves to `{relative}` "
"but must be a canonical repo-root-relative path. Specify either a "
"canonical absolute path to the redirection, or a canonical "
"(without `..` components) path "
f"relative to the repository root at `{checkout_path}`."
)
)
return repo_rel_path
@redirect_cmd("add", "Add or change a redirection")
class AddCmd(Subcmd):
def setup_parser(self, parser: argparse.ArgumentParser) -> None:
@ -785,6 +845,14 @@ class AddCmd(Subcmd):
# the reasons stated in the comment below.
effective_redirs = get_effective_redirections(checkout, mtab.new())
try:
args.repo_path = str(
resolve_repo_relative_path(checkout.path, Path(args.repo_path))
)
except RuntimeError as exc:
print(str(exc), file=sys.stderr)
return 1
# Get only the explicitly configured entries for the purposes of the
# add command, so that we avoid writing out any of the effective list
# of redirections to the local configuration. That doesn't matter so
@ -837,6 +905,10 @@ class DelCmd(Subcmd):
instance, checkout, _rel_path = cmd_util.require_checkout(args, args.mount)
redirs = get_configured_redirections(checkout)
# Note that we're deliberately not using the same validation logic
# for args.repo_path that we do for the add case for now so that we
# provide a way to remove bogus redirection paths. After we've deployed
# the improved `add` validation for a while, we can use it here also.
redir = redirs.get(args.repo_path)
if redir:
redir.remove_existing(checkout)