-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LFS object data get using git lfs smudge. Blob images for #2981 e… #17686
base: development
Are you sure you want to change the base?
Conversation
Thanks for the PR @sh0c, and apologies for not getting back to you sooner on this. I'll add this to our project board for review and we'll get back to you with feedback soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sh0c Thank you for your work in adding images for LFS!
I am not intimately familiar with LFS, but did a little looking and seems that using git lfs smudge
for this stabilized with git 2.34.0 and we are on 2.39.0. Seems like a logical approach.
I pulled down your code and tested on an LFS repo tracking png images. It works as expected for added, deleted, and removed images in the history and changes view as well as with with multi-commit diffing and in the pull request previewing.
I have left a few comments that are largely stylistic preferences that we would really appreciate you updated the code to meet our documented engineering values and style guide.
Great work. I will see if I can get another team member with more LFS background to do a look over after these comments are addressed.
Update to add: Looks like the linter is upset, would appreciate it if you could run yarn lint:fix
or yarn lint
to see what it is upset about. :)
I run locally yarn prettier to fix Lint failed build and commit changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sh0c Thank you for all your work thus far.
I discussed this with the team and we are concerned that smudge will always cause downloading a huge file and we are not communicating that. A user could end up pulling large amounts of data unintentionally and not know why the diff is sitting with a simple spinning loader.
Could you update the logic to instead of automatically pulling the image, you mark the diff as an LFS image and update the diff display to show a button and message about pulling a large file. On pushing that button, we retrieve the image diff with smudge
as you are doing here and display a more informative loader (a message/image like we have for other long load times - Pull request dropdown has an example) while the smudge
operation is taking place.
Hello,
And why I give you this example? Some of objects can be already locally and no need of DOWNLOAD button for them. I will search for API in repository that can search in .git/lfs folder. |
I may be misunderstanding but lets say we have a repository with 2 images. Image A is in the locally stored objects and image B image is not and needs to be downloaded. My understanding was that The problem comes in is that we do not know ahead of time which images are locally stored and which images are not. Thus, we cannot provide a download button for only the ones that need to be downloaded. If you have further understanding of how LFS works in that we can only offer the download button for the ones not in the local object store, that would be ideal. Another thought: I think there are use cases where a user may not mind that it always automatically downloads the large images (they don't have data constraints on their network and simply prefer for the download to start as soon as the diff is requested). Thus, when we offer the button to download the large file we could also have check box beneath the button along the lines of "Always download LFS images. Warning: This will lead to large network data consumption and may have long load times." Additionally, we would want a way to disable this function in the |
@tidy-dev From what I have read, I believe that is the case.
@tidy-dev I believe that is what @sh0c was referring to in #17686 (comment) when they describe the internals of this, and that they are going to look for an API/similar to access it more directly; thus allowing the download button to only be shown when needed. I wonder if we could land the changes in this PR 'as is' (eg. always showing the download button), and then handle the potential UX improvements of detecting if it is already downloaded/etc in a followup PR. That way the core functionality can land sooner, and if a better solution is found for detecting it, then the core functionality can be enhanced with that. |
RE: #17686 (review)
@sh0c I did a deep dive into the codebase on another issue (Ref), and found some of the parts that would probably be useful for implementing this. Skipping some of the component hierarchy, essentially the desktop/app/src/ui/diff/seamless-diff-switcher.tsx Lines 157 to 165 in 6aa1e09
desktop/app/src/ui/diff/seamless-diff-switcher.tsx Lines 351 to 378 in 6aa1e09
desktop/app/src/ui/diff/index.tsx Lines 121 to 143 in 6aa1e09
So I believe @tidy-dev's suggestion above may have been to implement a new desktop/app/src/models/diff/diff-data.ts Lines 10 to 23 in 6aa1e09
At first, it will show the message about pulling a large file and the button. You might be able to use this existing render method as an example: desktop/app/src/ui/diff/index.tsx Lines 133 to 137 in 6aa1e09
desktop/app/src/ui/diff/index.tsx Lines 175 to 190 in 6aa1e09
desktop/app/src/ui/diff/index.tsx Lines 311 to 313 in 6aa1e09
I think since your code is currently written to work with desktop/app/src/ui/diff/index.tsx Lines 145 to 155 in 6aa1e09
I'm not currently sure where the 'pull request dropdown' is to link to the example Edit: After following this code path, I'm not sure if this is actually correct.. hiding in an expandable in case i'm wrong, but leaving here for context in case it's useful This might not be correct, kept here in case it isI'm not currently sure where the 'pull request dropdown' is to link to the example though, but if I was to guess, I think @tidy-dev may mean the desktop/app/src/ui/toolbar/branch-dropdown.tsx Lines 369 to 386 in 6aa1e09
Which renders desktop/app/src/ui/branches/pull-request-badge.tsx Lines 80 to 93 in 6aa1e09
desktop/app/src/ui/branches/ci-status.tsx Lines 35 to 39 in 6aa1e09
And seems to have some stuff related to 'pending' state (see desktop/app/src/ui/branches/ci-status.tsx Lines 150 to 192 in 6aa1e09
These helpers then seem to be used with the desktop/app/src/ui/branches/ci-status.tsx Lines 110 to 120 in 6aa1e09
desktop/app/src/ui/octicons/octicon.tsx Lines 30 to 39 in 6aa1e09
My guess is that that is using the GitHub primer OctIcon symbols: Edit 2: Ok, I think I found the relevant code this time, in
desktop/app/src/ui/toolbar/branch-dropdown.tsx Lines 159 to 172 in 6aa1e09
desktop/app/src/ui/toolbar/dropdown.tsx Lines 223 to 229 in 6aa1e09
And seemingly drills the desktop/app/src/ui/toolbar/dropdown.tsx Lines 445 to 488 in 6aa1e09
desktop/app/src/ui/toolbar/button.tsx Lines 31 to 35 in 6aa1e09
desktop/app/src/ui/toolbar/button.tsx Lines 144 to 147 in 6aa1e09
And seemingly drills the desktop/app/src/ui/toolbar/button.tsx Lines 180 to 187 in 6aa1e09
desktop/app/src/ui/octicons/octicon.tsx Lines 30 to 39 in 6aa1e09
My guess is that that is using the GitHub Primer OctIcon symbols: |
@sh0c While I'm not deeply familiar with it myself, you can see the code that renders that dialog here: desktop/app/src/ui/discard-changes/discard-selection-dialog.tsx Lines 47 to 51 in 6aa1e09
desktop/app/src/ui/discard-changes/discard-changes-dialog.tsx Lines 44 to 48 in 6aa1e09
Those examples both seem to make use of this desktop/app/src/ui/dialog/dialog.tsx Lines 217 to 225 in 6aa1e09
I believe for the confirmation dialog, it's config also needs to be added somewhere like here (and maybe some other places): desktop/app/src/ui/preferences/prompts.tsx Lines 205 to 213 in 6aa1e09
desktop/app/src/ui/preferences/prompts.tsx Lines 71 to 78 in 6aa1e09
|
Just to chime in a little, I was imagining that either a new diff type or that the image diff type has an additional property. Then, likely show the same LFS diff we always do with the LFS lookup details, but below it have a message and a button. Not a popup dialog. |
…ired for pull request desktop#17686
Hello, Sorry for long delay but me and my whole family were sick for a several weeks. I added new diff type in app\src\models\diff\diff-data.ts: export enum DiffType {
/** Changes to a text file, which may be partially selected for commit */
Text,
/** Changes to a file with a known extension, which can be viewed in the app */
Image,
/** Changes to an unknown file format, which Git is unable to present in a human-friendly format */
Binary,
/** Change to a repository which is included as a submodule of this repository */
Submodule,
/** Diff is large enough to degrade ux if rendered */
LargeText,
+ /** Changes to a LFS file with a known extension, witch can be viewed in the app */
+ LFSImage,
/** Diff that will not be rendered */
Unrenderable,
}
+export interface ILFSImageDiff {
+ readonly kind: DiffType.LFSImage
+ /**
+ * The previous image promise, if the file was modified or deleted
+ *
+ * Will be undefined for an added image
+ */
+ readonly previous?: () => Promise<Image>
+ /**
+ * The current image promise, if the file was added or modified
+ *
+ * Will be undefined for a deleted image
+ */
+ readonly current?: () => Promise<Image>
+} After that I made changes in app\src\ui\diff\index.tsx for message showing before request of data using git lfs smudge |
Hello again, I was forced in my company to upload new feature. Photoshop (PSD) preview support. |
…ired for pull request desktop#17686
…ired for pull request desktop#17686
…ired for pull request desktop#17686
…ired for pull request desktop#17686
/** The standard error output from git. */ | ||
readonly stderr: Buffer | ||
/** The exit code of the git process. */ | ||
readonly exitCode: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0744310659
Co-authored-by: tidy-dev <[email protected]>
Co-authored-by: tidy-dev <[email protected]>
…ired for pull request desktop#17686
Closes #2981
Description
Added new function that blob image from LFS object metadata.
For example from this metadata:
version https://git-lfs.github.com/spec/v1 oid sha256:e39fcb1f948fde6262867a2a2487539b7bd554412d51325e6f8d48dab0b7dbbf size 78548
and pass it to stdin of git lfs smudge we can get blob of LFS file.
Added check for lfs object in convertDiff function in app/src/lib/git/diff.ts.
Added new function that get LFS metadata from DiffHunk for current and previous version of file.
Result
Now user can see LFS images diff not LFS metadata object diff.