This repository was archived by the owner on Sep 3, 2025. It is now read-only.
File creation bugfix for init + misc cleanup
#26
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
approved these changes
Feb 16, 2023
Collaborator
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
Approving, but have some questions about the side effect in ReadFileLines(). It's worth thinking about alternatives, but it's not a blocker to moving forward.
vdye
force-pushed
the
vdye/init-bugfix
branch
from
2538ffb to
8f6168c
Compare
derrickstolee
approved these changes
Feb 17, 2023
| file, err := os.Open(filename) | ||
| if err != nil { |
Comment on lines
+68
to
+75
| pathErr, ok := err.(*os.PathError) | ||
| if ok && pathErr.Err == syscall.ENOENT { | ||
| // If the file doesn't exist, return empty result rather than an | ||
| // error | ||
| return []string{}, nil | ||
| } else { | ||
| return nil, err | ||
| } |
Collaborator
There was a problem hiding this comment.
Looks good.
nit: the commit message no longer matches the change.
Remove the 'Wishlist.md' file. The first two items in the list (custom root directory for repository storage and tracing) have been added to the repository issue tracker ([1] & [2], respectively). The last (HTTPS web server support) was added in 5377d04 (web-server: capture and use web server options, 2023-01-26) and its immediately preceding commits [3]. [1] #25 [2] #22 [3] #10 Signed-off-by: Victoria Dye <[email protected]>
The 'doc.go' file seems to be leftover from an earlier version of the bundle server prototype. It currently contains no code and is not required for a successful build, so remove it. Signed-off-by: Victoria Dye <[email protected]>
If the file argument to 'ReadFileLines()' does not exist on disk, return an empty string list (corresponding to an empty file) with no error rather than pass the error on to the caller. This removes a pre-existing side-effect behavior of 'ReadFileLines()', where the 'os.O_CREATE' flag would create the file if it was missing when read. It would not create leading directories, though, leading to unhandled error cases (e.g. in 'git-bundle-server init' without '~/git-bundle-server'). By instead returning an empty result for any missing file, these errors are avoided. Signed-off-by: Victoria Dye <[email protected]>
Print the error received by 'CreateRepository()'/'RemoveRoute()' when attempting to read the repository routes file. This will help users identify the cause of the failure (e.g., permissions issue) so it can be remedied. Signed-off-by: Victoria Dye <[email protected]>
vdye
force-pushed
the
vdye/init-bugfix
branch
from
8f6168c to
9727ad6
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #13.
The first two commits remove
Wishlist.md(replaced with issues linked in the commit description) anddoc.go(I think it's just unused?). The third commit fixes #13 bycreating leading parent directoriesreturning an empty result if the file doesn't exist, and the fourth improves error reporting when trying to read the file.Manual testing confirmed that the missing file doesn't trigger an error (and that it's still successfully created by
CreateRepository()). I was also able to trigger an intentional failure ofReadFileLines()(by creating~/git-bundle-serverwithsudo mkdir -m 700 ~/git-bundle-server) and confirmed that the permission denied error was reported: