[submodule_update] Don't delete source-of-truth submodules
The `name` field in .gitmodules entries is generated by submodule_update
and is used to map the Jiri project name to the submodule. Therefore,
submodules that don't have corresponding Jiri projects need not have the
`name` field set. So we can use the presence/absence of the `name` field
to determine whether a submodule is the source of truth or whether it's
still being mirrored from Jiri, and don't attempt to delete such
submodules.
Bug: 326595281
Change-Id: Ib8a4a1a95d316380a2d5c2d36a9b40f6d1036331
Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/infra/+/997592
Reviewed-by: Danielle Kay <danikay@google.com>
Commit-Queue: Oliver Newman <olivernewman@google.com>
diff --git a/cmd/submodule_update/gitutil/git.go b/cmd/submodule_update/gitutil/git.go
index 09916c8..37eb92a 100644
--- a/cmd/submodule_update/gitutil/git.go
+++ b/cmd/submodule_update/gitutil/git.go
@@ -421,13 +421,18 @@
}
// ConfigGetKeyFromFile gets current git configuration value for the given key from file.
+//
+// Returns an empty string if the configuration value is not found.
func (g *Git) ConfigGetKeyFromFile(key string, file string) (string, error) {
- out, err := g.runOutput("config", "--file", file, "--get", key)
+ out, err := g.runOutput("config", "--file", file, "--default", "", "--get", key)
if err != nil {
return "", err
}
- if got, want := len(out), 1; got != want {
- glog.Warning("wanted one line log, got %d line log: %q", got, out)
+ if len(out) == 0 {
+ return "", nil
+ }
+ if len(out) > 1 {
+ glog.Warning("wanted one line log, got %d line log: %q", len(out), out)
}
return out[0], nil
}
diff --git a/cmd/submodule_update/submodule/submodule.go b/cmd/submodule_update/submodule/submodule.go
index 0887a72..8d55510 100644
--- a/cmd/submodule_update/submodule/submodule.go
+++ b/cmd/submodule_update/submodule/submodule.go
@@ -42,19 +42,17 @@
return Key(s.Path)
}
-var /* const */ submoduleStatusRe = regexp.MustCompile(`(?m)^[+\-U\s]?([0-9a-f]{40})\s([a-zA-Z0-9_.\-\/]+).*$`)
+var submoduleStatusRe = regexp.MustCompile(`(?m)^[+\-U\s]?([0-9a-f]{40})\s([a-zA-Z0-9_.\-\/]+).*$`)
func gitSubmodules(g *gitutil.Git, cached bool) (Submodules, error) {
gitSubmoduleStatus, err := g.SubmoduleStatus(gitutil.CachedOpt(cached))
if err != nil {
return nil, err
}
- gitSubmodules := gitSubmoduleStatusToSubmodule(gitSubmoduleStatus)
- addRemoteToSubmodules(g, gitSubmodules)
- return gitSubmodules, nil
+ return gitSubmoduleStatusToSubmodule(g, gitSubmoduleStatus)
}
-func gitSubmoduleStatusToSubmodule(status string) Submodules {
+func gitSubmoduleStatusToSubmodule(g *gitutil.Git, status string) (Submodules, error) {
var subModules = Submodules{}
subStatus := submoduleStatusRe.FindAllStringSubmatch(status, -1)
for _, status := range subStatus {
@@ -66,22 +64,23 @@
Path: status[2],
Revision: status[1],
}
- subModules[subM.Key()] = subM
- }
- return subModules
-}
-
-func addRemoteToSubmodules(g *gitutil.Git, s Submodules) error {
- for _, subM := range s {
- configKey := fmt.Sprintf("submodule.%s.url", subM.Path)
- url, err := g.ConfigGetKeyFromFile(configKey, ".gitmodules")
+ url, err := g.ConfigGetKeyFromFile(
+ fmt.Sprintf("submodule.%s.url", subM.Path),
+ ".gitmodules")
if err != nil {
- return err
+ return nil, err
}
subM.Remote = url
- s[Key(subM.Path)] = subM
+ name, err := g.ConfigGetKeyFromFile(
+ fmt.Sprintf("submodule.%s.name", subM.Path),
+ ".gitmodules")
+ if err != nil {
+ return nil, err
+ }
+ subM.Name = name
+ subModules[subM.Key()] = subM
}
- return nil
+ return subModules, nil
}
func addIgnoreToSubmodulesConfig(g *gitutil.Git, s Submodule) error {
@@ -296,11 +295,18 @@
return nil
}
-func getDiff(submodules1, submodules2 Submodules) (Diff, error) {
+func getDiff(gitSubmodules, jiriSubmodules Submodules) (Diff, error) {
diff := Diff{}
// Get deleted submodules
- for key, s1 := range submodules1 {
- if _, ok := submodules2[key]; !ok {
+ for key, s1 := range gitSubmodules {
+ if s1.Name == "" {
+ // Submodules that are source-of-truth in the superproject will not
+ // have the `name` field set. It is only set for submodules that
+ // correspond to Jiri projects. Submodules that intentionally do not
+ // correspond to any Jiri project should not be deleted.
+ continue
+ }
+ if _, ok := jiriSubmodules[key]; !ok {
diff.DeletedSubmodules = append(diff.DeletedSubmodules, DiffSubmodule{
Path: s1.Path,
Revision: s1.Revision,
@@ -310,8 +316,8 @@
}
// Get new and updated submodules
- for key, s2 := range submodules2 {
- if s1, ok := submodules1[key]; !ok {
+ for key, s2 := range jiriSubmodules {
+ if s1, ok := gitSubmodules[key]; !ok {
diff.NewSubmodules = append(diff.NewSubmodules, DiffSubmodule{
Name: s2.Name,
Path: s2.Path,
diff --git a/cmd/submodule_update/submodule/submodule_test.go b/cmd/submodule_update/submodule/submodule_test.go
index 6acc022..ed949f8 100644
--- a/cmd/submodule_update/submodule/submodule_test.go
+++ b/cmd/submodule_update/submodule/submodule_test.go
@@ -69,6 +69,7 @@
var subModules = Submodules{}
for subPath, rev := range submodules {
subM := Submodule{
+ Name: subPath,
Path: subPath,
Revision: rev,
// For testing, fake the remote
@@ -96,60 +97,93 @@
remoteSub1.Remote = path.Join("new", "remote", "sub1")
remoteChangeSub[Key("sub1")] = remoteSub1
- emptyDiff := Diff{}
type diffTest struct {
- sub1 Submodules
- sub2 Submodules
- want Diff
+ name string
+ gitSubmodules Submodules
+ jiriSubmodules Submodules
+ want Diff
}
testCases := []diffTest{
- // No diff
{
- initialSub,
- initialSub,
- emptyDiff,
+ name: "no diff",
+ gitSubmodules: initialSub,
+ jiriSubmodules: initialSub,
+ want: Diff{},
},
- // Add sub3
{
- initialSub,
- createSubmodules(t, map[string]string{"sub1": "12345a", "sub2": "12345b", "sub3": "12345c"}),
- Diff{
+ name: "add sub3",
+ gitSubmodules: initialSub,
+ jiriSubmodules: createSubmodules(t, map[string]string{
+ "sub1": "12345a",
+ "sub2": "12345b",
+ "sub3": "12345c",
+ }),
+ want: Diff{
NewSubmodules: []DiffSubmodule{{
+ Name: "sub3",
Path: "sub3",
Revision: "12345c",
Remote: path.Join("remote", "sub3"),
}},
},
},
- // Delete sub1
{
- initialSub,
- createSubmodules(t, map[string]string{"sub2": "12345b"}),
- Diff{
+ name: "delete sub1",
+ gitSubmodules: initialSub,
+ jiriSubmodules: createSubmodules(t, map[string]string{
+ "sub2": "12345b",
+ }),
+ want: Diff{
DeletedSubmodules: []DiffSubmodule{{
Path: "sub1",
Revision: "12345a",
Remote: path.Join("remote", "sub1")}},
},
},
- // Update sub1 (new revision)
{
- initialSub,
- createSubmodules(t, map[string]string{"sub1": "12346a", "sub2": "12345b"}),
- Diff{
+ name: "don't delete nameless submodule",
+ gitSubmodules: func() Submodules {
+ submodules := createSubmodules(t, map[string]string{
+ "sub1": "12345a",
+ "nameless": "12345b",
+ })
+ subm := submodules[Key("nameless")]
+ subm.Name = ""
+ submodules[Key("nameless")] = subm
+ return submodules
+ }(),
+ jiriSubmodules: createSubmodules(t, map[string]string{
+ "sub1": "12345a",
+ }),
+ want: Diff{},
+ },
+ {
+ name: "update sub1 (new revision)",
+ gitSubmodules: initialSub,
+ jiriSubmodules: createSubmodules(t, map[string]string{
+ "sub1": "12346a",
+ "sub2": "12345b",
+ }),
+ want: Diff{
UpdatedSubmodules: []DiffSubmodule{{
+ Name: "sub1",
Path: "sub1",
Revision: "12346a",
OldRevision: "12345a",
}},
},
},
- // Move sub1 (change path) - Processed as a delete (old path) and a new submodule (new path)
{
- initialSub,
- createSubmodules(t, map[string]string{"sub3": "12345a", "sub2": "12345b"}),
- Diff{
+ // Processed as a delete (old path) and a new submodule (new path).
+ name: "move sub1 (change path)",
+ gitSubmodules: initialSub,
+ jiriSubmodules: createSubmodules(t, map[string]string{
+ "sub3": "12345a",
+ "sub2": "12345b",
+ }),
+ want: Diff{
NewSubmodules: []DiffSubmodule{{
+ Name: "sub3",
Path: "sub3",
Revision: "12345a",
Remote: path.Join("remote", "sub3")}},
@@ -159,12 +193,14 @@
Remote: path.Join("remote", "sub1")}},
},
},
- // Move sub1 (change remote) - Processed as a delete (old remote) and a new submodule (new remote)
{
- initialSub,
- remoteChangeSub,
- Diff{
+ // Processed as a delete (old remote) and a new submodule (new remote)
+ name: "move sub1 (change remote)",
+ gitSubmodules: initialSub,
+ jiriSubmodules: remoteChangeSub,
+ want: Diff{
NewSubmodules: []DiffSubmodule{{
+ Name: "sub1",
Path: "sub1",
Revision: "12345a",
Remote: path.Join("new", "remote", "sub1")}},
@@ -177,14 +213,16 @@
}
for _, tc := range testCases {
- got, err := getDiff(tc.sub1, tc.sub2)
- if err != nil {
- t.Fatalf("GetDiff failed with: %s", err)
- }
+ t.Run(tc.name, func(t *testing.T) {
+ got, err := getDiff(tc.gitSubmodules, tc.jiriSubmodules)
+ if err != nil {
+ t.Fatalf("GetDiff failed with: %s", err)
+ }
- if diff := cmp.Diff(tc.want, got); diff != "" {
- t.Errorf("GetDiff failed (-want +got):\n%s", diff)
- }
+ if diff := cmp.Diff(tc.want, got); diff != "" {
+ t.Errorf("GetDiff failed (-want +got):\n%s", diff)
+ }
+ })
}
}
