Skip to content
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

fix: correct order of lfs pull and repo config for git proxy #486

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

ableuler
Copy link
Contributor

This also needed a bit of cleanup and refactoring of the autosave branch handling in the init container.

This needed a bit of cleanup and refactoring of the autosave branch handling in the init container.
@ableuler ableuler requested a review from a team as a code owner December 13, 2020 23:21
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

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

I tested this with a private project that has LFS files and things work. With autofetch on I have the files there right away without running the extra command to download files.

I just have one question about the code.

The thing about conflicting autosave branches with 7 character commit shas that match is not a problem IMO. Isn't the probability very low of getting the first 7 characters of the commit sha that are the same for two or even more branches?

@ableuler
Copy link
Contributor Author

ableuler commented Jan 6, 2021

The thing about conflicting autosave branches with 7 character commit shas that match is not a problem IMO. Isn't the probability very low of getting the first 7 characters of the commit sha that are the same for two or even more branches?

The naming of the autosave branches is determined here. So the commit SHA we're looking at the commented location is from the commit previous to the autosave commit. So basically the code here silently assumes that for one user/branch/commit-sha combo, there can be no more than one autosave branch. We kind of enforce this by always dropping the user into an existing autosave environment (and deleting the autosave branch while doing this) and by allowing only one running session per user/branch/commit, so one would have to almost know how to trick the system to end up with two autosave branches for the same base commit. Therefore I agree, my comment is probably not super relevant and should definitely not be phrased as a TODO. I'll change that.

@ableuler
Copy link
Contributor Author

ableuler commented Jan 6, 2021

I tested this with a private project that has LFS files and things work. With autofetch on I have the files there right away without running the extra command to download files.

If you haven't done so already, I'd be very grateful if you could also test the autosave recovery. As far as I can tell it still works in the same way, but someone else touching it would be great.

@olevski
Copy link
Member

olevski commented Jan 7, 2021

@ableuler I can confirm that uncommitted work is successfully recovered.

@olevski olevski self-requested a review January 7, 2021 09:06
olevski
olevski previously approved these changes Jan 7, 2021
@ableuler
Copy link
Contributor Author

ableuler commented Jan 7, 2021

@olevski I improved the language of the comments a bit - can you please re-approve? Thx!

@olevski olevski self-requested a review January 8, 2021 10:52
@ableuler ableuler merged commit 243af69 into master Jan 8, 2021
@ableuler ableuler deleted the 000-fix-lfs-auto-fetch branch January 8, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants