Skip to content

Conversation

@juntyr
Copy link
Contributor

@juntyr juntyr commented Sep 24, 2024

When a file is uploaded and has a name clash, a unique filename must be chosen. The current method appends the increment after the pathname, which interferes with the file extension, e.g. README.md becomes README.md_0. (Since the contents API uses the extension to determine how to interpret the file, this causes plain text files to be displayed as base64 encoded). The new variant puts the disambiguator in front to preserve the extension.

@github-actions
Copy link

Binder 👈 Launch a Binder on branch juntyr/litegitpuller/patch-1

@jtpio
Copy link
Member

jtpio commented Sep 24, 2024

Thanks @juntyr for catching this!

It looks like the current implementation would then create a file named 0_README.md?

Wondering if it should be named README_0.md instead, so the files are still next to each others when sorted by name?

@jtpio jtpio added the enhancement New feature or request label Sep 24, 2024
@juntyr
Copy link
Contributor Author

juntyr commented Sep 24, 2024

Thanks @juntyr for catching this!

It looks like the current implementation would then create a file named 0_README.md?

Wondering if it should be named README_0.md instead, so the files are still next to each others when sorted by name?

The file will be renamed once the upload of that file is completed anyways, so it will get its original name in the unique upload folder back. But if the name during this upload period matters, then your solution would be nicer.

@juntyr
Copy link
Contributor Author

juntyr commented Oct 2, 2024

I've been thinking a bit more about this. Extensions can meaningfully have one or multiple parts and I think it's really just easier to leave them be entirely, i.e. to not even try to determine where they start and end. Adding a prefix to the filename on upload may look weird for the split-second before the file is moved and renamed to the final location, but it works and fixes an existing bug.

@jtpio Would you be willing to merge this PR and release a patch release for this bug fix?

@jtpio
Copy link
Member

jtpio commented Oct 2, 2024

Adding a prefix to the filename on upload may look weird for the split-second before the file is moved and renamed to the final location, but it works and fixes an existing bug.

Ah right, if it's just temporary then it's probably fine.

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit f344716 into jupyterlite:main Oct 2, 2024
9 of 10 checks passed
@jtpio
Copy link
Member

jtpio commented Oct 2, 2024

@juntyr
Copy link
Contributor Author

juntyr commented Oct 2, 2024

Released: https://github.com/jupyterlite/litegitpuller/releases/tag/v0.2.1

Thank you so much <3

@juntyr juntyr deleted the patch-1 branch October 2, 2024 11:13
@brichet
Copy link
Collaborator

brichet commented Oct 3, 2024

Thanks @juntyr ans @jtpio for this update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants