Skip to content

commands,tq: specialized logging for missing/corrupt objects#2082

Merged
ttaylorr merged 9 commits intomasterfrom
tq-malformed-object
Mar 25, 2017
Merged

commands,tq: specialized logging for missing/corrupt objects#2082
ttaylorr merged 9 commits intomasterfrom
tq-malformed-object

Conversation

@ttaylorr
Copy link
Contributor

This pull request is a continuation of #2078 following up on #2078 (comment) with #2078 (comment).

Turns out that our *tq.TransferQueue already had some early returns built in for objects that are a) missing or b) corrupt. I taught the *tq.TransferQueue and the *commands.uploader types to recognize and log them appropriately.

From #2078 (comment), this allows:

  • All of the LFS objects that do exist to travel to the remote as early as possible
  • A user to see all of the missing objects in one push instead of getting one error at a time
  • A user to "fix" the missing object later, and have a smaller push size when they complete the fix.

With three files, missing.dat, corrupted.dat, and present.dat, the output looks like this:

~/g/git-lfs-test (master) $ git push origin master
Locking support detected on remote "origin". Consider enabling it with:
  $ git config 'lfs.http://127.0.0.1:61507/push-missing-objects.git/info/lfs.locksverify' true
Git LFS: (1 of 3 files) 7 B / 21 B
Push completed with missing objects:
  (missing) missing.dat (ffa63583dfa6706b87d284b86b0d693a161e4840aad2c5cf6b5d27c3b9621f7d)
  (corrupt) corrupt.dat (11d510e067d2cdcd7559bd86d27a2f4c20babd43670346b97af99b522c1f0075)
error: failed to push some refs to 'http://127.0.0.1:61507/push-missing-objects'
  • missing.dat & corrupted.dat are not uploaded
  • present.dat is uploaded to the LFS server
  • No Git blobs are uploaded to the Git server

/cc @git-lfs/core @shana

@ttaylorr ttaylorr added this to the v2.1.0 milestone Mar 24, 2017
@ttaylorr ttaylorr requested a review from technoweenie March 24, 2017 20:45
@ttaylorr ttaylorr modified the milestones: v2.0.2, v2.1.0 Mar 24, 2017
}

if len(missing) > 0 || len(corrupt) > 0 {
Print("Push completed with missing objects:")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this message makes it seem the upload was successful. What about something like LFS Upload failed:?

corrupt[malformed.Name] = malformed.Oid
}
} else {
FullError(err)
Copy link
Contributor

@technoweenie technoweenie Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about just collecting all the errors here, and not calling FullError()? It looks like partitionTransfers() can only send one other potential error:

errors.Errorf("Git LFS: object %q has invalid size (got: %d)", t.Oid, t.Size)

Even though that's technically a server issue, I don't think it's worth halting the push command over. It should just be listed below.

@ttaylorr
Copy link
Contributor Author

@technoweenie thanks for the review: I pushed up 0be9add & 52f04a9 to address.

I think going forward we can do some sort of "streaming" error reporting, where server/filesystem errors can be reported as they happen. We'd have to re-draw the progress meter below each error as we receive them so as not to create phantom progress meters, but this is doable.

I think this is an OK stopping point for now 👍 .

@ttaylorr ttaylorr merged commit 4eaaaa2 into master Mar 25, 2017
@ttaylorr ttaylorr deleted the tq-malformed-object branch March 25, 2017 04:13
ttaylorr added a commit that referenced this pull request Mar 25, 2017
Backport #2082 for v2.0.x: commands,tq: specialized logging for missing/corrupt objects
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 18, 2024
In commit 5e654f2 in PR git-lfs#565 a pair
of test assertion functions were added to the forerunner of our
current t/testhelpers.sh shell library.  These assert_local_object()
and refute_local_object() functions check for the presence or absence
of a file in the object cache maintained by the Git LFS client in
a local repository.

To perform these checks, the functions capture the output of the
"git lfs env" command and parse the contents of the LocalMediaDir
line, which reports the full path to the Git LFS object cache
location.  To retrieve the path, the functions ignore the first 14
characters of the line, as that corresponds to the length of the
LocalMediaDir field name (13 characters) plus one character in order
to account for the equals sign which follows the field name.

Later PRs have added three other assertion functions that follow
the same design.  The delete_local_object() function was added in
commit 97434fe of PR git-lfs#742 to help test
the "git lfs fetch" command's --prune option, the corrupt_local_object()
function was added in commit 4b0f50e
of PR git-lfs#2082 to help test the detection of corrupted local objects
during push operations, and most recently, the assert_remote_object()
function was added in commit 9bae8eb
of PR git-lfs#5905 to improve our tests of the SSH object transfer protocol
for Git LFS.

All of these functions retrieve the object cache location by ignoring
the first 14 characters from the LocalMediaDir line in the output of
the "git lfs env" command.  However, the refute_local_object() function
contains a hint of an alternative approach to parsing this line's data.

A local "regex" variable is defined in the refute_local_object()
function, which matches the LocalMediaDir field name and equals sign
and captures the subsequent object cache path value.  Although this
"regex" variable was included when the function was first introduced,
it has never been used, and does not appear in any of the other similar
functions.

While reviewing PR git-lfs#5905, larsxschneider suggested an even simpler
option than using a regular expression to extract the object cache
path from the LocalMediaDir line.  Rather than asking the Bash shell
to start its parameter expansion at a fixed offset of 14 characters
into the string, we can define a pattern which matches the leading
LocalMediaDir field name and equals sign and specify that the shell
should remove that portion of the string during parameter expansion.
See also the discussion in this review comment from PR git-lfs#5905:

  git-lfs#5905 (comment)

In addition to these changes, we can remove the definition of the
"regex" variable from the refute_local_object() function, as it
remains unused.

Co-authored-by: Lars Schneider <larsxschneider@github.com>
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Apr 3, 2025
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Apr 3, 2025
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.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request May 22, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants