Add LFS object data get using git lfs smudge. Blob images for #2981 e…

[sh0c]

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.

[steveward]

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!

[tidy-dev]

@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. :)

[sh0c]

I run locally yarn prettier to fix Lint failed build and commit changes.
Fix Prettier issues for #17686

[tidy-dev]

@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.

[sh0c]

Hello,
Yes, the smudge is used to get file content of LFS object and if it does not exists locally download it. LFS objects in every project lived in .git/lfs/objects folder and smudge command first check there for requested object. Here is example how to find object of LFS file:
From info of LFS object the given oid sha256 give us path to object. This is how to find specific file .git/lfs/objects/[oid sha256 first byte in hex]/[oid 256 second byte in hex]/[oid 256 in hex]

version https://git-lfs.github.com/spec/v1 oid sha256:66852057d5b9ba0c9a00aa259a0a8915185e33a3cae2704d9bc6f03bfae69d87 size 11102
For file with INFO given above location is .git/lfs/objects/66/85/66852057d5b9ba0c9a00aa259a0a8915185e33a3cae2704d9bc6f03bfae69d87

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.

[tidy-dev]

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 git lfs smudge will cause image B to be downloaded. Thus, without a button, the code as it sits will pull both images, but will cause Image B to be downloaded without communicating that a large file will be downloaded to the user.

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 Prompts settings and on the loading screen for the large image files.

[0xdevalias]

My understanding was that git lfs smudge will cause image B to be downloaded. Thus, without a button, the code as it sits will pull both images, but will cause Image B to be downloaded without communicating that a large file will be downloaded to the user.

@tidy-dev From what I have read, I believe that is the case.

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.

@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.

[sh0c]

Hello, sorry for long delay about answer, as I said before I am not TypeScript developer, I am learning your architecture to create something like this when you select LFS object:

Thank you for your patience!

[0xdevalias]

RE: #17686 (review)

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.

@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 SeamlessDiffSwitcher component renders the Diff component:

desktop/app/src/ui/diff/seamless-diff-switcher.tsx

Lines 157 to 165 in 6aa1e09

	/**
	* A component which attempts to minimize the need for unmounting
	* and remounting text diff components with the ultimate goal of
	* avoiding flickering when rapidly switching between files.
	*/
	export class SeamlessDiffSwitcher extends React.Component<
	ISeamlessDiffSwitcherProps,
	ISeamlessDiffSwitcherState
	> {

desktop/app/src/ui/diff/seamless-diff-switcher.tsx

Lines 351 to 378 in 6aa1e09

	return (
	<div className={className}>
	{diff !== null ? (
	<Diff
	repository={repository}
	imageDiffType={imageDiffType}
	file={file}
	diff={diff}
	fileContents={fileContents}
	readOnly={readOnly}
	hideWhitespaceInDiff={hideWhitespaceInDiff}
	showSideBySideDiff={showSideBySideDiff}
	askForConfirmationOnDiscardChanges={
	this.props.askForConfirmationOnDiscardChanges
	}
	onIncludeChanged={isLoadingDiff ? noop : onIncludeChanged}
	onDiscardChanges={isLoadingDiff ? noop : onDiscardChanges}
	onOpenBinaryFile={isLoadingDiff ? noop : onOpenBinaryFile}
	onOpenSubmodule={isLoadingDiff ? noop : onOpenSubmodule}
	onChangeImageDiffType={isLoadingDiff ? noop : onChangeImageDiffType}
	onHideWhitespaceInDiffChanged={
	isLoadingDiff ? noop : onHideWhitespaceInDiffChanged
	}
	/>
	) : null}
	{loadingIndicator}
	</div>
	)

Diff renders differently based on the props.diff.kind:

desktop/app/src/ui/diff/index.tsx

Lines 121 to 143 in 6aa1e09

	public render() {
	const diff = this.props.diff
	switch (diff.kind) {
	case DiffType.Text:
	return this.renderText(diff)
	case DiffType.Binary:
	return this.renderBinaryFile()
	case DiffType.Submodule:
	return this.renderSubmoduleDiff(diff)
	case DiffType.Image:
	return this.renderImage(diff)
	case DiffType.LargeText: {
	return this.state.forceShowLargeDiff
	? this.renderLargeText(diff)
	: this.renderLargeTextDiff()
	}
	case DiffType.Unrenderable:
	return this.renderUnrenderableDiff()
	default:
	return assertNever(diff, `Unsupported diff type: ${diff}`)
	}
	}

So I believe @tidy-dev's suggestion above may have been to implement a new DiffType for LFS Images:

desktop/app/src/models/diff/diff-data.ts

Lines 10 to 23 in 6aa1e09

	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,
	/** Diff that will not be rendered */
	Unrenderable,
	}

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

	case DiffType.LargeText: {
	return this.state.forceShowLargeDiff
	? this.renderLargeText(diff)
	: this.renderLargeTextDiff()
	}

desktop/app/src/ui/diff/index.tsx

Lines 175 to 190 in 6aa1e09

	private renderLargeTextDiff() {
	return (
	<div className="panel empty large-diff">
	<img src={NoDiffImage} className="blankslate-image" alt="" />
	<p>
	The diff is too large to be displayed by default.
	<br />
	You can try to show it anyway, but performance may be negatively
	impacted.
	</p>
	<Button onClick={this.showLargeDiff}>
	{__DARWIN__ ? 'Show Diff' : 'Show diff'}
	</Button>
	</div>
	)
	}

desktop/app/src/ui/diff/index.tsx

Lines 311 to 313 in 6aa1e09

	private showLargeDiff = () => {
	this.setState({ forceShowLargeDiff: true })
	}

I think since your code is currently written to work with DiffType.image, then after you show the large file message + button, then you might be able to just call through to the existing 'render image' functionality:

desktop/app/src/ui/diff/index.tsx

Lines 145 to 155 in 6aa1e09

	private renderImage(imageDiff: IImageDiff) {
	if (imageDiff.current && imageDiff.previous) {
	return (
	<ModifiedImageDiff
	onChangeDiffType={this.props.onChangeImageDiffType}
	diffType={this.props.imageDiffType}
	current={imageDiff.current}
	previous={imageDiff.previous}
	/>
	)
	}

---

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.

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 is

I'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 PullRequestBadge rendered within BranchDropdown:

desktop/app/src/ui/toolbar/branch-dropdown.tsx

Lines 369 to 386 in 6aa1e09

	private renderPullRequestInfo() {
	const pr = this.props.currentPullRequest
	if (pr === null) {
	return null
	}
	return (
	<PullRequestBadge
	number={pr.pullRequestNumber}
	dispatcher={this.props.dispatcher}
	repository={pr.base.gitHubRepository}
	onBadgeRef={this.onBadgeRef}
	onBadgeClick={this.onBadgeClick}
	/>
	)
	}
	}

Which renders CIStatus:

desktop/app/src/ui/branches/pull-request-badge.tsx

