On 7/27/2022 6:09 PM, Josh Steadmon wrote: > On 2022.07.25 20:34, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> >> Before implementing a way to fetch bundles into a repository, create the >> basic logic. Assume that the URI is actually a file path. Future logic >> will make this more careful to other protocols. >> >> For now, we also only succeed if the content at the URI is a bundle >> file, not a bundle list. Bundle lists will be implemented in a future >> change. >> >> Note that the discovery of a temporary filename is slightly racy because >> the odb_mkstemp() relies on the temporary file not existing. With the >> current implementation being limited to file copies, we could replace >> the copy_file() with copy_fd(). The tricky part comes in future changes >> that send the filename to 'git remote-https' and its 'get' capability. > >> At that point, we need the file descriptor closed _and_ the file >> unlinked. > > Ahh, it looks like this was the point I missed in my previous review. > IIUC, we need the file unlinked because http_get_file() will eventually > call finalize_object_file() to move a tempfile to the final object name, > and that will fail if we have an empty file already in place. Yes, and I also was not sure what would happen if the empty file existed. I tested it and thought allowing overwriting an existing file would be a bigger problem than this choice of a filename. We also discussed options about how it would be nice to have a predictable filename so we could resume downloads _across Git process failures_ instead of just a network failure within a single Git process. This is something to explore when creating that functionality. Thanks, -Stolee
