ref/store: Shape Batch a bit more like Transaction and eagerly validate what we could.

6 files changed, 66 insertions, 46 deletions

diff --git a/network/receivepack/service/apply.go b/network/receivepack/service/apply.go
index 8dcc004f..fdf3eef6 100644
--- a/network/receivepack/service/apply.go
+++ b/network/receivepack/service/apply.go
@@ -57,7 +57,15 @@ func (service *Service) applyBatch(result *Result, commands []Command) error {
 	}
 
 	for _, command := range commands {
-		queueWriteBatch(batch, command)
+		err = queueWriteBatch(batch, command)
+		if err != nil {
+			_ = batch.Abort()
+
+			fillCommandErrors(result, commands, err.Error())
+			utils.BestEffortFprintf(service.opts.Progress, "updating refs: failed while queueing batch.\n")
+
+			return nil
+		}
 	}
 
 	batchResults, err := batch.Apply()
@@ -107,20 +115,16 @@ func queueWriteTransaction(tx refstore.Transaction, command Command) error {
 	return tx.Update(command.Name, command.NewID, command.OldID)
 }
 
-func queueWriteBatch(batch refstore.Batch, command Command) {
+func queueWriteBatch(batch refstore.Batch, command Command) error {
 	if isDelete(command) {
-		batch.Delete(command.Name, command.OldID)
-
-		return
+		return batch.Delete(command.Name, command.OldID)
 	}
 
 	if command.OldID == command.OldID.Algorithm().Zero() {
-		batch.Create(command.Name, command.NewID)
-
-		return
+		return batch.Create(command.Name, command.NewID)
 	}
 
-	batch.Update(command.Name, command.NewID, command.OldID)
+	return batch.Update(command.Name, command.NewID, command.OldID)
 }
 
 func successCommandResult(command Command) CommandResult {
diff --git a/ref/store/batch.go b/ref/store/batch.go
index 765846d6..11423cec 100644
--- a/ref/store/batch.go
+++ b/ref/store/batch.go
@@ -14,32 +14,35 @@ import objectid "codeberg.org/lindenii/furgit/object/id"
 type Batch interface {
 	// Create creates one detached reference, requiring that the logical
 	// reference does not already exist.
-	Create(name string, newID objectid.ObjectID)
+	Create(name string, newID objectid.ObjectID) error
 	// Update updates one detached reference, requiring that the current logical
 	// reference value matches oldID.
-	Update(name string, newID, oldID objectid.ObjectID)
+	Update(name string, newID, oldID objectid.ObjectID) error
 	// Delete deletes one detached reference, requiring that the current logical
 	// reference value matches oldID.
-	Delete(name string, oldID objectid.ObjectID)
+	Delete(name string, oldID objectid.ObjectID) error
 	// Verify verifies that the current logical reference value matches oldID.
-	Verify(name string, oldID objectid.ObjectID)
+	Verify(name string, oldID objectid.ObjectID) error
 
 	// CreateSymbolic creates one symbolic reference, requiring that the named
 	// reference does not already exist.
-	CreateSymbolic(name, newTarget string)
+	CreateSymbolic(name, newTarget string) error
 	// UpdateSymbolic updates one symbolic reference directly, requiring that its
 	// current target matches oldTarget.
-	UpdateSymbolic(name, newTarget, oldTarget string)
+	UpdateSymbolic(name, newTarget, oldTarget string) error
 	// DeleteSymbolic deletes one symbolic reference directly, requiring that its
 	// current target matches oldTarget.
-	DeleteSymbolic(name, oldTarget string)
+	DeleteSymbolic(name, oldTarget string) error
 	// VerifySymbolic verifies that the named symbolic reference currently points
 	// at oldTarget.
-	VerifySymbolic(name, oldTarget string)
+	VerifySymbolic(name, oldTarget string) error
 
 	// Apply validates and applies queued operations, returning one result per
 	// queued operation in order. Fatal backend failures are returned separately.
 	//
+	// Malformed operations are rejected by the queueing methods above and do not
+	// enter the batch.
+	//
 	// Apply invalidates the receiver.
 	Apply() ([]BatchResult, error)
 	// Abort abandons the batch and releases any resources it holds.
diff --git a/ref/store/files/batch_apply.go b/ref/store/files/batch_apply.go
index b4be9079..1847b544 100644
--- a/ref/store/files/batch_apply.go
+++ b/ref/store/files/batch_apply.go
@@ -13,14 +13,6 @@ func (batch *Batch) Apply() ([]refstore.BatchResult, error) {
 	for i, op := range batch.ops {
 		results[i].Name = op.name
 
-		err := executor.validateQueuedUpdate(op)
-		if err != nil {
-			results[i].Status = refstore.BatchStatusRejected
-			results[i].Error = batchResultError(err)
-
-			continue
-		}
-
 		target, err := executor.resolveQueuedUpdateTarget(op)
 		if err != nil {
 			if isBatchRejected(err) {
diff --git a/ref/store/files/batch_queue.go b/ref/store/files/batch_queue.go
index 5937c6fb..afec5a78 100644
--- a/ref/store/files/batch_queue.go
+++ b/ref/store/files/batch_queue.go
@@ -1,5 +1,12 @@
 package files
 
-func (batch *Batch) queue(op queuedUpdate) {
+func (batch *Batch) queue(op queuedUpdate) error {
+	err := (&refUpdateExecutor{store: batch.store}).validateQueuedUpdate(op)
+	if err != nil {
+		return err
+	}
+
 	batch.ops = append(batch.ops, op)
+
+	return nil
 }
diff --git a/ref/store/files/batch_queue_ops.go b/ref/store/files/batch_queue_ops.go
index a99b3c89..441bcaba 100644
--- a/ref/store/files/batch_queue_ops.go
+++ b/ref/store/files/batch_queue_ops.go
@@ -3,41 +3,41 @@ package files
 import objectid "codeberg.org/lindenii/furgit/object/id"
 
 // Create queues a detached reference creation.
-func (batch *Batch) Create(name string, newID objectid.ObjectID) {
-	batch.queue(queuedUpdate{name: name, kind: updateCreate, newID: newID})
+func (batch *Batch) Create(name string, newID objectid.ObjectID) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateCreate, newID: newID})
 }
 
 // Update queues a detached reference update.
-func (batch *Batch) Update(name string, newID, oldID objectid.ObjectID) {
-	batch.queue(queuedUpdate{name: name, kind: updateReplace, newID: newID, oldID: oldID})
+func (batch *Batch) Update(name string, newID, oldID objectid.ObjectID) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateReplace, newID: newID, oldID: oldID})
 }
 
 // Delete queues a detached reference deletion.
-func (batch *Batch) Delete(name string, oldID objectid.ObjectID) {
-	batch.queue(queuedUpdate{name: name, kind: updateDelete, oldID: oldID})
+func (batch *Batch) Delete(name string, oldID objectid.ObjectID) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateDelete, oldID: oldID})
 }
 
 // Verify queues a detached reference verification.
-func (batch *Batch) Verify(name string, oldID objectid.ObjectID) {
-	batch.queue(queuedUpdate{name: name, kind: updateVerify, oldID: oldID})
+func (batch *Batch) Verify(name string, oldID objectid.ObjectID) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateVerify, oldID: oldID})
 }
 
 // CreateSymbolic queues a symbolic reference creation.
