Merge pull request #1931 from dmcgowan/tar-diff-include-parent-dirs

archive: track and include parent directories

archive/tar.go

@@ -395,6 +395,7 @@ type changeWriter struct {
 	whiteoutT time.Time
 	inodeSrc  map[uint64]string
 	inodeRefs map[uint64][]string
+	addedDirs map[string]struct{}
 }
 
 func newChangeWriter(w io.Writer, source string) *changeWriter {
@@ -404,6 +405,7 @@ func newChangeWriter(w io.Writer, source string) *changeWriter {
 		whiteoutT: time.Now(),
 		inodeSrc:  map[uint64]string{},
 		inodeRefs: map[uint64][]string{},
+		addedDirs: map[string]struct{}{},
 	}
 }
 
@@ -416,6 +418,7 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e
 		whiteOutBase := filepath.Base(p)
 		whiteOut := filepath.Join(whiteOutDir, whiteoutPrefix+whiteOutBase)
 		hdr := &tar.Header{
+			Typeflag:   tar.TypeReg,
 			Name:       whiteOut[1:],
 			Size:       0,
 			ModTime:    cw.whiteoutT,
@@ -485,10 +488,8 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e
 				additionalLinks = cw.inodeRefs[inode]
 				delete(cw.inodeRefs, inode)
 			}
-		} else if k == fs.ChangeKindUnmodified && !f.IsDir() {
+		} else if k == fs.ChangeKindUnmodified {
 			// Nothing to write to diff
-			// Unmodified directories should still be written to keep
-			// directory permissions correct on direct unpack
 			return nil
 		}
 
@@ -501,6 +502,9 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e
 			hdr.PAXRecords[paxSchilyXattr+"security.capability"] = string(capability)
 		}
 
+		if err := cw.includeParents(hdr); err != nil {
+			return err
+		}
 		if err := cw.tw.WriteHeader(hdr); err != nil {
 			return errors.Wrap(err, "failed to write file header")
 		}
@@ -530,6 +534,10 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e
 				hdr.Typeflag = tar.TypeLink
 				hdr.Linkname = source
 				hdr.Size = 0
+
+				if err := cw.includeParents(hdr); err != nil {
+					return err
+				}
 				if err := cw.tw.WriteHeader(hdr); err != nil {
 					return errors.Wrap(err, "failed to write file header")
 				}
@@ -546,6 +554,29 @@ func (cw *changeWriter) Close() error {
 	return nil
 }
 
