Skip to content

Conversation

mstv
Copy link
Member

@mstv mstv commented Nov 29, 2024

Fixes #6146
Finishes #6189

Proposed changes

  • Detect LFS pointer files and apply the LFS smudge filter in this case - in order to always get the plain file contents

Screenshots

Before

image

After

image

Test methodology

image

  • created an invalid pointer file, which cannot be pushed to GH

image

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Nov 29, 2024
@gerhardol
Copy link
Member

How can you see if a file is lfs now?
That is my primary use case, to see that a binary file is properly added.

Save as may be useful for some, sure.

Diff and view is not interesting for me, these files are binary or very large normally, it is just to get something that seem complete.

@mstv
Copy link
Member Author

mstv commented Nov 30, 2024

How can you see if a file is lfs now? That is my primary use case, to see that a binary file is properly added.

What is missing for you?

LFS file added:

a.bin is a file which is not detected as binary by GE's heuristic.

image

0.bin contains a zero byte. I admit, there is no clear indication that the file "differs" because of being turned into an LFS pointer. At least, one can see that the file is affected.
But this affects only the commit where files are moved to LFS belatedly - (which should be avoided anyway).

image

LFS file changed:

(looks the same regardless of the heuristic for binary files)

image

Large text files

a.long is handled differently on purpose. It has been configured to be diffed as text by defining an adapted diff filter:

git config diff.lfs-text.textconv cat
*.long filter=lfs diff=lfs-text merge=lfs -text

New LFS file

looks the same for "Index" and real commit

image

If really in doubt, there is a mix of related LFS commands:

git lfs ls-files --all
git lfs status
git lfs fsck
git add --renormalize *

Save as may be useful for some, sure.

It was useless and rather a bug because of its unexpected behavior. (It transparently works for difftool. Why not for "Save as" and for displaying the picture of the second revision?)

@mstv
Copy link
Member Author

mstv commented Nov 30, 2024

A missing separate feature is to provide a context menu item for git lfs track <file.ext>.
I think of a TaskDialog asking whether to track that file or all files with its extension, i.e. "*.ext".

@pmiossec
Copy link
Member

pmiossec commented Nov 30, 2024

A missing separate feature is to provide a context menu item for git lfs track <file.ext>. I think of a TaskDialog asking whether to track that file or all files with its extension, i.e. "*.ext".

Also: #8355 & #8344 (lock/unlock LFS feature)

@gerhardol
Copy link
Member

What is missing for you?

To see if a file is handled by lfs or not. This can be done in other ways, maybe in the context menu.
In your example repo, it is just GitExtensionsLogoWide.png where the behaviour is changed, the other are still displayed as lfs.
(I expected a.long too from the diff tab too as attributes were changed.)

and my opinions is probably the important here, this is plugging something that users see as a problem (but it is not really that).
A way to display this can be added later.

@mstv
Copy link
Member Author

mstv commented Dec 1, 2024

I expected a.long too from the diff tab too as attributes were changed.

You need to run configure_lfs-text.sh, i.e git config diff.lfs-text.textconv cat.

@mstv
Copy link
Member Author

mstv commented Dec 1, 2024

To see if a file is handled by lfs or not. This can be done in other ways, maybe in the context menu.
In your example repo, it is just GitExtensionsLogoWide.png where the behaviour is changed, the other are still displayed as lfs.

This PR is about getting / saving selected files. Refer to the issue being fixed, please!
Whether a file is tracked by LFS or not and changing this state, is a separate feature.
I am changing the PR title.

@mstv mstv changed the title feat: Add LFS support feat: Support saving LFS files Dec 1, 2024
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

+1, quick review

@gerhardol
Copy link
Member

I am not thrilled to not see if the image file is handled or not, but approving.

@mstv mstv merged commit 6c60414 into gitextensions:master Dec 16, 2024
3 of 4 checks passed
@mstv mstv deleted the feature/lfs branch December 16, 2024 18:52
@mstv mstv added this to the v5.2 milestone Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving an old revision of an LFS file saves only the LFS pointer
3 participants