internal/vcs: refactor to prepare for LICENSE file inclusion

Including LICENSE files from the root of a VCS repo in the case where no
LICENSE file is present in a subdirectory module requires that we
perform all operations from the module root.

We prepare for this by making ListFiles and Status take paths instead of
implicitly operating on the module subdirectory. This allows us to
perform separate checks on the root LICENSE file if one exists.  Indeed
the concept of a VCS implementation keeping track of a subdir is
eliminated entirely.

As part of this we expand the VCS test suite to include a number of
scenarios that cover the "edges" of ListFiles and Status.

We also fix a bug in passing where git ListFiles implementation did not
correctly handle an empty result, i.e. the situation where no files
match args.

For #3130.

Signed-off-by: Paul Jolly <[email protected]>
Change-Id: Id669490a447a4f07ced611f3323868b4519b1919
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1195548
Unity-Result: CUE porcuepine <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>

cmd/cue/cmd/modpublish.go

@@ -156,7 +156,7 @@ func runModUpload(cmd *Command, args []string) error {
 		if err != nil {
 			return err
 		}
-		status, err := vcsImpl.Status(ctx)
+		status, err := vcsImpl.Status(ctx, modRoot)
 		if err != nil {
 			return err
 		}

internal/vcs/git.go

@@ -25,8 +25,7 @@ import (
 )
 
 type gitVCS struct {
-	root   string
-	subDir string
+	root string
 }
 
 func newGitVCS(dir string) (VCS, error) {
@@ -38,8 +37,7 @@ func newGitVCS(dir string) (VCS, error) {
 		}
 	}
 	return gitVCS{
-		root:   root,
-		subDir: dir,
+		root: root,
 	}, nil
 }
 
@@ -48,31 +46,40 @@ func (v gitVCS) Root() string {
 	return v.root
 }
 
-// ListFiles implements [VCS.ListFiles].
-func (v gitVCS) ListFiles(ctx context.Context, dir string) ([]string, error) {
-	rel, err := filepath.Rel(v.root, dir)
-	if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
-		return nil, fmt.Errorf("cannot list files from %q, outside VCS root %q", dir, v.root)
+// fixDir adjusts dir according to the semantics described in [VCS.ListFiles].
+func fixDir(v VCS, dir string) string {
+	if dir == "" {
+		return v.Root()
+	}
+	if !filepath.IsAbs(dir) {
+		return filepath.Join(v.Root(), dir)
 	}
+	return dir
+}
+
+// ListFiles implements [VCS.ListFiles].
+func (v gitVCS) ListFiles(ctx context.Context, dir string, paths ...string) ([]string, error) {
+	dir = fixDir(v, dir)
+
 	// TODO should we use --recurse-submodules?
-	out, err := runCmd(ctx, dir, "git", "ls-files", "-z")
+	gitargs := append([]string{"ls-files", "-z", "--"}, paths...)
+	out, err := runCmd(ctx, dir, "git", gitargs...)
 	if err != nil {
 		return nil, err
 	}
-	files := strings.Split(strings.TrimSuffix(out, "\x00"), "\x00")
+	out = strings.TrimSuffix(out, "\x00")
+	if out == "" {
+		return nil, nil
+	}
+	files := strings.Split(out, "\x00")
 	sort.Strings(files)
 	return files, nil
 }
 
 // Status implements [VCS.Status].
-func (v gitVCS) Status(ctx context.Context) (Status, error) {
-	// We only care about the module's subdirectory status - if anything
-	// else is dirty, it won't go into the module so we don't care.
-	// TODO this will change if/when we include license files
-	// from outside the module directory. It also over-reports dirtiness
-	// because there might be nested modules that aren't included, but
-	// are nonetheless included in the status check.
-	out, err := runCmd(ctx, v.root, "git", "status", "--porcelain", v.subDir)
+func (v gitVCS) Status(ctx context.Context, paths ...string) (Status, error) {
+	gitargs := append([]string{"status", "--porcelain", "--"}, paths...)
+	out, err := runCmd(ctx, v.root, "git", gitargs...)
 	if err != nil {
 		return Status{}, err
 	}

internal/vcs/vcs.go

@@ -31,17 +31,25 @@ type VCS interface {
 	// the VCS (e.g. the directory containing .git).
 	Root() string
 
-	// ListFiles returns a list of all the files tracked by the VCS
-	// under the given directory, relative to that directory, as
-	// filepaths, in lexical order. It does not include directory
-	// names.
+	// ListFiles returns a list of files tracked by VCS, rooted at dir. The
+	// optional paths determine what should be listed. If no paths are provided,
+	// then all of the files under VCS control under dir are returned. An empty
+	// dir is interpretted as [VCS.Root]. A non-empty relative dir is
+	// interpretted relative to [VCS.Root]. It us up to the caller to ensure
+	// that dir and paths are contained by the VCS root Filepaths are relative
+	// to dir and returned in lexical order.
 	//
-	// The directory should be within the VCS root.
-	ListFiles(ctx context.Context, dir string) ([]string, error)
+	// Note that ListFiles is generally silent in the case an arg is provided
+	// that does correspond to a VCS-controlled file. For example, calling
+	// with an arg of "BANANA" where no such file is controlled by VCS will
+	// result in no filepaths being returned.
+	ListFiles(ctx context.Context, dir string, paths ...string) ([]string, error)
 
-	// Status returns the current state of the repository holding
-	// the given directory.
-	Status(ctx context.Context) (Status, error)
+	// Status returns the current state of the repository holding the given paths.
+	// If paths is not provided it implies the state of
+	// the VCS repository in its entirety, including untracked files. paths are
+	// interpretted relative to the [VCS.Root].
+	Status(ctx context.Context, paths ...string) (Status, error)
 }
 
 // Status is the current state of a local repository.
@@ -57,10 +65,7 @@ var vcsTypes = map[string]func(dir string) (VCS, error){
 
 // New returns a new VCS value representing the
 // version control system of the given type that
-// controls the given directory.
-//
-// Status checks apply only to the given directory; other
-// directories controlled by the VCS will not be considered.
+// controls the given directory dir.
 //
 // It returns an error if a VCS of the specified type
 // cannot be found.

internal/vcs/vcs_test.go

@@ -44,58 +44,148 @@ func TestGit(t *testing.T) {
 	err := copyFS(dir, testFS)
 	qt.Assert(t, qt.IsNil(err))
 
-	_, err = New("git", filepath.Join(dir, "subdir"))
+	// In the tests that follow, we are testing the scenario where a module is
+	// present in $dir/subdir (the VCS is rooted at $dir). cue/load or similar
+	// would establish the absolute path $dir/subdir is the CUE module root, and
+	// as such we use that absolute path as an argument in the calls to the VCS
+	// implementation.
+	subdir := filepath.Join(dir, "subdir")
+
+	_, err = New("git", subdir)
 	qt.Assert(t, qt.ErrorMatches(err, `git VCS not found in any parent of ".+"`))
 
 	initTestEnv(t)
 	mustRunCmd(t, dir, "git", "init")
-	v, err := New("git", filepath.Join(dir, "subdir"))
+	v, err := New("git", subdir)
 	qt.Assert(t, qt.IsNil(err))
 
 	// The status shows that we have uncommitted files
 	// because we haven't yet added the files after doing
 	// git init.
-	status, err := v.Status(ctx)
+	statusuncommitted, err := v.Status(ctx, subdir)
 	qt.Assert(t, qt.IsNil(err))
-	qt.Assert(t, qt.IsTrue(status.Uncommitted))
+	qt.Assert(t, qt.IsTrue(statusuncommitted.Uncommitted))
 
 	mustRunCmd(t, dir, "git", "add", ".")
-	status, err = v.Status(ctx)
+	statusuncommitted, err = v.Status(ctx, subdir)
 	qt.Assert(t, qt.IsNil(err))
-	qt.Assert(t, qt.IsTrue(status.Uncommitted))
+	qt.Assert(t, qt.IsTrue(statusuncommitted.Uncommitted))
 
 	commitTime := time.Now().Truncate(time.Second)
 	mustRunCmd(t, dir, "git",
 		"-c", "[email protected]",
 		"-c", "user.name=cueckoo",
 		"commit", "-m", "something",
 	)
-	status, err = v.Status(ctx)
+	status, err := v.Status(ctx, subdir)
 	qt.Assert(t, qt.IsNil(err))
 	qt.Assert(t, qt.IsFalse(status.Uncommitted))
 	qt.Assert(t, qt.IsTrue(!status.CommitTime.Before(commitTime)))
 	qt.Assert(t, qt.Matches(status.Revision, `[0-9a-f]+`))
-	files, err := v.ListFiles(ctx, filepath.Join(dir, "subdir"))
+
+	// Test various permutations of ListFiles
+	var files []string
+	allFiles := []string{
+		"bar.txt",
+		"baz/something",
+		"subdir/bar/baz",
+		"subdir/foo",
+	}
+
+	// Empty dir implies repo root, i.e. all files
+	files, err = v.ListFiles(ctx, "")
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+
+	// Explicit repo root
+	files, err = v.ListFiles(ctx, dir)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+
+	// Relative path file under repo root
+	files, err = v.ListFiles(ctx, dir, "bar.txt")
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, []string{"bar.txt"}))
+
+	// Absolute path file under repo root
+	files, err = v.ListFiles(ctx, dir, filepath.Join(dir, "bar.txt"))
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, []string{"bar.txt"}))
+
+	// Relative path sub directory listed from root
+	files, err = v.ListFiles(ctx, dir, "subdir")
 	qt.Assert(t, qt.IsNil(err))
 	qt.Assert(t, qt.DeepEquals(files, []string{
-		"bar/baz",
-		"foo",
+		"subdir/bar/baz",
+		"subdir/foo",
 	}))
-	files, err = v.ListFiles(ctx, dir)
+
+	// Absolute path sub directory listed from root
+	files, err = v.ListFiles(ctx, dir, filepath.Join(dir, "subdir"))
 	qt.Assert(t, qt.IsNil(err))
 	qt.Assert(t, qt.DeepEquals(files, []string{
-		"bar.txt",
-		"baz/something",
 		"subdir/bar/baz",
 		"subdir/foo",
 	}))
 
-	// Change a file that's not in subdir. The status should remain the same.
+	// Listing of files in sub directory
+	files, err = v.ListFiles(ctx, subdir)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, []string{
+		"bar/baz",
+		"foo",
+	}))
+
+	// Change a file that's not in subdir. The status in subdir should remain
+	// the same.
 	err = os.WriteFile(filepath.Join(dir, "bar.txt"), []byte("something else"), 0o666)
 	qt.Assert(t, qt.IsNil(err))
