/go/store/blobstore: pr feedback

go/store/blobstore/git_blobstore.go

@@ -263,6 +263,12 @@ func (gbs *GitBlobstore) buildPutCommit(ctx context.Context, parent git.OID, has
 		}
 	}
 
+	// TODO(gitblobstore): Decide on a policy for file-vs-directory prefix conflicts when staging keys.
+	// For example, staging "a" when "a/b" already exists in the tree/index (or vice-versa) can fail
+	// with a git index error (path appears as both a file and directory). Today our NBS keyspace is
+	// flat (e.g. "manifest", "<tableid>", "<tableid>.records"), so this should not occur. If we ever
+	// namespace keys into directories, consider proactively removing conflicting paths from the index
+	// before UpdateIndexCacheInfo so Put/CheckAndPut remain robust.
 	if err := gbs.api.UpdateIndexCacheInfo(ctx, indexFile, "100644", blobOID, key); err != nil {
 		return "", "", err
 	}
@@ -312,12 +318,22 @@ func isMissingGitIdentityErr(err error) bool {
 }
 
 func newTempIndex() (dir, indexFile string, cleanup func(), err error) {
-	dir, err = os.MkdirTemp("", "dolt-gitblobstore-index-")
+	// Create a unique temp index file. This is intentionally *not* placed under GIT_DIR:
+	// - some git dirs may be read-only or otherwise unsuitable for scratch files
+	// - we don't want to leave temp files inside the repo on crashes
+	//
+	// Note: git will also create a sibling lock file (<index>.lock) during index writes.
+	f, err := os.CreateTemp("", "dolt-gitblobstore-index-")
 	if err != nil {
 		return "", "", nil, err
 	}
-	indexFile = filepath.Join(dir, "index")
-	cleanup = func() { _ = os.RemoveAll(dir) }
+	indexFile = f.Name()
+	_ = f.Close()
+	dir = filepath.Dir(indexFile)
+	cleanup = func() {
+		_ = os.Remove(indexFile)
+		_ = os.Remove(indexFile + ".lock")
+	}
 	return dir, indexFile, cleanup, nil
 }
 

go/store/blobstore/internal/git/impl_test.go

@@ -238,6 +238,134 @@ func TestGitAPIImpl_WriteTree_FromEmptyIndex(t *testing.T) {
 	}
 }
 
+func TestGitAPIImpl_UpdateIndexCacheInfo_ReplacesExistingEntry(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	repo, err := gitrepo.InitBareTemp(ctx, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r, err := NewRunner(repo.GitDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	api := NewGitAPIImpl(r)
+
+	indexFile := tempIndexFile(t)
+	if err := api.ReadTreeEmpty(ctx, indexFile); err != nil {
+		t.Fatal(err)
+	}
+
+	path := "same.txt"
+
+	oid1, err := api.HashObject(ctx, bytes.NewReader([]byte("one\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := api.UpdateIndexCacheInfo(ctx, indexFile, "100644", oid1, path); err != nil {
+		t.Fatal(err)
+	}
+
+	// Update the same path again; this should succeed and replace the index entry.
+	oid2, err := api.HashObject(ctx, bytes.NewReader([]byte("two\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := api.UpdateIndexCacheInfo(ctx, indexFile, "100644", oid2, path); err != nil {
+		t.Fatal(err)
+	}
+
+	treeOID, err := api.WriteTree(ctx, indexFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	commitOID, err := api.CommitTree(ctx, treeOID, nil, "replace entry", testAuthor())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	gotBlobOID, err := api.ResolvePathBlob(ctx, commitOID, path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	rc, err := api.BlobReader(ctx, gotBlobOID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rc.Close()
+	got, err := io.ReadAll(rc)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(got, []byte("two\n")) {
+		t.Fatalf("expected replacement contents %q, got %q", "two\n", string(got))
+	}
+}
+
+func TestGitAPIImpl_UpdateIndexCacheInfo_FileDirectoryConflictErrors(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	repo, err := gitrepo.InitBareTemp(ctx, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	r, err := NewRunner(repo.GitDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	api := NewGitAPIImpl(r)
+
+	indexFile := tempIndexFile(t)
+	if err := api.ReadTreeEmpty(ctx, indexFile); err != nil {
+		t.Fatal(err)
+	}
+
+	oidDirChild, err := api.HashObject(ctx, bytes.NewReader([]byte("child\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := api.UpdateIndexCacheInfo(ctx, indexFile, "100644", oidDirChild, "a/b.txt"); err != nil {
+		t.Fatal(err)
+	}
+
+	// Now try to stage "a" as a file; git should reject this (file vs directory conflict).
+	oidA, err := api.HashObject(ctx, bytes.NewReader([]byte("a\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = api.UpdateIndexCacheInfo(ctx, indexFile, "100644", oidA, "a")
+	if err == nil {
+		t.Fatalf("expected conflict error staging %q when %q exists", "a", "a/b.txt")
+	}
+
+	// Inverse conflict: stage "x" as a file, then try to stage "x/y".
+	indexFile2 := tempIndexFile(t)
+	if err := api.ReadTreeEmpty(ctx, indexFile2); err != nil {
+		t.Fatal(err)
+	}
+
+	oidX, err := api.HashObject(ctx, bytes.NewReader([]byte("x\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := api.UpdateIndexCacheInfo(ctx, indexFile2, "100644", oidX, "x"); err != nil {
+		t.Fatal(err)
+	}
+
+	oidXY, err := api.HashObject(ctx, bytes.NewReader([]byte("xy\n")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = api.UpdateIndexCacheInfo(ctx, indexFile2, "100644", oidXY, "x/y.txt")
+	if err == nil {
+		t.Fatalf("expected conflict error staging %q when %q exists", "x/y.txt", "x")
+	}
+}
+
 func TestGitAPIImpl_ReadTree_PreservesExistingPaths(t *testing.T) {
 	t.Parallel()