docs(snapshot s3): trim code comments to what the code does not say

Keep each comment to the one fact a reader could not get from the code,
stated once, where it applies.

Several facts were repeated across files: that S3 returns a stored
checksum only when asked appeared four times, that unusable checksums are
collected rather than fatal three, and that the shared pipeline keeps the
sources from disagreeing three. Each now lives in one place -- the
interface, the error type and the digest source respectively -- and the
copies are gone. Test comments that restated their own assertions or case
names are dropped, and the rest are cut to their load-bearing sentence.

Godoc on exported identifiers stays. No code changes; gofmt re-aligned
two structs whose field comments were removed.

Author: claude

cmd/kosli/snapshotS3.go

@@ -84,7 +84,6 @@ kosli snapshot s3 yourEnvironmentName \
 	--org yourOrgName
 `
 
-// fingerprint sources accepted by --fingerprint-source
 const (
 	fingerprintSourceContent  = "content"
 	fingerprintSourceMetadata = "metadata"
@@ -138,6 +137,7 @@ func newSnapshotS3Cmd(out io.Writer) *cobra.Command {
 					o.fingerprintSource, validS3FingerprintSources))
 			}
 
+			// Changed covers env vars and config too: bindFlags applies them with Flags().Set.
 			return o.resolveDownloadLimits(cmd.Flags().Changed("download-concurrency"))
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -207,10 +207,6 @@ func (o *snapshotS3Options) run(args []string) error {
 // spells it; a test keeps the two equal.
 const defaultDownloadBudget = "512M"
 
-// resolveDownloadLimits validates the download flags into o.downloadLimits.
-// concurrencySet reports whether --download-concurrency was given by flag,
-// environment or config file; bindFlags applies the latter two through
-// Flags().Set, so all three mark the flag changed.
 func (o *snapshotS3Options) resolveDownloadLimits(concurrencySet bool) error {
 	if o.downloadConcurrency < 1 {
 		return fmt.Errorf("--download-concurrency must be at least 1, got %d", o.downloadConcurrency)
@@ -220,8 +216,6 @@ func (o *snapshotS3Options) resolveDownloadLimits(concurrencySet bool) error {
 		return fmt.Errorf("invalid --download-budget: %w", err)
 	}
 	concurrency := o.downloadConcurrency
-	// The flag's default is sized for download buffers, which reading stored
-	// checksums does not use.
 	if o.fingerprintSource == fingerprintSourceMetadata && !concurrencySet {
 		concurrency = aws.DefaultMetadataConcurrency
 	}

cmd/kosli/snapshotS3Limits_test.go

@@ -8,10 +8,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// The download defaults are sized for download buffers and temp disk, which a
-// source that reads stored checksums never uses, so metadata mode takes its own
-// concurrency default. An explicit --download-concurrency, from the flag, the
-// environment or the config file, still decides in either mode.
 func TestResolveDownloadLimitsPicksTheConcurrencyForTheSource(t *testing.T) {
 	for _, tc := range []struct {
 		name           string
@@ -44,8 +40,7 @@ func TestDefaultMetadataConcurrencyIsWiderThanTheDownloadDefault(t *testing.T) {
 		"a source that holds no buffers should not be throttled below the download default")
 }
 
-// cobra prints only the flag's own default, so the metadata default is spelt
-// out in the help text, and has to keep matching the constant.
+// cobra shows only the flag's own default, so the help text spells out the metadata one.
 func TestDownloadConcurrencyHelpStatesTheMetadataDefault(t *testing.T) {
 	require.Contains(t, downloadConcurrencyFlag, fmt.Sprintf("defaults to %d", aws.DefaultMetadataConcurrency))
 }

cmd/kosli/snapshotS3_test.go

@@ -40,8 +40,7 @@ func (suite *SnapshotS3TestSuite) SetupTest() {
 		"README.md":                  []byte("# kosli cli public\n"),
 		"dummy/dummy_2/template.yml": []byte("key: value\n"),
 	}
-	// Only README.md carries a stored checksum, so the metadata cases cover both
-	// an object that can be fingerprinted from metadata and one that cannot.
+	// Only README.md has a stored checksum, so the metadata cases can cover both outcomes.
 	readmeSum := sha256.Sum256(objects["README.md"])
 	aws.NewS3ClientFunc = func(_ *aws.AWSStaticCreds) (aws.S3API, error) {
 		return &aws.FakeS3Client{

internal/aws/aws.go

@@ -151,12 +151,9 @@ type S3DownloadAPI interface {
 	DownloadObject(ctx context.Context, params *transfermanager.DownloadObjectInput, optFns ...func(*transfermanager.Options)) (*transfermanager.DownloadObjectOutput, error)
 }
 
-// S3HeadAPI reads an object's metadata without reading the object itself,
-// including the checksum S3 stores for it. The real *s3.Client satisfies this
-// implicitly.
-//
-// The stored checksum is only returned when the request sets ChecksumMode to
-// ChecksumModeEnabled.
+// S3HeadAPI reads an object's metadata, including the checksum S3 stores for
+// it, which is returned only when the request sets ChecksumMode to
+// ChecksumModeEnabled. The real *s3.Client satisfies this implicitly.
 type S3HeadAPI interface {
 	HeadObject(ctx context.Context, params *s3.HeadObjectInput, optFns ...func(*s3.Options)) (*s3.HeadObjectOutput, error)
 }
@@ -400,9 +397,8 @@ func formatLambdaLastModified(lastModified string) (time.Time, error) {
 	return time.Parse(layout, lastModified)
 }
 
-// decodeBase64Sha256 converts a Base64-encoded SHA256 digest into the hex form
-// Kosli fingerprints use. AWS reports stored digests in Base64: Lambda's
-// CodeSha256 and an S3 object's checksum both arrive this way.
+// decodeBase64Sha256 converts a Base64 SHA256 digest, the form AWS reports
+// stored digests in, into the hex form Kosli fingerprints use.
 func decodeBase64Sha256(fingerprint string) (string, error) {
 	sha256base64, err := base64.StdEncoding.DecodeString(fingerprint)
 	if err != nil {
@@ -479,8 +475,7 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 }
 
 // getS3DataWithSource lists and filters the bucket, then fingerprints what is
-// left with digests from source. Everything but the digest source is shared, so
-// the sources cannot disagree on which objects a snapshot covers.
+// left with digests from source.
 func getS3DataWithSource(client S3API, source s3DigestSource, bucket string, includePaths, includeRegex, excludePaths, excludeRegex []string, limits DownloadLimits, logger *logger.Logger) ([]*S3Data, error) {
 	s3Data := []*S3Data{}
 
@@ -545,12 +540,10 @@ type DownloadLimits struct {
 // Lambda's default /tmp, and part buffers near 320 MiB of memory.
 var DefaultDownloadLimits = DownloadLimits{Concurrency: 8, BytesInFlight: 512 << 20}
 
-// DefaultMetadataConcurrency is how many checksum reads run at once when a
-// bucket is fingerprinted from S3 metadata. A HeadObject holds no part buffers
-// and no temp disk, so the download default says nothing about it. At a 30 ms
-// round trip, 32 in flight is around a thousand objects a second: a million in
-// under twenty minutes, and still well under the 5,500 reads a second S3
-// supports per prefix. The adaptive retryer absorbs throttling beyond that.
+// DefaultMetadataConcurrency is how many checksum reads run at once when
+// fingerprinting from S3 metadata. A HeadObject holds no part buffers or temp
+// disk, so it is sized against S3's request rate instead: 32 at a 30 ms round
+// trip is about 1,000 reads a second, well under the 5,500 S3 allows per prefix.
 const DefaultMetadataConcurrency = 32
 
 // listMatchingS3Objects lists the bucket, dropping folder markers and keys the
@@ -602,18 +595,15 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin
 	return objects, nil
 }
 
-// s3DigestSource is where the fingerprint pipeline gets each object's content
-// sha256 once the tree is known. Content mode downloads the object into the
-// pipeline's temp dir and hashes it; a source that reads S3's stored checksum
-// never touches the disk. Everything else -- key rule, .kosli_ignore, the tree
-// walk -- is shared, so the two sources cannot fingerprint the same bucket
-// differently.
+// s3DigestSource supplies each object's content sha256 to the fingerprint
+// pipeline. It is the only part that varies by source, so two sources cannot
+// fingerprint the same bucket differently.
 type s3DigestSource struct {
-	// sha256 returns the hex digest of one object's content. tempDir is scratch
-	// space the pipeline owns and removes when it is done.
+	// sha256 returns the hex digest of an object's content. tempDir is scratch
+	// space the pipeline removes afterwards.
 	sha256 func(ctx context.Context, tempDir string, object s3Object) (string, error)
-	// usesDisk reports whether an object's listed size occupies temp disk while
-	// sha256 runs, and so counts against DownloadLimits.BytesInFlight.
+	// usesDisk means an object's listed size occupies temp disk while sha256
+	// runs, so it counts against DownloadLimits.BytesInFlight.
 	usesDisk bool
 }
 
@@ -627,24 +617,19 @@ func downloadDigests(downloader S3DownloadAPI, bucket string, logger *logger.Log
 	}
 }
 
-// fingerprintS3Objects fingerprints the objects as the directory their keys
-// describe, downloading each one to an anonymous temp file, hashing it and
-// removing it. See fingerprintS3Tree for the pipeline.
+// fingerprintS3Objects fingerprints the objects with the download source.
 func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, limits DownloadLimits, logger *logger.Logger) (string, string, error) {
 	return fingerprintS3Tree(downloader, downloadDigests(downloader, bucket, logger), bucket, objects, limits, logger)
 }
 
 // fingerprintS3Tree fingerprints the objects as the directory their keys
-// describe, without ever using a key as a local file name. Each object's
-// content sha256 comes from source; the fingerprint is then computed from the
-// (key, sha256) pairs by digest.VirtualDirSha256, which reproduces what
-// digest.DirSha256 gives the same tree on disk. A single object is
-// fingerprinted as that file and named after it, as before.
+// describe, without using a key as a local file name: digest.VirtualDirSha256
+// combines the (key, sha256) pairs from source exactly as digest.DirSha256
+// would on disk. A single object is fingerprinted as that file and named
+// after it.
 //
-// A root .kosli_ignore is always downloaded first, whatever the source, because
-// its rules decide which other objects take part; objects the rules exclude are
-// not fetched at all. The remaining objects are fetched in parallel within
-// limits, and the first failure cancels the rest.
+// The root .kosli_ignore is always downloaded, whatever the source, because its
+// rules decide which other objects are fetched at all.
 func fingerprintS3Tree(downloader S3DownloadAPI, source s3DigestSource, bucket string, objects []s3Object, limits DownloadLimits, logger *logger.Logger) (string, string, error) {
 	keys := make([]string, len(objects))
 	for i, object := range objects {
@@ -675,8 +660,7 @@ func fingerprintS3Tree(downloader S3DownloadAPI, source s3DigestSource, bucket s
 	// when the objects were laid out on disk.
 	if file, ok := digest.SingleVirtualFile(files); ok {
 		fetch := source
-		// The root ignore file is always downloaded, so no source ever asks it
-		// for a checksum -- even alone, when its rules have nothing to decide.
+		// The ignore file is downloaded even when alone, so no source needs its checksum.
 		if file.Path == digest.IgnoreFileName {
 			fetch = downloadDigests(downloader, bucket, logger)
 		}
@@ -755,9 +739,8 @@ func ignoreRuleError(err error) error {
 // source and writes each into files at the same index. A fixed worker pool
 // bounds fetches and goroutines alike, a weighted semaphore bounds the listed
 // bytes of sources that use the disk, and the first transport error cancels the
-// context so nothing further starts. An unusableChecksumError is about one
-// object rather than the connection, so those are collected and reported
-// together once the rest have run.
+// context so nothing further starts. unusableChecksumErrors are collected
+// and reported together instead.
 func fetchS3DigestsInParallel(source s3DigestSource, tempDir string, objects []s3Object, indexes []int,
 	files []digest.VirtualFile, limits DownloadLimits, logger *logger.Logger) error {
 	ctx, cancel := context.WithCancel(context.Background())
@@ -786,7 +769,6 @@ func fetchS3DigestsInParallel(source s3DigestSource, tempDir string, objects []s
 			for i := range work {
 				object := objects[i]
 				// An object larger than the budget takes all of it and so runs alone.
-				// A source that never touches the disk owes the budget nothing.
 				var weight int64
 				if source.usesDisk {
 					weight = max(min(object.size, limits.BytesInFlight), 1)

internal/aws/fake_s3.go

@@ -18,16 +18,10 @@ import (
 var fakeS3LastModified = time.Date(2024, 1, 15, 10, 30, 0, 0, time.UTC)
 
 // FakeS3Checksum is the additional checksum S3 has stored for an object.
-// Objects uploaded without an explicit checksum algorithm have none, which is
-// why FakeS3Client.Checksums is keyed sparsely rather than derived from content.
 type FakeS3Checksum struct {
-	// SHA256 is Base64-encoded, as S3 returns it. A composite (multipart)
-	// checksum carries a "-N" part-count suffix and is a hash of the part
-	// hashes, not of the object content.
+	// SHA256 is Base64, as S3 returns it; a composite one ends in a "-N" part count.
 	SHA256 string
-	// Type is COMPOSITE for multipart uploads and FULL_OBJECT for whole-object
-	// checksums.
-	Type s3Types.ChecksumType
+	Type   s3Types.ChecksumType
 }
 
 // FakeS3Client is an in-memory implementation of S3API for testing.
@@ -46,9 +40,8 @@ type FakeS3Client struct {
 	// NoLastModified lists keys whose listing entry carries no LastModified at
 	// all, as some S3-compatible stores return.
 	NoLastModified map[string]bool
-	// Checksums maps object key to the additional checksum S3 has stored for
-	// it. A key with no entry has no additional checksum, as objects uploaded
-	// without --checksum-algorithm do, and HeadObject returns none for it.
+	// Checksums maps object key to its stored checksum. A key with no entry has
+	// none, like an object uploaded without --checksum-algorithm.
 	Checksums map[string]FakeS3Checksum
 	// PageSize controls how many objects are returned per ListObjectsV2 call.
 	// Defaults to 1000 (matching the AWS default) if zero.
@@ -183,8 +176,8 @@ func (f *FakeS3Client) HeadObject(_ context.Context, params *s3.HeadObjectInput,
 		LastModified:  aws.Time(f.lastModified(*params.Key)),
 	}
 
-	// S3 only returns a stored checksum when the request asks for it. Returning
-	// it unconditionally would hide a caller that forgets to set ChecksumMode.
+	// Withhold the checksum unless asked, as S3 does, so a caller that forgets
+	// ChecksumMode fails here too.
 	if params.ChecksumMode != s3Types.ChecksumModeEnabled {
 		return out, nil
 	}

internal/aws/s3_contract_test.go

@@ -21,8 +21,7 @@ import (
 // errInjected is the error tests inject into FakeS3Client to exercise error paths.
 var errInjected = errors.New("injected error")
 
-// base64Sha256 returns the Base64-encoded SHA256 of content, the form S3
-// reports a stored full-object checksum in.
+// base64Sha256 is content's SHA256 in the Base64 form S3 reports checksums in.
 func base64Sha256(content []byte) string {
 	sum := sha256.Sum256(content)
 	return base64.StdEncoding.EncodeToString(sum[:])
@@ -37,9 +36,9 @@ func base64Sha256(content []byte) string {
 //
 // bucket must name a bucket the client can see, holding at least two objects.
 // existingKey must name an object in that bucket with a non-empty body.
-// sha256ChecksumKey must name an object stored with an SHA256 checksum, or be
-// empty to skip the checksum sub-tests -- kosli-cli-public holds no such object
-// yet, and adding one would change the golden fingerprints TestGetS3Data pins.
+// sha256ChecksumKey names an object with a stored SHA256 checksum, or is empty
+// to skip those sub-tests: kosli-cli-public has none, and adding one would move
+// the fingerprints TestGetS3Data pins.
 func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey, sha256ChecksumKey string) {
 	t.Helper()
 
@@ -169,8 +168,6 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey, sha256C
 		if sha256ChecksumKey == "" {
 			t.Skip("no object with an SHA256 checksum available in this bucket")
 		}
-		// S3 only returns stored checksums when asked. A fake that always
-		// returned them would hide a caller that forgets to set ChecksumMode.
 		out, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{
 			Bucket: aws.String(bucket),
 			Key:    aws.String(sha256ChecksumKey),
@@ -192,9 +189,6 @@ func runS3ContractTests(t *testing.T, client S3API, bucket, existingKey, sha256C
 		require.NoError(t, err)
 		require.NotNil(t, out.ChecksumSHA256, "ChecksumSHA256 should be present")
 		require.NotEmpty(t, *out.ChecksumSHA256)
-		// A full-object checksum is plain Base64. A composite (multipart) one
-		// carries a "-N" part-count suffix, which is how both this codebase and
-		// the SDK's own response validation tell them apart.
 		require.NotContains(t, *out.ChecksumSHA256, "-",
 			"a single-part upload should carry a full-object checksum")
 		require.Equal(t, s3Types.ChecksumTypeFullObject, out.ChecksumType)