[BUG] LFS objects are not always detected!

[goran-w]

In methods CommitDetail.SetViewingBlobAsync() and FileHistoriesSingleRevision.GetRevisionFileContentAsync(), when processing a ObjectType.Blob the Commands.IsBinary() check is made BEFORE the LFSObject.Parse() check!

This sometimes causes an LFS object to be wrongly detected as a Binary object and displayed as such, which is clearly a bug. Currently, this in turn leads to either (wrongly) displaying the BINARY FILE NOT SUPPORTED!!! indication, or trying to display the file as an image but failing.
(Example: JPG 0 x 0 0 B)

The fix is simple - make sure to check if the Blob uses LFS before checking if it's Binary / Text.

[love-linger]

For LFS object, git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD -- <obj_path> command which is used by Commands.IsBinary() will output like this

This is because LFS pointer file is a simple text file, for example:

[goran-w]

Well, that does not always work, apparently! I have a repo where this test fails on all my LFS files. Example:

$ git lfs fsck
Git LFS fsck OK

$ git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD -- image.jpg
diff --git a/image.jpg b/image.jpg
new file mode 100644
index 0000000..b391e6f
Binary files /dev/null and b/image.jpg differ

$ git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD --stat -- image.jpg
 .../image.jpg        | Bin 0 -> 132 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

$ git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904 HEAD --numstat -- image.jpg
-       -       image.jpg

$ git show HEAD:image.jpg
version https://git-lfs.github.com/spec/v1
oid sha256:e5f827d9e66f44d236fd8b389f16d579ccff79a55c3e2f390f8b9e53d3f3c250
size 2795537

$ git ls-files -- image.jpg
image.jpg

$ git ls-files --eol -- image.jpg
i/lf    w/-text attr/-text              image.jpg

$ git lfs ls-files --include image.jpg
e5f827d9e6 * image.jpg

[goran-w]

Aha, I found out why. In my .gitattributes for this repo, I have (by mistake/experimentation) the following spec for LFS files in this specific repo :
*.jpg filter=lfs diff=lfs merge=lfs binary

That's instead of this much more common spec :
*.jpg filter=lfs diff=lfs merge=lfs -text

The binary spec is an alias expanding to -text -diff, so it's a bit redundant here, and basically unsets the diff=lfs and merge=lfs specs.

Changing this solves the problem, since (I think) the -diff part is what makes git diff <A> <B> --numstat report the file as binary.

However, I'm wondering if it's not still safer and more robust (plus easier to follow the logic in the code) if we first check for LFS and only then for Binary or Text? (If there are performance considerations around checking LFS first, maybe we could check LFS only if the blob-size is "reasonably" small for a LFS-pointer file?)

[love-linger]

You are right. When the diff=lfs and merge=lfs configurations are unsetted by binary, not only the revision file viewer can not display the content, the diff viewer also displays the wrong result.

Currently, I don't know how to be compatible with this mode. I also checked SourceTree, GitExtensions and Fork, they also have issues with binary mode

[goran-w]

Currently, I don't know how to be compatible with this mode

Well, I did test my suggested fix of moving the LFS check (in both the mentioned methods) to before we check for Binary vs Text, and that solves the issue. (Since once we actually check the file contents, it's found to be text anyway, when it's an LFS pointer file.)

As mentioned, there may be performance issues associated with extracting (?) the whole blob content in order to check for LFS pointer data, in the case where it's a large file that's NOT handled by LFS. But this could probably be avoided, by first checking the size of the blob contents, since an LFS pointer file can only be of relatively small size (basically limited by the maximum path length of the file system + some header overhead). This would probably be a good pre-check, regardless...

BTW, while looking at this I found that the "core" parts of the the two mentioned methods, for the case of a ObjectType.Blob, have quite similar logic and could probably be using a single (somewhat adjusted) helper method, to reduce duplicated code?

[love-linger]

For LFS image

• diff=lfs merge=lfs binary

• diff=lfs merge=lfs -text

For normal LFS binary file

• diff=lfs merge=lfs binary

• diff=lfs merge=lfs -text

You can see that it not only affects the revision file content view, it also affects the diff command.

[goran-w]

Ah, yes I see - I only briefly tested my changes on the two instances where the IsBinary() method was called.

Not sure how much effort should be put into supporting this "misconfigured" case, after all... 😅

But anyway, for reference, I did some more digging and found the following useful info :

• Since version 2.7.0 of Git-LFS, the low-level command git lfs pointer has a --check flag, which can be used to report whether a given --file or the stream-content on --stdin contains well-formed LFS-pointer data or not.
• Given a blob-SHA, this command-line can be used to check if the blob is an LFS pointer-file:
  • git cat-file blob <sha> | git lfs pointer --check --stdin
  • This command exits with code 0 if the content can be parsed as a LFS-pointer, and exits with code 1 otherwise.
• This command-line is supposed to be quite efficient, since even for very large blobs the opened stream will be closed after reading the very first chunk(s) - just enough bytes to conclude if it contains LFS-pointer data or not.
• Since the command-line works on blobs, it could (in theory) be used to check if one or both of the 2 blobs involved in a diff are LFS pointers.
• WARNING: The variant git lfs pointer --check --file=<file-path> seems to do an early exit if the file is classified as binary will check the LFS-filtered file contents, so it will always exit with code 1 unless the raw pointer-file has been left unfiltered (by having skipped the smudge filter) !
• (For more background, see the discussion in the PR that implemented this flag.)

[goran-w]

☝️ NOTE: My previous comment contained some mistakes at first, but I've now checked and corrected these...

[goran-w]

@love-linger We can close this issue now, if no further fix is being planned for this (fringe) LFS issue...