-	status1, err := v.Status(ctx)
+	statuschanged, err := v.Status(ctx)
+	qt.Assert(t, qt.IsTrue(statuschanged.Uncommitted))
+	status1, err := v.Status(ctx, subdir)
 	qt.Assert(t, qt.IsNil(err))
 	qt.Assert(t, qt.DeepEquals(status1, status))
+
+	// Restore the file and ensure Status is clean
+	err = os.WriteFile(filepath.Join(dir, "bar.txt"), nil, 0o666)
+	qt.Assert(t, qt.IsNil(err))
+	files, err = v.ListFiles(ctx, dir)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+	status2, err := v.Status(ctx)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(status2, status))
+
+	// Add an untracked file
+	untracked := filepath.Join(dir, "untracked")
+	err = os.WriteFile(untracked, nil, 0666)
+	qt.Assert(t, qt.IsNil(err))
+	files, err = v.ListFiles(ctx, dir) // Does not include untracked file
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+	statusuntracked, err := v.Status(ctx) // Status does now show uncommitted changes
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.IsTrue(statusuntracked.Uncommitted))
+
+	// Remove the untracked file and ensure Status is clean
+	err = os.Remove(untracked)
+	qt.Assert(t, qt.IsNil(err))
+	files, err = v.ListFiles(ctx, dir)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+	status3, err := v.Status(ctx)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(status3, status))
+
+	// // Remove a tracked file so that it is now "missing"
+	err = os.Remove(filepath.Join(dir, "bar.txt"))
+	qt.Assert(t, qt.IsNil(err))
+	files, err = v.ListFiles(ctx, dir) // Still reports "missing" file
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.DeepEquals(files, allFiles))
+	statusmissing, err := v.Status(ctx) // Status does now show uncommitted changes
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.IsTrue(statusmissing.Uncommitted))
 }
 
 // initTestEnv sets up the environment so that