[scm] Use correct action when returning all files

Previously, every file returned by `ctx.scm.all_files()` or (when the
`--all` flag was set) `ctx.scm.affected_files()` would have a "A" in its
action field, falsely implying that all files were newly added, and thus
making it impossible for checks to accurately determine whether a file
was new or not.

Now files will have the correct action value even when all files are
returned.

To test, I ran a locally compiled version of shac with this change
through the static-checks builder in led and confirmed that a check that
only checks files with action="A" no longer reports any findings:
https://ci.chromium.org/ui/p/fuchsia/builders/try.shadow/static-checks/b8740770129818037905/test-results?q=ExactID%3Ashac%2Funderscore_vs_dash+VHash%3Ae3b0c44298fc1c14

Change-Id: I4dfd50386d6de9659eb9f8d91647fb22ce7a6c84
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/1092836
Commit-Queue: Auto-Submit <[email protected]>
Reviewed-by: Jerry Belton <[email protected]>
Fuchsia-Auto-Submit: Oliver Newman <[email protected]>

doc/stdlib.md

@@ -540,6 +540,10 @@ shac.register_check(new_todos)
 A map of {path: struct()} where the struct has a string field action and a
 function new_lines().
 
+The action field's value is a single letter corresponding to the
+`--diff-filter` representation of the action per
+https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
+
 ## ctx.scm.all_files
 
 Returns all files found in the current workspace.
@@ -568,6 +572,13 @@ shac.register_check(all_todos)
 A map of {path: struct()} where the struct has a string field action and a
 function new_lines().
 
+The action field's value is a single letter corresponding to the
+`--diff-filter` representation of the action per
+https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
+
+If a file was not modified relative to the upstream commit, the action
+field will be an empty string.
+
 ## ctx.vars
 
 ctx.vars provides access to runtime-configurable variables.

doc/stdlib.star

@@ -468,6 +468,10 @@ def _ctx_scm_affected_files(glob = None, include_deleted = False):
     Returns:
       A map of {path: struct()} where the struct has a string field action and a
       function new_lines().
+
+      The action field's value is a single letter corresponding to the
+      `--diff-filter` representation of the action per
+      https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
     """
     pass
 
@@ -495,6 +499,13 @@ def _ctx_scm_all_files(glob = None, include_deleted = False):
     Returns:
       A map of {path: struct()} where the struct has a string field action and a
       function new_lines().
+
+      The action field's value is a single letter corresponding to the
+      `--diff-filter` representation of the action per
+      https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203
+
+      If a file was not modified relative to the upstream commit, the action
+      field will be an empty string.
     """
     pass
 

internal/engine/run_test.go

@@ -903,18 +903,18 @@ func TestRun_SCM_Git_NoUpstream_Pristine(t *testing.T) {
 			"ctx-scm-affected_files.star",
 			true,
 			"[//ctx-scm-affected_files.star:19] \n" +
-				"a.txt: A\n" +
+				"a.txt: \n" +
 				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"z.txt: \n" +
 				"\n",
 		},
 		{
 			"ctx-scm-all_files.star",
 			false,
 			"[//ctx-scm-all_files.star:19] \n" +
-				"a.txt: A\n" +
+				"a.txt: \n" +
 				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"z.txt: \n" +
 				"\n",
 		},
 	}
@@ -966,9 +966,9 @@ func TestRun_SCM_Git_NoUpstream_Staged(t *testing.T) {
 			"ctx-scm-all_files.star",
 			false,
 			"[//ctx-scm-all_files.star:19] \n" +
-				"a.txt: A\n" +
+				"a.txt: \n" +
 				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"z.txt: \n" +
 				"\n",
 		},
 	}
