Make rebase_path("//foo", "//foo/bar/baz") return "../..".
Before this change, it returned "../../../foo" which technically
wasn't wrong, but it was ugly.
It's also slightly less code than the less general previous similar
fix https://codereview.chromium.org/2346913003
Bug: gn:166, chromium:647679
Change-Id: Ieac42c317426b86ef2a56371d8b83ce921aa31c3
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/8421
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
diff --git a/src/gn/filesystem_utils.cc b/src/gn/filesystem_utils.cc
index bc95dce..ae6d4d9 100644
--- a/src/gn/filesystem_utils.cc
+++ b/src/gn/filesystem_utils.cc
@@ -415,7 +415,7 @@
// part after it.
if (!IsSlash(path[after_common_index])) {
- // path is ${source-root}SUFIX/...
+ // path is ${source-root}SUFFIX/...
return false;
}
// A source-root relative path, The input may have an unknown number of
@@ -440,7 +440,7 @@
*dest = "//";
return true;
} else if (!IsSlash(path[source_root_len])) {
- // path is ${source-root}SUFIX/...
+ // path is ${source-root}SUFFIX/...
return false;
}
// A source-root relative path, The input may have an unknown number of
@@ -671,14 +671,16 @@
}
#endif
+ DCHECK(EndsWithSlash(dest));
std::string ret;
// Skip the common prefixes of the source and dest as long as they end in
- // a [back]slash.
+ // a [back]slash or end the string. dest always ends with a (back)slash in
+ // this function, so checking dest for just that is sufficient.
size_t common_prefix_len = 0;
size_t max_common_length = std::min(input.size(), dest.size());
- for (size_t i = common_prefix_len; i < max_common_length; i++) {
- if (IsSlash(input[i]) && IsSlash(dest[i]))
+ for (size_t i = common_prefix_len; i <= max_common_length; i++) {
+ if ((IsSlash(input[i]) || input[i] == '\0') && IsSlash(dest[i]))
common_prefix_len = i + 1;
else if (input[i] != dest[i])
break;
@@ -691,7 +693,10 @@
}
// Append any remaining unique input.
- ret.append(&input[common_prefix_len], input.size() - common_prefix_len);
+ if (common_prefix_len <= input.size())
+ ret.append(&input[common_prefix_len], input.size() - common_prefix_len);
+ else if (input.back() != '/' && !ret.empty())
+ ret.pop_back();
// If the result is still empty, the paths are the same.
if (ret.empty())
@@ -745,7 +750,7 @@
}
ret = MakeRelativePath(input_full, dest_full);
if (remove_slash && ret.size() > 1)
- ret.resize(ret.size() - 1);
+ ret.pop_back();
return ret;
}
diff --git a/src/gn/filesystem_utils_unittest.cc b/src/gn/filesystem_utils_unittest.cc
index 3d30d52..347d0fe 100644
--- a/src/gn/filesystem_utils_unittest.cc
+++ b/src/gn/filesystem_utils_unittest.cc
@@ -451,6 +451,7 @@
// Sharing prefix.
EXPECT_EQ("foo", RebasePath("//a/foo", SourceDir("//a/"), source_root));
+ EXPECT_EQ("foo", RebasePath("//a/foo", SourceDir("//a"), source_root));
EXPECT_EQ("foo/", RebasePath("//a/foo/", SourceDir("//a/"), source_root));
EXPECT_EQ("foo", RebasePath("//a/b/foo", SourceDir("//a/b/"), source_root));
EXPECT_EQ("foo/", RebasePath("//a/b/foo/", SourceDir("//a/b/"), source_root));
@@ -458,13 +459,12 @@
RebasePath("//a/b/foo/bar", SourceDir("//a/b/"), source_root));
EXPECT_EQ("foo/bar/",
RebasePath("//a/b/foo/bar/", SourceDir("//a/b/"), source_root));
-
- // One could argue about this case. Since the input doesn't have a slash it
- // would normally not be treated like a directory and we'd go up, which is
- // simpler. However, since it matches the output directory's name, we could
- // potentially infer that it's the same and return "." for this.
- EXPECT_EQ("../bar",
+ EXPECT_EQ(".",
RebasePath("//foo/bar", SourceDir("//foo/bar/"), source_root));
+ EXPECT_EQ("..",
+ RebasePath("//foo", SourceDir("//foo/bar/"), source_root));
+ EXPECT_EQ("../",
+ RebasePath("//foo/", SourceDir("//foo/bar/"), source_root));
// Check when only |input| is system-absolute
EXPECT_EQ("foo", RebasePath("/source/root/foo", SourceDir("//"),
diff --git a/src/gn/function_rebase_path.cc b/src/gn/function_rebase_path.cc
index bf7ec42..7e912aa 100644
--- a/src/gn/function_rebase_path.cc
+++ b/src/gn/function_rebase_path.cc
@@ -106,17 +106,9 @@
value, err, scope->settings()->build_settings()->root_path_utf8());
if (err->has_error())
return Value();
- // Special case:
- // rebase_path("//foo", "//bar") ==> "../foo"
- // rebase_path("//foo", "//foo") ==> "." and not "../foo"
- if (resolved_file.value() ==
- to_dir.value().substr(0, to_dir.value().size() - 1)) {
- result.string_value() = ".";
- } else {
- result.string_value() =
- RebasePath(resolved_file.value(), to_dir,
- scope->settings()->build_settings()->root_path_utf8());
- }
+ result.string_value() =
+ RebasePath(resolved_file.value(), to_dir,
+ scope->settings()->build_settings()->root_path_utf8());
}
return result;
@@ -151,12 +143,12 @@
with a double slash like "//foo/bar"), you should use the get_path_info()
function. This function won't work because it will always make relative
paths, and it needs to support making paths relative to the source root, so
- can't also generate source-absolute paths without more special-cases.
+ it can't also generate source-absolute paths without more special-cases.
Arguments
input
- A string or list of strings representing file or directory names These
+ A string or list of strings representing file or directory names. These
can be relative paths ("foo/bar.txt"), system absolute paths
("/foo/bar.txt"), or source absolute paths ("//foo/bar.txt").