-func (batch *Batch) CreateSymbolic(name, newTarget string) {
-	batch.queue(queuedUpdate{name: name, kind: updateCreateSymbolic, newTarget: newTarget})
+func (batch *Batch) CreateSymbolic(name, newTarget string) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateCreateSymbolic, newTarget: newTarget})
 }
 
 // UpdateSymbolic queues a symbolic reference update.
-func (batch *Batch) UpdateSymbolic(name, newTarget, oldTarget string) {
-	batch.queue(queuedUpdate{name: name, kind: updateReplaceSymbolic, newTarget: newTarget, oldTarget: oldTarget})
+func (batch *Batch) UpdateSymbolic(name, newTarget, oldTarget string) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateReplaceSymbolic, newTarget: newTarget, oldTarget: oldTarget})
 }
 
 // DeleteSymbolic queues a symbolic reference deletion.
-func (batch *Batch) DeleteSymbolic(name, oldTarget string) {
-	batch.queue(queuedUpdate{name: name, kind: updateDeleteSymbolic, oldTarget: oldTarget})
+func (batch *Batch) DeleteSymbolic(name, oldTarget string) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateDeleteSymbolic, oldTarget: oldTarget})
 }
 
 // VerifySymbolic queues a symbolic reference verification.
-func (batch *Batch) VerifySymbolic(name, oldTarget string) {
-	batch.queue(queuedUpdate{name: name, kind: updateVerifySymbolic, oldTarget: oldTarget})
+func (batch *Batch) VerifySymbolic(name, oldTarget string) error {
+	return batch.queue(queuedUpdate{name: name, kind: updateVerifySymbolic, oldTarget: oldTarget})
 }
diff --git a/ref/store/files/batch_test.go b/ref/store/files/batch_test.go
index 55ea4d0a..80575e5e 100644
--- a/ref/store/files/batch_test.go
+++ b/ref/store/files/batch_test.go
@@ -29,8 +29,15 @@ func TestBatchApplyRejectsStaleDeleteAndAppliesIndependentDelete(t *testing.T) {
 			t.Fatalf("BeginBatch: %v", err)
 		}
 
-		batch.Delete("refs/heads/main", staleID)
-		batch.Delete("refs/heads/topic", commitID)
+		err = batch.Delete("refs/heads/main", staleID)
+		if err != nil {
+			t.Fatalf("Delete(main) queue: %v", err)
+		}
+
+		err = batch.Delete("refs/heads/topic", commitID)
+		if err != nil {
+			t.Fatalf("Delete(topic) queue: %v", err)
+		}
 
 		results, err := batch.Apply()
 		if err != nil {
@@ -83,8 +90,15 @@ func TestBatchApplyRejectsDuplicateQueuedRef(t *testing.T) {
 			t.Fatalf("BeginBatch: %v", err)
 		}
 
-		batch.Delete("refs/heads/main", commitID)
-		batch.Verify("refs/heads/main", commitID)
+		err = batch.Delete("refs/heads/main", commitID)
+		if err != nil {
+			t.Fatalf("Delete(main) queue: %v", err)
+		}
+
+		err = batch.Verify("refs/heads/main", commitID)
+		if err != nil {
+			t.Fatalf("Verify(main) queue: %v", err)
+		}
 
 		results, err := batch.Apply()
 		if err != nil {