Skip to content

Rename/move binary and LFS files in web UI #34350

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

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

bytedream
Copy link
Contributor

@bytedream bytedream commented May 2, 2025

Adds the ability to rename/move binary files like binary blobs or images and files that are too large in the web ui.
This was purposed in #24722, along with the ability edit images via an upload of a new image, which I didn't implement here (could be done in a separate PR).

Binary file content:
binary

File too large:
toolarge

GitHub does the same (I've copied the text from there):
gh

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 2, 2025
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels May 2, 2025
@lunny lunny added this to the 1.25.0 milestone May 3, 2025
@lunny
Copy link
Member

lunny commented May 3, 2025

Could you also have a screenshot for editing text files stored in LFS and binary files stored in LFS?

@bytedream
Copy link
Contributor Author

bytedream commented May 3, 2025

I'm working on adding support for LFS (I thought it'd behave the same as non LFS files), and noticed that atm LFS files can be edited directly on the web ui if the file path is manually set in the URL.

(this actually is a 20MB binary file stored in LFS)
Screenshot_20250503_035508

This behavior/bug will be removed in this PR, but as it's scheduled for version 1.25, it will be present in 1.24. Should I do a separate PR that fixes this for 1.24 backporting (@lunny)?

@lunny
Copy link
Member

lunny commented May 3, 2025

I'm working on adding support for LFS (I thought it'd behave the same as non LFS files), and noticed that atm LFS files can be edited directly on the web ui if the file path is manually set in the URL.

(this actually is a 20MB binary file stored in LFS) Screenshot_20250503_035508

This behavior/bug will be removed in this PR, but as it's scheduled for version 1.25, it will be present in 1.24. Should I do a separate PR that fixes this for 1.24 backporting (@lunny)?

I think the bug fix cannot be backport easily to v1.24 if the PR is complex. But maybe we can try. Please send another PR to fix the bug.

@bytedream bytedream changed the title Rename/move binary files in Web UI WIP: Rename/move binary files in Web UI May 5, 2025
@bytedream bytedream changed the title WIP: Rename/move binary files in Web UI Rename/move binary and LFS files in web UI May 8, 2025
@bytedream
Copy link
Contributor Author

LFS support, incl. moving files from git to lfs and from lfs to git
lfs

Edit button is also shown in pull request changes
image


Similar to file edit, the commit button will only be clickable if the path really changed.

@lunny
Copy link
Member

lunny commented May 8, 2025

It's better to display a lfs label when new/upload/update a lfs file.

@lunny
Copy link
Member

lunny commented May 8, 2025

Actually, I think the text lfs file should be editable via web UI. This can be implemented in this PR or a new PR.

@bytedream
Copy link
Contributor Author

bytedream commented May 8, 2025

I though the same, I think a new PR would be better to separate the feature additions

@wxiaoguang
Copy link
Contributor

Actually, I think the text lfs file should be editable via web UI. This can be implemented in this PR or a new PR.

Are you serious .... when a file is in LFS, it is likely quite large, would you like to edit a 10MB text file on the web UI?

@lunny
Copy link
Member

lunny commented May 9, 2025

Actually, I think the text lfs file should be editable via web UI. This can be implemented in this PR or a new PR.

Are you serious .... when a file is in LFS, it is likely quite large, would you like to edit a 10MB text file on the web UI?

There are two conditions for a file to be editable via the web interface: it must be a text file, and its size must be small. While files stored in Git LFS are typically large, we also support editing smaller text files stored in LFS since this PR have supported rename/remove such files.

@wxiaoguang
Copy link
Contributor

While files stored in Git LFS are typically large, ... we also support editing smaller text files stored in LFS

Well, that's the question: since the files in LFS are typically large, why should we spend more time and code on supporting editing "small text files in LFS"? Why such small text files were stored in LFS, isn't it an abuse ......

@lunny
Copy link
Member

lunny commented May 9, 2025

While files stored in Git LFS are typically large, ... we also support editing smaller text files stored in LFS

Well, that's the question: since the files in LFS are typically large, why should we spend more time and code on supporting editing "small text files in LFS"? Why such small text files were stored in LFS, isn't it an abuse ......

There is an inconsistency here. If this PR is merged, it would allow a text LFS file to be created or uploaded via UI, but not edited — which is confusing. I previously mentioned that editing support could be handled in a separate PR, and we shouldn’t implement it as part of this one.

assert.NoError(t, err)

filesAfterRename, _ := temp.LsFiles(ctx, "README.txt", "README.md")
assert.Equal(t, []string{"README.txt", ""}, filesAfterRename)
Copy link
Member

@lunny lunny May 11, 2025

Choose a reason for hiding this comment

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

Could you have more tests for old file is/isnot lfs file and the new file is/isnot lfs file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants