9397: Fix major bug in firstBlock(). Refactor ManifestTextForPath() to

Extract().  Test returning errors from Extract().

diff --git a/sdk/go/manifest/manifest.go b/sdk/go/manifest/manifest.go
index e8be7a2308c089803d227c7d50c19255b981dbd5..67bd02bec9db6877adae15453d7404cccf7ddee1 100644 (file)
--- a/sdk/go/manifest/manifest.go
+++ b/sdk/go/manifest/manifest.go
@@ -48,7 +48,7 @@ type FileStreamSegment struct {
 type ManifestStream struct {
        StreamName         string
        Blocks             []string
-       BlockOffsets       []uint64
+       blockOffsets       []uint64
        FileStreamSegments []FileStreamSegment
        Err                error
 }
@@ -162,6 +162,7 @@ func firstBlock(offsets []uint64, range_start uint64) int {
        // assumes that all of the blocks are contiguous, so range_start is guaranteed
        // to either fall into the range of a block or be outside the block range entirely
        for !(range_start >= block_start && range_start < block_end) {
+               fmt.Println(i, block_start, block_end)
                if lo == i {
                        // must be out of range, fail
                        return -1
@@ -170,10 +171,10 @@ func firstBlock(offsets []uint64, range_start uint64) int {
                        lo = i
                } else {
                        hi = i
-                       i = ((hi + lo) / 2)
-                       block_start = offsets[i]
-                       block_end = offsets[i+1]
                }
+               i = ((hi + lo) / 2)
+               block_start = offsets[i]
+               block_end = offsets[i+1]
        }
        return i
 }
@@ -195,14 +196,14 @@ func (s *ManifestStream) sendFileSegmentIterByName(filepath string, ch chan<- *F
                }
 
                // Binary search to determine first block in the stream
-               i := firstBlock(s.BlockOffsets, wantPos)
+               i := firstBlock(s.blockOffsets, wantPos)
                if i == -1 {
                        // Shouldn't happen, file segments are checked in parseManifestStream
                        panic(fmt.Sprintf("File segment %v extends past end of stream", fTok))
                }
                for ; i < len(s.Blocks); i++ {
-                       blockPos := s.BlockOffsets[i]
-                       blockEnd := s.BlockOffsets[i+1]
+                       blockPos := s.blockOffsets[i]
+                       blockEnd := s.blockOffsets[i+1]
                        if blockEnd <= wantPos {
                                // Shouldn't happen, FirstBlock() should start
                                // us on the right block, so if this triggers
@@ -255,7 +256,7 @@ func parseManifestStream(s string) (m ManifestStream) {
                return
        }
 
-       m.BlockOffsets = make([]uint64, len(m.Blocks)+1)
+       m.blockOffsets = make([]uint64, len(m.Blocks)+1)
        var streamoffset uint64
        for i, b := range m.Blocks {
                bl, err := ParseBlockLocator(b)
@@ -263,10 +264,10 @@ func parseManifestStream(s string) (m ManifestStream) {
                        m.Err = err
                        return
                }
-               m.BlockOffsets[i] = streamoffset
+               m.blockOffsets[i] = streamoffset
                streamoffset += uint64(bl.Size)
        }
-       m.BlockOffsets[len(m.Blocks)] = streamoffset
+       m.blockOffsets[len(m.Blocks)] = streamoffset
 
        if len(fileTokens) == 0 {
                m.Err = fmt.Errorf("No file tokens found")
@@ -311,13 +312,13 @@ func splitPath(srcpath string) (streamname, filename string) {
        return
 }
 
-func (m *Manifest) segment() *segmentedManifest {
+func (m *Manifest) segment() (*segmentedManifest, error) {
        files := make(segmentedManifest)
 
        for stream := range m.StreamIter() {
                if stream.Err != nil {
-                       // Skip streams with errors
-                       continue
+                       // Stream has an error
+                       return nil, stream.Err
                }
                currentStreamfiles := make(map[string]bool)
                for _, f := range stream.FileStreamSegments {
@@ -343,7 +344,7 @@ func (m *Manifest) segment() *segmentedManifest {
                }
        }
 
-       return &files
+       return &files, nil
 }
 
 func (stream segmentedStream) normalizedText(name string) string {
@@ -455,9 +456,12 @@ func (m segmentedManifest) manifestTextForPath(srcpath, relocate string) string
        return manifest
 }
 
-// ManifestTextForPath extracts some or all of the manifest and returns
-// normalized manifest text.  This is a swiss army knife function that can be
-// used a couple of different ways:
+// Extract extracts some or all of the manifest and returns the extracted
+// portion as a normalized manifest.  This is a swiss army knife function that
+// can be several ways:
+//
+// If 'srcpath' and 'relocate' are '.' it simply returns an equivalent manifest
+// in normalized form.
 //
 // If 'srcpath' points to a single file, it will return manifest text for just that file.
 // The value of "relocate" is can be used to rename the file or set the file stream.
@@ -473,13 +477,14 @@ func (m segmentedManifest) manifestTextForPath(srcpath, relocate string) string
 // ManifestTextForPath(".", ".")  (return entire normalized manfest text)
 // ManifestTextForPath("./stream", ".")  (extract "./stream" to "." and "./stream/subdir" to "./subdir")
 // ManifestTextForPath("./stream", "./bar")  (extract "./stream" to "./bar" and "./stream/subdir" to "./bar/subdir")
-func (m *Manifest) ManifestTextForPath(srcpath, relocate string) string {
-       return m.segment().manifestTextForPath(srcpath, relocate)
-}
-
-// NormalizedText returns the manifest text in normalized form.
-func (m *Manifest) NormalizedText() string {
-       return m.ManifestTextForPath(".", ".")
+func (m Manifest) Extract(srcpath, relocate string) (ret Manifest) {
+       segmented, err := m.segment()
+       if err != nil {
+               ret.Err = err
+               return
+       }
+       ret.Text = segmented.manifestTextForPath(srcpath, relocate)
+       return
 }
 
 func (m *Manifest) StreamIter() <-chan ManifestStream {

diff --git a/sdk/go/manifest/manifest_test.go b/sdk/go/manifest/manifest_test.go
index 084d06c8d83b4f0479fcf4aea8bb598dbd0dca7e..c72ed7a77d4c8908849a658ae19d346ecb4f61fe 100644 (file)
--- a/sdk/go/manifest/manifest_test.go
+++ b/sdk/go/manifest/manifest_test.go
@@ -1,14 +1,14 @@
 package manifest
 
 import (
+       "fmt"
+       "git.curoverse.com/arvados.git/sdk/go/arvadostest"
+       "git.curoverse.com/arvados.git/sdk/go/blockdigest"
        "io/ioutil"
        "reflect"
        "regexp"
        "runtime"
        "testing"
-
-       "git.curoverse.com/arvados.git/sdk/go/arvadostest"
-       "git.curoverse.com/arvados.git/sdk/go/blockdigest"
 )
 
 func getStackTrace() string {
@@ -257,19 +257,21 @@ func TestNormalizeManifest(t *testing.T) {
 . 085c37f02916da1cad16f93c54d899b7+41 0:41:md5sum.txt
 . 8b22da26f9f433dea0a10e5ec66d73ba+43 0:43:md5sum.txt
 `}
-       expectEqual(t, m1.NormalizedText(),
+       expectEqual(t, m1.Extract(".", ".").Text,
                `. 5348b82a029fd9e971a811ce1f71360b+43 085c37f02916da1cad16f93c54d899b7+41 8b22da26f9f433dea0a10e5ec66d73ba+43 0:127:md5sum.txt
 `)
 
        m2 := Manifest{Text: `. 204e43b8a1185621ca55a94839582e6f+67108864 b9677abbac956bd3e86b1deb28dfac03+67108864 fc15aff2a762b13f521baf042140acec+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:227212247:var-GS000016015-ASM.tsv.bz2
 `}
-       expectEqual(t, m2.NormalizedText(), m2.Text)
+       expectEqual(t, m2.Extract(".", ".").Text, m2.Text)
 
        m3 := Manifest{Text: `. 5348b82a029fd9e971a811ce1f71360b+43 3:40:md5sum.txt
 . 085c37f02916da1cad16f93c54d899b7+41 0:41:md5sum.txt
 . 8b22da26f9f433dea0a10e5ec66d73ba+43 0:43:md5sum.txt
 `}
-       expectEqual(t, m3.NormalizedText(), `. 5348b82a029fd9e971a811ce1f71360b+43 085c37f02916da1cad16f93c54d899b7+41 8b22da26f9f433dea0a10e5ec66d73ba+43 3:124:md5sum.txt
+       expectEqual(t, m3.Extract(".", ".").Text, `. 5348b82a029fd9e971a811ce1f71360b+43 085c37f02916da1cad16f93c54d899b7+41 8b22da26f9f433dea0a10e5ec66d73ba+43 3:124:md5sum.txt
+`)
+       expectEqual(t, m3.Extract("/md5sum.txt", "/wiggle.txt").Text, `. 5348b82a029fd9e971a811ce1f71360b+43 085c37f02916da1cad16f93c54d899b7+41 8b22da26f9f433dea0a10e5ec66d73ba+43 3:124:wiggle.txt
 `)
 
        m4 := Manifest{Text: `. 204e43b8a1185621ca55a94839582e6f+67108864 0:3:foo/bar
@@ -277,48 +279,48 @@ func TestNormalizeManifest(t *testing.T) {
 ./foo 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `}
 
-       expectEqual(t, m4.NormalizedText(),
+       expectEqual(t, m4.Extract(".", ".").Text,
                `./foo 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar
 ./zzz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 `)
 
-       expectEqual(t, m4.ManifestTextForPath("./foo", "."), ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
-       expectEqual(t, m4.ManifestTextForPath("./foo", "./baz"), "./baz 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
-       expectEqual(t, m4.ManifestTextForPath("./foo/bar", "."), ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
-       expectEqual(t, m4.ManifestTextForPath("./foo/bar", "./baz"), ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:baz 67108864:3:baz\n")
-       expectEqual(t, m4.ManifestTextForPath("./foo/bar", "./quux/"), "./quux 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
-       expectEqual(t, m4.ManifestTextForPath("./foo/bar", "./quux/baz"), "./quux 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:baz 67108864:3:baz\n")
-       expectEqual(t, m4.ManifestTextForPath(".", "."), `./foo 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar
+       expectEqual(t, m4.Extract("./foo", ".").Text, ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
+       expectEqual(t, m4.Extract("./foo", "./baz").Text, "./baz 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
+       expectEqual(t, m4.Extract("./foo/bar", ".").Text, ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
+       expectEqual(t, m4.Extract("./foo/bar", "./baz").Text, ". 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:baz 67108864:3:baz\n")
+       expectEqual(t, m4.Extract("./foo/bar", "./quux/").Text, "./quux 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar\n")
+       expectEqual(t, m4.Extract("./foo/bar", "./quux/baz").Text, "./quux 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:baz 67108864:3:baz\n")
+       expectEqual(t, m4.Extract(".", ".").Text, `./foo 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar
 ./zzz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 `)
-       expectEqual(t, m4.ManifestTextForPath(".", "./zip"), `./zip/foo 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar
+       expectEqual(t, m4.Extract(".", "./zip").Text, `./zip/foo 204e43b8a1185621ca55a94839582e6f+67108864 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar 67108864:3:bar
 ./zip/zzz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 `)
 
-       expectEqual(t, m4.ManifestTextForPath("foo/.//bar/../../zzz/", "/waz/"), `./waz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
+       expectEqual(t, m4.Extract("foo/.//bar/../../zzz/", "/waz/").Text, `./waz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 `)
 
        m5 := Manifest{Text: `. 204e43b8a1185621ca55a94839582e6f+67108864 0:3:foo/bar
 ./zzz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 ./foo 204e43b8a1185621ca55a94839582e6f+67108864 3:3:bar
 `}
-       expectEqual(t, m5.NormalizedText(),
+       expectEqual(t, m5.Extract(".", ".").Text,
                `./foo 204e43b8a1185621ca55a94839582e6f+67108864 0:6:bar
 ./zzz 204e43b8a1185621ca55a94839582e6f+67108864 0:999:zzz
 `)
 
        m8 := Manifest{Text: `./a\040b\040c 59ca0efa9f5633cb0371bbc0355478d8+13 0:13:hello\040world.txt
 `}
-       expectEqual(t, m8.NormalizedText(), m8.Text)
+       expectEqual(t, m8.Extract(".", ".").Text, m8.Text)
 
        m9 := Manifest{Text: ". acbd18db4cc2f85cedef654fccc4a4d8+40 0:10:one 20:10:two 10:10:one 30:10:two\n"}
-       expectEqual(t, m9.ManifestTextForPath("", ""), ". acbd18db4cc2f85cedef654fccc4a4d8+40 0:20:one 20:20:two\n")
+       expectEqual(t, m9.Extract("", "").Text, ". acbd18db4cc2f85cedef654fccc4a4d8+40 0:20:one 20:20:two\n")
 
        m10 := Manifest{Text: ". acbd18db4cc2f85cedef654fccc4a4d8+40 0:10:one 20:10:two 10:10:one 30:10:two\n"}
-       expectEqual(t, m10.ManifestTextForPath("./two", "./three"), ". acbd18db4cc2f85cedef654fccc4a4d8+40 20:20:three\n")
+       expectEqual(t, m10.Extract("./two", "./three").Text, ". acbd18db4cc2f85cedef654fccc4a4d8+40 20:20:three\n")
 
        m11 := Manifest{Text: arvadostest.PathologicalManifest}
-       expectEqual(t, m11.NormalizedText(), `. acbd18db4cc2f85cedef654fccc4a4d8+3 37b51d194a7513e45b56f6524f2d51f2+3 73feffa4b7f6bb68e44cf984c85f6e88+3+Z+K@xyzzy 0:1:f 1:4:ooba 5:1:r 5:4:rbaz 0:0:zero@0 0:0:zero@1 0:0:zero@4 0:0:zero@9
+       expectEqual(t, m11.Extract(".", ".").Text, `. acbd18db4cc2f85cedef654fccc4a4d8+3 37b51d194a7513e45b56f6524f2d51f2+3 73feffa4b7f6bb68e44cf984c85f6e88+3+Z+K@xyzzy 0:1:f 1:4:ooba 5:1:r 5:4:rbaz 0:0:zero@0 0:0:zero@1 0:0:zero@4 0:0:zero@9
 ./foo acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo 0:3:foo 0:0:zero
 ./foo\040bar acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:baz 0:3:baz\040waz
 ./overlapReverse acbd18db4cc2f85cedef654fccc4a4d8+3 2:1:o 2:1:ofoo 0:3:ofoo 1:2:oo
@@ -330,17 +332,40 @@ func TestNormalizeManifest(t *testing.T) {
 ./foo/baz 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `}
 
-       expectEqual(t, m12.ManifestTextForPath("./foo", "."), `. 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
+       expectEqual(t, m12.Extract("./foo", ".").Text, `. 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
 ./baz 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `)
-       expectEqual(t, m12.ManifestTextForPath("./foo", "./blub"), `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
+       expectEqual(t, m12.Extract("./foo", "./blub").Text, `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
 ./blub/baz 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `)
-       expectEqual(t, m12.ManifestTextForPath("./foo", "./blub/"), `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
+       expectEqual(t, m12.Extract("./foo", "./blub/").Text, `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
 ./blub/baz 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `)
-       expectEqual(t, m12.ManifestTextForPath("./foo/", "./blub/"), `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
+       expectEqual(t, m12.Extract("./foo/", "./blub/").Text, `./blub 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
 ./blub/baz 323d2a3ce20370c4ca1d3462a344f8fd+25885655 0:3:bar
 `)
 
+       m13 := Manifest{Text: `foo 204e43b8a1185621ca55a94839582e6f+67108864 0:3:bar
+`}
+
+       expectEqual(t, m13.Extract(".", ".").Text, ``)
+       expectEqual(t, m13.Extract(".", ".").Err.Error(), "Invalid stream name: foo")
+
+       m14 := Manifest{Text: `./foo 204e43b8a1185621ca55a94839582e6f+67108864 67108863:3:bar
+`}
+
+       expectEqual(t, m14.Extract(".", ".").Text, ``)
+       expectEqual(t, m14.Extract(".", ".").Err.Error(), "File segment 67108863:3:bar extends past end of stream 67108864")
+
+       m15 := Manifest{Text: `./foo 204e43b8a1185621ca55a94839582e6f+67108864 0:3bar
+`}
+
+       expectEqual(t, m15.Extract(".", ".").Text, ``)
+       expectEqual(t, m15.Extract(".", ".").Err.Error(), "Invalid file token: 0:3bar")
+}
+
+func TestFirstBlock(t *testing.T) {
+       fmt.Println("ZZZ")
+       expectEqual(t, firstBlock([]uint64{1, 2, 3, 4}, 3), 2)
+       expectEqual(t, firstBlock([]uint64{1, 2, 3, 4, 5, 6}, 4), 3)
 }