Skip to content

Commit 1567990

Browse files
committed
t,tq: make missing object push errors consistent
Since commit 1412d6e of PR git-lfs#3634, during push operations the Git LFS client has sometimes avoided reporting an error when an object to be pushed is missing locally if the remote server reports that it has a copy already. To implement this feature, a new Missing element was added to the Transfer and objectTuple structures in our "tq" package, and the Add() method of the TransferQueue structure in that package was updated to accept an additional "missing" flag, which the method uses to set the Missing element of the objectTuple structure it creates and sends to the "incoming" channel. Batches of objects to be pushed are then gathered from this channel by the collectBatches() method of the TransferQueue structure. As batches of objectTuple structures are collected, they are passed to the enqueueAndCollectRetriesFor() method, which converts them to Transfer structures using the ToTransfers() method and then passes them to the Batch() function, which is defined in our tq/api.go source file. This function initializes a batchRequest structure which contains the set of Transfer structures as its Objects element, and then passes those to the Batch() method specific to the current batch transfer adapter's structure. These Batch() methods return a BatchResponse structure, which the Batch() function then returns to the enqueueAndCollectRetriesFor() method. The BatchResponse structure also contains an Objects element which is another set of Transfer structures that represent the per-object metadata received from the remote server. After the enqueueAndCollectRetriesFor() method receives a BatchResponse during a push operation, if any of the Transfer structures in that response define an upload action to be performed, this implies that the remote server does not have a copy of those objects. As one of the changes we made in PR git-lfs#3634, we introduced a step into the enqueueAndCollectRetriesFor() method which halts the push operation if the server's response indicates that the server lacks a copy of an object, and if the "missing" value passed to the Add() method for that object was set to "true". (Initially, this step also decremented the count of the number of objects waiting to be transferred, but this created the potential for stalled push operations, and so another approach to handling an early exit from the batch transfer process was implemented in commit eb83fcd of PR git-lfs#3800.) Also in PR git-lfs#3634, several methods of the uploadContext structure in our "commands" package were revised to set the "missing" value for each object before calling the Add() method of the TransferQueue structure. Specifically, the ensureFile() method of the uploadContext structure first checks for the presence of the object data file in the .git/lfs/objects local storage directories. If that does not exist, then the method looks for a file in the working tree at the path associated with the Git LFS pointer that corresponds to the object. If that file also does not exist, and if the "lfs.allowIncompletePush" Git configuration option is set to "false", then the method returns "true", and this value is ultimately used for the "missing" argument in the call to the TransferQueue's Add() method for the object. Note that the file in the working tree may have any content, or be entirely empty; its simple presence is enough to change the value returned by the ensureFile() method, given the other conditions described above. We expect to revise this unintuitive behaviour in a subsequent commit in this PR. Before we make that change, however, we first adjust two aspects of the implementation from PR git-lfs#3634 so as to simplify our handling of missing objects during push operations. We made one of these adjustments in the previous commit in this PR, and we make the other in this commit. As noted above, the enqueueAndCollectRetriesFor() method halts a push operation if the server's response indicates that the server lacks a copy of an object, and if the "missing" value passed to the Add() method for that object was set to "true". When this occurs, the enqueueAndCollectRetriesFor() method outputs an error message that is distinct from the message which would otherwise be reported later, when the partitionTransfers() method of the TransferQueue rechecks whether individual objects' data files are present in the local storage directories. The message reported by the enqueueAndCollectRetriesFor() method when it abandons a push operation states that the client was "Unable to find source for object". This message was originally introduced in commit fea77e1 of PR git-lfs#3398, and was generated by the ensureFile() method of the uploadContext structure in our "commands" package, when that method was unable to locate either an object file in the local storage directories, or a corresponding file in the working tree at the path of the object's Git LFS pointer. As such, the wording of the message alludes to the expectation that the ensureFile() method will try to recreate a missing object file from a file in the working tree, i.e., from the object's "source". This is how the method is described in the code comments that precede it, and how it was intended to operate since it was first added in PR git-lfs#176. However, the ensureFile() has never actually recreated object files in this way, due to an oversight in its implementation, and given the challenges posed by the likelihood that files in the current working tree do not correspond exactly to the source of missing Git LFS object files, we expect to simply remove the ensureFile() method in a subsequent commit in this PR. In PR git-lfs#3634 the enqueueAndCollectRetriesFor() method of the TransferQueue structure was revised to halt push operations if the server reports that it requires the upload of an object for which the "missing" value provided to the TransferQueue's Add() method was set to "true". The error message reported in such a case was copied from the message formerly output by the ensureFile() method of the uploadContext structure, and that method was altered so it no longer generated this error message. This error message is not the only one reported by the Git LFS client when an object file is missing during a push operation, though, because it is only output when the ensureFile() method does not find a file (with any content) in the working at the path associated with the object's pointer. When a file does exist in the working tree, but the actual object file in the Git LFS local storage directories is missing, the push operation proceeds past the checks in the enqueueAndCollectRetriesFor() method and continues to the point where that method calls the addToAdapter() method of the TransferQueue structure. That method in turn calls the partitionTransfers() method to determine which of the current batch of objects to be uploaded have local object files present and which do not. If the partitionTransfers() method finds that an object file is missing for a given object, it creates a new MalformedObjectError with the "missing" element of that error structure set to "true". The method then instantiates a TransferResult structure for the object and sets the Error element of the TransferResult to the new MalformedObjectError. Finally, the method returns this TransferResult along with the other TransferResult structures it creates for all the other objects in the batch. The addToAdapter() method passes these TransferResult structures individually to the handleTransferResult() method, which checks whether the Error element is defined for the given TransferResult. If an error was encountered, and is one which indicates the object transfer should not be retried, then the handleTransferResult() method sends the error to the TransferQueue's "errorc" channel. For errors of the MalformedObjectError type, this is always the case. During a push operation, the CollectErrors() method of the uploadContext structure in the "commands" package receives these errors from the channel, and if they are errors of the MalformedObjectError type and have a "true" values in their "missing" element, the object's ID and the filename associated with the object's pointer are recorded in the "missing" map of the uploadContext structure. When the ReportErrors() method of the uploadContext structure is then run, it iterates over the keys and values of the "missing" map and outputs an error message containing both the object ID and the associated filename of the object's pointer, along with a "(missing)" prefix. As well, a leading error message is output, whose exact text depends on the value of the "lfs.allowIncompletePush" configuration option, and if this option is set to its default value of "false", several trailing error messages are output which provide a hint as to how to set that option if the user wants to allow a subsequent push operation with missing objects to proceed to completion as best it can. These per-object error messages were first defined in commit 9be11e8 of PR git-lfs#2082, and the trailing hints were added in commit f5f5731 of PR git-lfs#3109, at which time the "lfs.allowIncompletePush" option's default values was changed to "false". As mentioned above, in a subsequent commit in this PR we expect to remove the ensureFile() method of the uploadContext structure. We still expect to perform a check for missing object files in the uploadTransfer() method, though, as this will typically find any missing object files at the start of a push operation, and thereby allow the enqueueAndCollectRetriesFor() method of the TransferQueue structure to halt the operation as soon as one of those objects is found to be required by the remote server. For the time being, we will leave the secondary check for missing object files in place in the partitionTransfers() method of the TransferQueue structure, as this method also tests the size of the object file and reports those with unexpected sizes as corrupt. Nevertheless, we would like to make our error messages as consistent as possible when handling missing object files. Therefore we revise the enqueueAndCollectRetriesFor() method so it no longer returns the "Unable to find source for object" error message, but instead returns a new MalformedObjectError with a "true" value for its "missing" element. One advantage of this change is that we remove the somewhat stale wording of the previous message, which reflected the assumption that the ensureFile() method of the uploadContext structure would attempt to recreate missing object files from "source" files in the working tree, even though the method has never actually done so. Another advantage is that by returning a MalformedObjectError, the existing logic of the CollectErrors() and ReportErrors() methods of the uploadContext structure will handle the error exactly as if it had been generated by the TransferQueue's partitionTransfers() method, and will output both the same leading error message and trailing hint messages as in that case. As a result, we also adjust several tests in our t/t-pre-push.sh and t/t-push-failures-local.sh scripts to expect the error messages output by the ReportErrors() method instead of the message previously generated by the enqueueAndCollectRetriesFor() method.
1 parent 77f8d1f commit 1567990

