Community
Participate
Working Groups
Created attachment 288863 [details] screenshot of AWS console illustrating bad paths I haven't tried this w/ the latest release of jgit yet, it's possible it's been addressed. On Windows, the directory separator character is backslash. That is leaking into the bucket URLs for *.pack and *.idx files. S3 treats backslash as a normal character, so instead of these files going into the objects/pack/*.(pack|idx) paths, they end up in the objects folder with filenames that begin with `pack\`. Subsequent attempts to fetch/clone from the repo then can't find the pack files and the objects they contain, and the fetch/clone fails. Generally I'm able to reproduce this by: * Have a repository stored in S3 * Make some local commits. * Run `git gc` locally to create new *.packs (this may not be required) * Run `jgit push` * Examine the S3 storage, note new pack files with the broken paths * Try to fetch/clone from a separate enlistment, you'll get failed object fetches because the pack files weren't found/indexed.
I just tried again with `jgit version 6.3.0.202209071007-r`, and confirmed it's still reproducing the problem. And I confirmed the steps that produce it are: * Clone an S3 repository * Make a new local commit * `git gc` * `jgit push` * Examine the S3 bucket's `object` folder, and note pack/idx files with `pack\` prefixes
Yes. TransportAmazonS3.DatabaseS3 assumes /-delimited paths, but is called in WalkPushConnection on several occasions with some java.io.File.getPath(), which uses \ if on Windows.
Would you recommend the paths get sanitized at `TransportAmazonS3.DatabaseS3` or at `WalkPushConnection`? Would a `path = path.replace('\\', '/')` in DatabaseS3 suffice?
(In reply to Jeremy Braun from comment #3) > Would you recommend the paths get sanitized at > `TransportAmazonS3.DatabaseS3` or at `WalkPushConnection`? Would a `path = > path.replace('\\', '/')` in DatabaseS3 suffice? Good question. DatabaseS3 is a WalkRemoteObjectDatabase, which uses String, not Path or some such in the its interface. It assumes /. This could be made a bit more explicit in the javadoc. There are two more implementations, one for Http and one for Sftp, which also work with /. So I would think this should be fixed in WalkPushConnection. WalkPushConnection already uses / in other places. I would suggest doing the translation only if necessary: if (File.separatorChar != '/') { path = path.replace(File.separatorChar, '/'); } This is what File.toURI() does, too.
I think I've got a preliminary path. I ran into another problem while looking at this, I wasn't getting any pack files when trying to list the pack directory, because the `name` parameter to the listParser in AmazonS3.java was zero-length, and the element tag was in the qName parameter. I've "fixed" this too, but don't really know why that was occurring, and whether the fix (look at the first non-zero length argument) is really the right thing. I'm off to figure out how to post a pull request or patch for review.
Changes for review: https://git.eclipse.org/r/c/jgit/jgit/+/197493 https://git.eclipse.org/r/c/jgit/jgit/+/197494