@@ -1010,7 +1010,7 @@ func TestRun_SCM_Git_Upstream_Staged(t *testing.T) {
 		{
 			"ctx-scm-all_files.star",
 			"[//ctx-scm-all_files.star:19] \n" +
-				"a.txt: A\n" +
+				"a.txt: R\n" +
 				scmStarlarkFiles("A") +
 				"z.txt: A\n" +
 				"\n",
@@ -1044,12 +1044,12 @@ func TestRun_SCM_Git_Untracked(t *testing.T) {
 		{
 			"ctx-scm-all_files.star",
 			"[//shac.star:19] \n" +
-				".gitignore: A\n" +
-				"a.txt: A\n" +
-				"shac.star: A\n" +
+				".gitignore: \n" +
+				"a.txt: \n" +
+				"shac.star: \n" +
 				"staged.txt: A\n" +
-				"untracked.txt: A\n" +
-				"z.txt: A\n" +
+				"untracked.txt: \n" +
+				"z.txt: \n" +
 				"\n",
 		},
 	}
@@ -1128,9 +1128,9 @@ func TestRun_SCM_Git_Submodule(t *testing.T) {
 			"ctx-scm-all_files.star",
 			"[//ctx-scm-all_files.star:19] \n" +
 				".gitmodules: A\n" +
-				"a.txt: A\n" +
+				"a.txt: \n" +
 				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"z.txt: \n" +
 				"\n",
 		},
 	}
@@ -1178,24 +1178,24 @@ func TestRun_SCM_DeletedFile(t *testing.T) {
 		{
 			"ctx-scm-all_files.star",
 			"[//ctx-scm-all_files.star:19] \n" +
-				"a.txt: A\n" +
-				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"a.txt: \n" +
+				scmStarlarkFiles("") +
+				"z.txt: \n" +
 				"\n",
 		},
 		{
 			"ctx-scm-all_files-include_deleted.star",
 			"[//ctx-scm-all_files-include_deleted.star:26] \n" +
 				"With deleted:\n" +
-				"a.txt: A\n" +
-				scmStarlarkFiles("A") +
+				"a.txt: \n" +
+				scmStarlarkFiles("") +
 				"file-to-delete.txt: D\n" +
-				"z.txt: A\n" +
+				"z.txt: \n" +
 				"\n" +
 				"Without deleted:\n" +
-				"a.txt: A\n" +
-				scmStarlarkFiles("A") +
-				"z.txt: A\n" +
+				"a.txt: \n" +
+				scmStarlarkFiles("") +
+				"z.txt: \n" +
 				"\n"},
 	}
 	for i := range data {

internal/engine/runtime_ctx_scm.go

@@ -429,17 +429,26 @@ func (g *gitCheckout) affectedFiles(ctx context.Context, includeDeleted bool) ([
 	}
 	g.mu.Lock()
 	defer g.mu.Unlock()
+	modified := g.affectedFilesImpl(ctx, includeDeleted)
+	if g.err != nil {
+		return nil, g.err
+	}
 	// Untracked files are always considered affected (as long as they're not
 	// ignored).
 	o := g.run(ctx, "ls-files", "-z", "--others", "--exclude-standard")
 	var items []string
 	if len(o) > 0 {
 		items = strings.Split(o[:len(o)-1], "\x00")
 	}
-	modified := make([]file, 0, len(items))
 	for _, path := range items {
 		modified = append(modified, &fileImpl{a: "A", path: filepath.ToSlash(path)})
 	}
+	sort.Slice(modified, func(i, j int) bool { return modified[i].rootedpath() < modified[j].rootedpath() })
+	return modified, g.err
+}
+
+func (g *gitCheckout) affectedFilesImpl(ctx context.Context, includeDeleted bool) []file {
+	var modified []file
 	// Each line has a variable number of NUL character, so process one at a time.
 	for o := g.run(ctx, "diff", "--name-status", "-z", "-C", g.upstream.hash); len(o) != 0; {
 		var action, path string
@@ -486,12 +495,10 @@ func (g *gitCheckout) affectedFiles(ctx context.Context, includeDeleted bool) ([
 		// TODO(olivernewman): Omit deleted submodules. For now they're
 		// treated the same as deleted regular files.
 		if action == "D" || !g.isSubmodule(path) {
-			// TODO(maruel): Share with allFiles.
 			modified = append(modified, &fileImpl{a: action, path: filepath.ToSlash(path)})
 		}
 	}
-	sort.Slice(modified, func(i, j int) bool { return modified[i].rootedpath() < modified[j].rootedpath() })
-	return modified, g.err
+	return modified
 }
 
 // allFiles returns all the files in this checkout.
@@ -508,6 +515,14 @@ func (g *gitCheckout) allFiles(ctx context.Context, includeDeleted bool) ([]file
 		// ls-files output may not be parseable and we should exit early.
 		return nil, g.err
 	}
+	affected := g.affectedFilesImpl(ctx, includeDeleted)
+	if g.err != nil {
+		return nil, g.err
+	}
+	affectedFileActions := make(map[string]string, len(affected))
+	for _, f := range affected {
+		affectedFileActions[f.rootedpath()] = f.action()
+	}
 	items := strings.Split(o[:len(o)-1], "\x00")
 	all := make([]file, 0, len(items))
 	for _, path := range items {
@@ -521,9 +536,11 @@ func (g *gitCheckout) allFiles(ctx context.Context, includeDeleted bool) ([]file
 			return nil, err
 		}
 		if !fi.IsDir() { // Not a submodule.
-			// TODO(maruel): Still include action from affectedFiles()?
-			// TODO(maruel): Share with affectedFiles.
-			all = append(all, &fileImpl{a: "A", path: filepath.ToSlash(path)})
+			p := filepath.ToSlash(path)
+			// action will be an empty string if the file has not changed
+			// relative to the upstream commit.
+			action := affectedFileActions[p]
+			all = append(all, &fileImpl{a: action, path: p})
 		}
 	}
 	sort.Slice(all, func(i, j int) bool { return all[i].rootedpath() < all[j].rootedpath() })

internal/engine/version.go

@@ -26,7 +26,7 @@ var (
 	// Version is the current tool version.
 	//
 	// TODO(maruel): Add proper version, preferably from git tag.
-	Version = shacVersion{0, 1, 17}
+	Version = shacVersion{0, 1, 18}
 )
 
 func (v shacVersion) String() string {