+func (cw *changeWriter) includeParents(hdr *tar.Header) error {
+	name := strings.TrimRight(hdr.Name, "/")
+	fname := filepath.Join(cw.source, name)
+	parent := filepath.Dir(name)
+	pname := filepath.Join(cw.source, parent)
+
+	// Do not include root directory as parent
+	if fname != cw.source && pname != cw.source {
+		_, ok := cw.addedDirs[parent]
+		if !ok {
+			cw.addedDirs[parent] = struct{}{}
+			fi, err := os.Stat(pname)
+			if err != nil {
+				return err
+			}
+			if err := cw.HandleChange(fs.ChangeKindModify, parent, fi, nil); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func copyBuffered(ctx context.Context, dst io.Writer, src io.Reader) (written int64, err error) {
 	buf := bufferPool.Get().(*[]byte)
 	defer bufferPool.Put(buf)

archive/tar_test.go

@@ -698,6 +698,44 @@ func TestDiffTar(t *testing.T) {
 				fstest.CreateFile("/d2/f", []byte("ok"), 0644),
 			),
 		},
+		{
+			name: "HardlinkParentInclusion",
+			validators: []tarEntryValidator{
+				dirEntry("d2/", 0755),
+				fileEntry("d2/l1", []byte("link me"), 0644),
+				// d1/f1 and its parent is included after the new link,
+				// before the new link was included, these files would
+				// not habe needed
+				dirEntry("d1/", 0755),
+				linkEntry("d1/f1", "d2/l1"),
+				dirEntry("d3/", 0755),
+				fileEntry("d3/l1", []byte("link me"), 0644),
+				dirEntry("d4/", 0755),
+				linkEntry("d4/f1", "d3/l1"),
+				whiteoutEntry("d6/l1"),
+				whiteoutEntry("d6/l2"),
+			},
+			a: fstest.Apply(
+				fstest.CreateDir("/d1/", 0755),
+				fstest.CreateFile("/d1/f1", []byte("link me"), 0644),
+				fstest.CreateDir("/d2/", 0755),
+				fstest.CreateFile("/d2/f1", []byte("link me"), 0644),
+				fstest.CreateDir("/d3/", 0755),
+				fstest.CreateDir("/d4/", 0755),
+				fstest.CreateFile("/d4/f1", []byte("link me"), 0644),
+				fstest.CreateDir("/d5/", 0755),
+				fstest.CreateFile("/d5/f1", []byte("link me"), 0644),
+				fstest.CreateDir("/d6/", 0755),
+				fstest.Link("/d1/f1", "/d6/l1"),
+				fstest.Link("/d5/f1", "/d6/l2"),
+			),
+			b: fstest.Apply(
+				fstest.Link("/d1/f1", "/d2/l1"),
+				fstest.Link("/d4/f1", "/d3/l1"),
+				fstest.Remove("/d6/l1"),
+				fstest.Remove("/d6/l2"),
+			),
+		},
 	}
 
 	for _, at := range tests {
@@ -740,6 +778,37 @@ func fileEntry(name string, expected []byte, mode int) tarEntryValidator {
 	}
 }
 
+func linkEntry(name, link string) tarEntryValidator {
+	return func(hdr *tar.Header, b []byte) error {
+		if hdr.Typeflag != tar.TypeLink {
+			return errors.New("not link type")
+		}
+		if hdr.Name != name {
+			return errors.Errorf("wrong name %q, expected %q", hdr.Name, name)
+		}
+		if hdr.Linkname != link {
+			return errors.Errorf("wrong link %q, expected %q", hdr.Linkname, link)
+		}
+		return nil
+	}
+}
+
+func whiteoutEntry(name string) tarEntryValidator {
+	whiteOutDir := filepath.Dir(name)
+	whiteOutBase := filepath.Base(name)
+	whiteOut := filepath.Join(whiteOutDir, whiteoutPrefix+whiteOutBase)
+
+	return func(hdr *tar.Header, b []byte) error {
+		if hdr.Typeflag != tar.TypeReg {
+			return errors.Errorf("not file type: %q", hdr.Typeflag)
+		}
+		if hdr.Name != whiteOut {
+			return errors.Errorf("wrong name %q, expected whiteout %q", hdr.Name, name)
+		}
+		return nil
+	}
+}
+
 func makeDiffTarTest(validators []tarEntryValidator, a, b fstest.Applier) func(*testing.T) {
 	return func(t *testing.T) {
 		ad, err := ioutil.TempDir("", "test-make-diff-tar-")

fs/diff.go

@@ -224,8 +224,6 @@ func doubleWalkDiff(ctx context.Context, changeFn ChangeFunc, a, b string) (err
 
 		f1, f2 *currentPath
 		rmdir  string
-		lastEmittedDir = string(filepath.Separator)
-		parents        []os.FileInfo
 	)
 	g.Go(func() error {
 		defer close(c1)
@@ -260,10 +258,7 @@ func doubleWalkDiff(ctx context.Context, changeFn ChangeFunc, a, b string) (err
 				continue
 			}
 
-			var (
+			var f os.FileInfo
-				f    os.FileInfo
-				emit = true
-			)
 			k, p := pathChange(f1, f2)
 			switch k {
 			case ChangeKindAdd:
@@ -299,83 +294,17 @@ func doubleWalkDiff(ctx context.Context, changeFn ChangeFunc, a, b string) (err
 				f2 = nil
 				if same {
 					if !isLinked(f) {
-						emit = false
+						continue
 					}
 					k = ChangeKindUnmodified
 				}
 			}
-			if emit {
-				emittedDir, emitParents := commonParents(lastEmittedDir, p, parents)
-				for _, pf := range emitParents {
-					p := filepath.Join(emittedDir, pf.Name())
-					if err := changeFn(ChangeKindUnmodified, p, pf, nil); err != nil {
-						return err
-					}
-					emittedDir = p
-				}
-
 			if err := changeFn(k, p, f, nil); err != nil {
 				return err
 			}
-
-				if f != nil && f.IsDir() {
-					lastEmittedDir = p
-				} else {
-					lastEmittedDir = emittedDir
-				}
-
-				parents = parents[:0]
-			} else if f.IsDir() {
-				lastEmittedDir, parents = commonParents(lastEmittedDir, p, parents)
-				parents = append(parents, f)
-			}
 		}
 		return nil
 	})
 
 	return g.Wait()
 }
-
-func commonParents(base, updated string, dirs []os.FileInfo) (string, []os.FileInfo) {
-	if basePrefix := makePrefix(base); strings.HasPrefix(updated, basePrefix) {
-		var (
-			parents []os.FileInfo
-			last    = base
-		)
-		for _, d := range dirs {
-			next := filepath.Join(last, d.Name())
-			if strings.HasPrefix(updated, makePrefix(last)) {
-				parents = append(parents, d)
-				last = next
-			} else {
-				break
-			}
-		}
-		return base, parents
-	}
-
-	baseS := strings.Split(base, string(filepath.Separator))
-	updatedS := strings.Split(updated, string(filepath.Separator))
-	commonS := []string{string(filepath.Separator)}
-
-	min := len(baseS)
-	if len(updatedS) < min {
-		min = len(updatedS)
-	}
-	for i := 0; i < min; i++ {
-		if baseS[i] == updatedS[i] {
-			commonS = append(commonS, baseS[i])
-		} else {
-			break
-		}
-	}
-
-	return filepath.Join(commonS...), []os.FileInfo{}
-}
-
-func makePrefix(d string) string {
-	if d == "" || d[len(d)-1] != filepath.Separator {
-		return d + string(filepath.Separator)
-	}
-	return d
-}

fs/diff_test.go

@@ -44,7 +44,6 @@ func TestSimpleDiff(t *testing.T) {
 		fstest.Remove("/etc/unexpected"),
 	)
 	diff := []TestChange{
-		Unchanged("/etc"),
 		Modify("/etc/hosts"),
 		Modify("/etc/profile"),
 		Delete("/etc/unexpected"),
@@ -71,7 +70,6 @@ func TestDirectoryReplace(t *testing.T) {
 		fstest.CreateFile("/dir1/f2", []byte("Now file"), 0666),
 	)
 	diff := []TestChange{
-		Unchanged("/dir1"),
 		Add("/dir1/f11"),
 		Modify("/dir1/f2"),
 	}
@@ -134,13 +132,10 @@ func TestParentDirectoryPermission(t *testing.T) {
 		fstest.CreateFile("/dir3/f", []byte("irrelevant"), 0644),
 	)
 	diff := []TestChange{
-		Unchanged("/dir1"),
 		Add("/dir1/d"),
 		Add("/dir1/d/f"),
 		Add("/dir1/f"),
-		Unchanged("/dir2"),
 		Add("/dir2/f"),
-		Unchanged("/dir3"),
 		Add("/dir3/f"),
 	}
 
@@ -198,56 +193,6 @@ func TestUpdateWithSameTime(t *testing.T) {
 	}
 }
 
-func TestUnchangedParent(t *testing.T) {
-	skipDiffTestOnWindows(t)
-	l1 := fstest.Apply(
-		fstest.CreateDir("/dir1", 0755),
-		fstest.CreateDir("/dir3", 0755),
-		fstest.CreateDir("/dir3/a", 0755),
-		fstest.CreateDir("/dir3/a/e", 0755),
-		fstest.CreateDir("/dir3/z", 0755),
-	)
-	l2 := fstest.Apply(
-		fstest.CreateDir("/dir1/a", 0755),
-		fstest.CreateFile("/dir1/a/b", []byte("irrelevant"), 0644),
-		fstest.CreateDir("/dir1/a/e", 0755),
-		fstest.CreateFile("/dir1/a/e/f", []byte("irrelevant"), 0644),
-		fstest.CreateFile("/dir1/f", []byte("irrelevant"), 0644),
-		fstest.CreateDir("/dir2", 0755),
-		fstest.CreateFile("/dir2/f", []byte("irrelevant"), 0644),
-		fstest.CreateFile("/dir3/a/b", []byte("irrelevant"), 0644),
-		fstest.CreateDir("/dir3/a/c", 0755),
-		fstest.CreateDir("/dir3/a/e/i", 0755),
-		fstest.CreateFile("/dir3/a/e/i/f", []byte("irrelevant"), 0644),
-		fstest.CreateFile("/dir3/f", []byte("irrelevant"), 0644),
-		fstest.CreateFile("/dir3/z/f", []byte("irrelevant"), 0644),
-	)
-	diff := []TestChange{
-		Unchanged("/dir1"),
-		Add("/dir1/a"),
-		Add("/dir1/a/b"),
-		Add("/dir1/a/e"),
-		Add("/dir1/a/e/f"),
-		Add("/dir1/f"),
-		Add("/dir2"),
-		Add("/dir2/f"),
-		Unchanged("/dir3"),
-		Unchanged("/dir3/a"),
-		Add("/dir3/a/b"),
-		Add("/dir3/a/c"),
-		Unchanged("/dir3/a/e"),
-		Add("/dir3/a/e/i"),
-		Add("/dir3/a/e/i/f"),
-		Add("/dir3/f"),
-		Unchanged("/dir3/z"),
-		Add("/dir3/z/f"),
-	}
-
-	if err := testDiffWithBase(l1, l2, diff); err != nil {
-		t.Fatalf("Failed diff with base: %+v", err)
-	}
-}
-
 // buildkit#172
 func TestLchtimes(t *testing.T) {
 	skipDiffTestOnWindows(t)
@@ -411,7 +356,7 @@ func diffString(c1, c2 []TestChange) string {
 func changesString(c []TestChange) string {
 	strs := make([]string, len(c))
 	for i := range c {
-		strs[i] = fmt.Sprintf("\t%-10s\t%s", c[i].Kind, c[i].Path)
+		strs[i] = fmt.Sprintf("\t%s\t%s", c[i].Kind, c[i].Path)
 	}
 	return strings.Join(strs, "\n")
 }
@@ -436,10 +381,3 @@ func Modify(p string) TestChange {
 		Path: filepath.FromSlash(p),
 	}
 }
-
-func Unchanged(p string) TestChange {
-	return TestChange{
-		Kind: ChangeKindUnmodified,
-		Path: filepath.FromSlash(p),
-	}
-}