File tree

3 files changed

+7
-4
lines changed

3 files changed

+7
-4
lines changed

t/t-pre-push.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,8 @@ begin_test "pre-push reject missing object (lfs.allowincompletepush default)"
449449
fi
450450

451451
grep "tq: stopping batched queue, object \"$missing_oid\" missing locally and on remote" push.log
452-
grep "Unable to find source for object $missing_oid" push.log
452+
grep "LFS upload failed:" push.log
453+
grep " (missing) missing.dat ($missing_oid)" push.log
453454

454455
refute_server_object "$reponame" "$present_oid"
455456
refute_server_object "$reponame" "$missing_oid"

t/t-push-failures-local.sh

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,8 @@ begin_test "push reject missing object (lfs.allowincompletepush false)"
128128
fi
129129

130130
grep "tq: stopping batched queue, object \"$missing_oid\" missing locally and on remote" push.log
131-
grep "Unable to find source for object $missing_oid" push.log
131+
grep "LFS upload failed:" push.log
132+
grep " (missing) missing.dat ($missing_oid)" push.log
132133

133134
refute_server_object "$reponame" "$present_oid"
134135
refute_server_object "$reponame" "$missing_oid"
@@ -179,7 +180,8 @@ begin_test "push reject missing object (lfs.allowincompletepush false) (git-lfs-
179180
grep "pure SSH connection successful" push.log
180181

181182
grep "tq: stopping batched queue, object \"$missing_oid\" missing locally and on remote" push.log
182-
grep "Unable to find source for object $missing_oid" push.log
183+
grep "LFS upload failed:" push.log
184+
grep " (missing) missing.dat ($missing_oid)" push.log
183185

184186
refute_remote_object "$reponame" "$present_oid"
185187
refute_remote_object "$reponame" "$missing_oid"

tq/transfer_queue.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ func (q *TransferQueue) enqueueAndCollectRetriesFor(batch batch) (batch, error)
630630
// transfer queue.
631631
if ok && objects.First().Missing {
632632
tracerx.Printf("tq: stopping batched queue, object %q missing locally and on remote", o.Oid)
633-
return nil, errors.New(tr.Tr.Get("Unable to find source for object %v (try running `git lfs fetch --all`)", o.Oid))
633+
return nil, newObjectMissingError(objects.First().Name, o.Oid)
634634
}
635635
}
636636
}

0 commit comments

Comments
 (0)