[project] Support moving projects into subdirectories
Suppose a project located in "third_party/foo" contained a directory
called "src". In this case, changing the project's path from
"third_party/foo" to "third_party/foo/src" would fail because the
destination directory already existed.
However, this move is actually safe - we're just moving the entire
directory down a level. So add a check to handle this special case.
Also add an integration test to make sure it works as intended. To get
the test to reproduce the failure, I needed to update the code that
calls `op.Test()` to actually call `Test()` on all operations in the
batch - previously it was skipping the first operation in the batch, and
so the test wasn't able to reproduce the failure because the test case
produced a batch of size one.
Change-Id: I557bdb436205ae128a2f0976d1f965e4b41c00b6
Reviewed-on: https://fuchsia-review.googlesource.com/c/jiri/+/1348544
Commit-Queue: Oliver Newman <olivernewman@google.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Reviewed-by: Erick Tryzelaar <etryzelaar@google.com>
diff --git a/integrationtests/update_test.go b/integrationtests/update_test.go
index 941cfa3..722df5a 100644
--- a/integrationtests/update_test.go
+++ b/integrationtests/update_test.go
@@ -6,6 +6,7 @@
import (
"path/filepath"
+ "strings"
"testing"
"github.com/google/go-cmp/cmp"
@@ -463,3 +464,50 @@
t.Errorf("Root project revision incorrect; want %q, got %q. Git log:\n%s", oldRev, newRev, log)
}
}
+
+// Test that it's possible to change the path of a project to a subdirectory of
+// the old path, even if that subdirectory already existed.
+func TestMovingProjectIntoSubdir(t *testing.T) {
+ t.Parallel()
+
+ aRemoteDir := t.TempDir()
+ setupGitRepo(t, aRemoteDir, map[string]any{
+ // project A contains a "src" subdirectory, which shouldn't stop us from
+ // moving project A into the "src" subdirectory of its original
+ // path.
+ "src/foo.txt": "foo\n",
+ })
+
+ bRemoteDir := t.TempDir()
+ setupGitRepo(t, bRemoteDir, map[string]any{
+ "manifest": project.Manifest{
+ Projects: []project.Project{
+ {
+ Name: "a",
+ Path: "path_to_a",
+ Remote: aRemoteDir,
+ },
+ },
+ },
+ })
+
+ root := t.TempDir()
+ jiri := jiriInit(t, root)
+ jiri("import", "manifest", bRemoteDir)
+ jiri("update")
+ checkDirContents(t, root, []string{
+ "path_to_a/src/foo.txt",
+ })
+
+ manifestPath := filepath.Join(bRemoteDir, "manifest")
+ manifestContents := readFile(t, manifestPath)
+ // Update project a's path from `path_to_a` to `path_to_a/src`, which is a
+ // directory that already exists (but as a subdirectory of project a).
+ writeFile(t, manifestPath, strings.ReplaceAll(manifestContents, "path_to_a", "path_to_a/src"))
+ runSubprocess(t, bRemoteDir, "git", "commit", "-am", "Update project a's path")
+
+ jiri("update")
+ checkDirContents(t, root, []string{
+ "path_to_a/src/src/foo.txt",
+ })
+}
diff --git a/project/operations.go b/project/operations.go
index 20e1d50..489c509 100644
--- a/project/operations.go
+++ b/project/operations.go
@@ -8,7 +8,6 @@
"fmt"
"hash/fnv"
"io"
- "io/ioutil"
"maps"
"os"
"path"
@@ -410,21 +409,44 @@
}
return fmtError(err)
}
+
if _, err := os.Stat(op.destination); err != nil {
if !os.IsNotExist(err) {
return fmtError(err)
}
- } else {
- // Check if the destination is our parent, and if we are the only child.
- // This allows `jiri` to move repositories up a directory.
- files, err := ioutil.ReadDir(op.destination)
- if err != nil {
- return fmtError(err)
- }
- if len(files) > 1 || (len(files) > 0 && filepath.Join(op.destination, files[0].Name()) != op.source) {
+ // The destination doesn't exist so the move should succeed, no further
+ // validation necessary.
+ return nil
+ }
+
+ // If we get here, it means the destination already exists. This may be
+ // acceptable under certain conditions, which we proceed to check for.
+
+ // Check if the project is being moved down a directory, i.e. the
+ // destination is a subdirectory of the source.
+ // TODO(olivernewman): This is only safe as long as the source directory
+ // doesn't contain any nested projects, since any nested projects will also
+ // be moved even if their paths were not updated. In practice it should be
+ // quite rare to change the path of a project that contains nested projects,
+ // so we don't bother handling that case.
+ if strings.HasPrefix(op.destination, op.source+string(filepath.Separator)) {
+ return nil
+ }
+
+ // Check if the project is being moved up a directory.
+ files, err := os.ReadDir(op.destination)
+ if err != nil {
+ return fmtError(err)
+ }
+ for _, file := range files {
+ // If we find any file in the destination directory besides the source
+ // directory, it's not safe to move the source directory to the
+ // destination.
+ if filepath.Join(op.destination, file.Name()) != op.source {
return fmt.Errorf("cannot move %q to %q as the destination already exists", op.source, op.destination)
}
}
+
return nil
}
diff --git a/project/project.go b/project/project.go
index c6e730f..d375c02 100644
--- a/project/project.go
+++ b/project/project.go
@@ -2619,12 +2619,14 @@
opType := fmt.Sprintf("%T", batchOps[0])
batchOps = batchOps[1:]
for len(batchOps) > 0 && opType == fmt.Sprintf("%T", batchOps[0]) {
- if err := batchOps[0].Test(jirix); err != nil {
- return err
- }
batch = append(batch, batchOps[0])
batchOps = batchOps[1:]
}
+ for _, op := range batch {
+ if err := op.Test(jirix); err != nil {
+ return err
+ }
+ }
if err := runBatch(jirix, params.GC, batch); err != nil {
return err
}