Lines 80 to 93 in 6aa1e09

	public render() {
	const ref = getPullRequestCommitRef(this.props.number)
	return (
	// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions
	<div className="pr-badge" onClick={this.onBadgeClick} ref={this.onRef}>
	<span className="number">#{this.props.number}</span>
	<CIStatus
	commitRef={ref}
	dispatcher={this.props.dispatcher}
	repository={this.props.repository}
	onCheckChange={this.onCheckChange}
	/>
	</div>
	)

CIStatus is defined here:

desktop/app/src/ui/branches/ci-status.tsx

Lines 35 to 39 in 6aa1e09

	/** The little CI status indicator. */
	export class CIStatus extends React.PureComponent<
	ICIStatusProps,
	ICIStatusState
	> {

And seems to have some stuff related to 'pending' state (see getSymbolForCheck/ getClassNameForCheck / etc), which if I remember correctly, may show the 'loader' that @tidy-dev was referring to:

desktop/app/src/ui/branches/ci-status.tsx

Lines 150 to 192 in 6aa1e09

	export function getClassNameForCheck(
	check: ICombinedRefCheck | IRefCheck | IAPIWorkflowJobStep
	): string {
	switch (check.conclusion) {
	case 'timed_out':
	return 'timed-out'
	case 'action_required':
	return 'action-required'
	case 'failure':
	case 'neutral':
	case 'success':
	case 'cancelled':
	case 'skipped':
	case 'stale':
	return check.conclusion
	}
	// Pending
	return 'pending'
	}
	export function getSymbolForLogStep(
	logStep: IAPIWorkflowJobStep
	): OcticonSymbolType {
	switch (logStep.conclusion) {
	case 'success':
	return OcticonSymbol.checkCircleFill
	case 'failure':
	return OcticonSymbol.xCircleFill
	}
	return getSymbolForCheck(logStep)
	}
	export function getClassNameForLogStep(logStep: IAPIWorkflowJobStep): string {
	switch (logStep.conclusion) {
	case 'failure':
	return logStep.conclusion
	}
	// Pending
	return ''
	}

getSymbolForCheck seems to define the symbol for pending as OcticonSymbol.dotFill.

These helpers then seem to be used with the Octicon component:

desktop/app/src/ui/branches/ci-status.tsx

Lines 110 to 120 in 6aa1e09

	return (
	<Octicon
	className={classNames(
	'ci-status',
	`ci-status-${getClassNameForCheck(check)}`,
	this.props.className
	)}
	symbol={getSymbolForCheck(check)}
	title={getRefCheckSummary(check)}
	/>
	)

OctIcon is defined here:

desktop/app/src/ui/octicons/octicon.tsx

Lines 30 to 39 in 6aa1e09

	/**
	* A React component for displaying octicon symbols in SVG format.
	*
	* Note that the aspect ratios of the octicons will always be preserved
	* which is why the width and height properties specify the maximum and
	* not the minimum size.
	*
	* Usage: `<Octicon symbol={OcticonSymbol.mark_github} />`
	*/
	export class Octicon extends React.Component<IOcticonProps, {}> {

My guess is that that is using the GitHub primer OctIcon symbols:

• https://github.com/primer/octicons
• https://primer.style/foundations/icons/
  • https://primer.style/foundations/icons/dot-fill-16

Edit 2: Ok, I think I found the relevant code this time, in BranchDropdown there is some code related to checkoutProgress that uses icon = syncClockwise and iconClassName = 'spin', which are then passed into ToolbarDropdown:

desktop/app/src/ui/toolbar/branch-dropdown.tsx

Line 4 in 6aa1e09

import { syncClockwise } from '../octicons'

desktop/app/src/ui/toolbar/branch-dropdown.tsx

Lines 159 to 172 in 6aa1e09

	if (checkoutProgress) {
	title = checkoutProgress.target
	description = checkoutProgress.description
	if (checkoutProgress.value > 0) {
	const friendlyProgress = Math.round(checkoutProgress.value * 100)
	description = `${description} (${friendlyProgress}%)`
	}
	tooltip = `Checking out ${checkoutProgress.target}`
	progressValue = checkoutProgress.value
	icon = syncClockwise
	iconClassName = 'spin'
	canOpen = false

ToolbarDropdown is defined here:

desktop/app/src/ui/toolbar/dropdown.tsx

Lines 223 to 229 in 6aa1e09

	/**
	* A toolbar dropdown button
	*/
	export class ToolbarDropdown extends React.Component<
	IToolbarDropdownProps,
	IToolbarDropdownState
	> {

And seemingly drills the icon / iconClassName props down to the ToolbarButton component:

desktop/app/src/ui/toolbar/dropdown.tsx

Lines 445 to 488 in 6aa1e09

	return (
	<div
	className={className}
	onKeyDown={this.props.onKeyDown}
	role={this.props.role}
	onDragOver={this.props.onDragOver}
	ref={this.rootDiv}
	>
	{this.renderDropdownContents()}
	<ToolbarButton
	className={this.props.buttonClassName}
	ref={this.innerButton}
	icon={this.props.icon}
	title={this.props.title}
	description={this.props.description}
	tooltip={this.props.tooltip}
	onClick={this.onMainButtonClick}
	onContextMenu={this.onContextMenu}
	onMouseEnter={this.props.onMouseEnter}
	style={this.props.style}
	iconClassName={this.props.iconClassName}
	disabled={this.props.disabled}
	tabIndex={this.props.tabIndex}
	progressValue={this.props.progressValue}
	role={this.props.buttonRole}
	onlyShowTooltipWhenOverflowed={
	this.props.onlyShowTooltipWhenOverflowed
	}
	isOverflowed={this.props.isOverflowed}
	ariaExpanded={
	this.props.dropdownStyle === ToolbarDropdownStyle.MultiOption
	? undefined
	: this.isOpen
	}
	ariaHaspopup={this.props.buttonAriaHaspopup}
	>
	{this.props.children}
	{this.props.dropdownStyle !== ToolbarDropdownStyle.MultiOption &&
	this.renderDropdownArrow()}
	</ToolbarButton>
	{this.props.dropdownStyle === ToolbarDropdownStyle.MultiOption &&
	this.renderDropdownArrow()}
	</div>
	)

ToolbarButton is defined here:

desktop/app/src/ui/toolbar/button.tsx

Lines 31 to 35 in 6aa1e09

	/** An optional symbol to be displayed next to the button text */
	readonly icon?: OcticonSymbolType
	/** The class name for the icon element. */
	readonly iconClassName?: string

desktop/app/src/ui/toolbar/button.tsx

Lines 144 to 147 in 6aa1e09

	/**
	* A general purpose toolbar button
	*/
	export class ToolbarButton extends React.Component<IToolbarButtonProps, {}> {

And seemingly drills the icon / iconClassName props down to the Octicon component:

desktop/app/src/ui/toolbar/button.tsx

Lines 180 to 187 in 6aa1e09

	public render() {
	const { tooltip } = this.props
	const icon = this.props.icon ? (
	<Octicon
	symbol={this.props.icon}
	className={classNames('icon', this.props.iconClassName)}
	/>
	) : null

OctIcon is defined here:

desktop/app/src/ui/octicons/octicon.tsx

Lines 30 to 39 in 6aa1e09

	/**
	* A React component for displaying octicon symbols in SVG format.
	*
	* Note that the aspect ratios of the octicons will always be preserved
	* which is why the width and height properties specify the maximum and
	* not the minimum size.
	*
	* Usage: `<Octicon symbol={OcticonSymbol.mark_github} />`
	*/
	export class Octicon extends React.Component<IOcticonProps, {}> {

My guess is that that is using the GitHub Primer OctIcon symbols:

• https://github.com/primer/octicons
• https://primer.style/foundations/icons/
  • https://primer.style/foundations/icons/sync-24
• https://primer.style/ui-patterns/loading#indeterminate-loading-indicators

[0xdevalias]

I am learning your architecture to create something like this when you select LFS object:

@sh0c While I'm not deeply familiar with it myself, you can see the code that renders that dialog here:

• https://github.com/search?q=repo%3Adesktop%2Fdesktop+%22confirm+discard+changes%22&type=code

desktop/app/src/ui/discard-changes/discard-selection-dialog.tsx

Lines 47 to 51 in 6aa1e09

	/** A component to confirm and then discard changes from a selection. */
	export class DiscardSelection extends React.Component<
	IDiscardSelectionProps,
	IDiscardSelectionState
	> {

desktop/app/src/ui/discard-changes/discard-changes-dialog.tsx

Lines 44 to 48 in 6aa1e09

	/** A component to confirm and then discard changes. */
	export class DiscardChanges extends React.Component<
	IDiscardChangesProps,
	IDiscardChangesState
	> {

Those examples both seem to make use of this Dialog component:

desktop/app/src/ui/dialog/dialog.tsx

Lines 217 to 225 in 6aa1e09

	/**
	* A general purpose, versatile, dialog component which utilizes the new
	* <dialog> element. See https://demo.agektmr.com/dialog/
	*
	* A dialog is opened as a modal that prevents keyboard or pointer access to
	* underlying elements. It's not possible to use the tab key to move focus
	* out of the dialog without first dismissing it.
	*/
	export class Dialog extends React.Component<DialogProps, IDialogState> {

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

	<Checkbox
	label="Discarding changes permanently"
	value={
	this.state.confirmDiscardChangesPermanently
	? CheckboxValue.On
	: CheckboxValue.Off
	}
	onChange={this.onConfirmDiscardChangesPermanentlyChanged}
	/>

desktop/app/src/ui/preferences/prompts.tsx

Lines 71 to 78 in 6aa1e09

	private onConfirmDiscardChangesPermanentlyChanged = (
	event: React.FormEvent<HTMLInputElement>
	) => {
	const value = event.currentTarget.checked
	this.setState({ confirmDiscardChangesPermanently: value })
	this.props.onConfirmDiscardChangesPermanentlyChanged(value)
	}

[tidy-dev]

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.

[sh0c]

Hello, Sorry for long delay but me and my whole family were sick for a several weeks.
Thank you for suggestions @0xdevalias and for clear request @tidy-dev .

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
I meet some challenges to draw loading icon on center of diff window. I used Loading class from app\src\ui\lib\loading.tsx

[sh0c]

Hello again, I was forced in my company to upload new feature. Photoshop (PSD) preview support.

[McLeanKing11]

0744310659