Wesley Smith <wsmith@xxxxxxxxxxxxxxxxx> writes: > 1) This bug is triggered because "git fetch" is causing a pack > file to be written when that same pack file already exists. It > seems like this is harmless and shouldn't cause a problem. Is > that correct? The final name of the packfile is derived from the entire contents of the packfile; it should be harmless when we attempt to rename a new file, which has exactly the same contents as an existing file, to the existing file and see a failure out of that attempt. > 2) It seems that finalize_object_file is not accounting for the > fact that "link" will return EEXIST if the destination file > already exists but is not writeable, whereas "rename" will return > EACCESS in this case. Is that correct? If so, should > finalize_object_file be fixed to account for this? Perhaps it > should check if the newfile exists before calling rename. Or, > should the Windows mingw_rename function be modified to return > EEXIST in this case, even though that's not the standard errno for > that situation? The codepath that is triggered by OBJECT_CREATION_USES_RENAMES ought to behave correctly even on non-Windows platforms, so bending the error code of rename() only on Windows to fit the existing error handling would not be a smart thing to do. Rather, the rename() emulation should leave a correct errno and the caller should be updated to be aware of that error that is not EEXIST, which it currently knows about. Thanks for spotting a problem and digging to its cause. This is a #leftoverbits tangent, and should be done separately from your "OBJECT_CREATION_USES_RENAMES is broken" topic, but I think it is a bug to use finalize_object_file() directly to "finalize" anything but an individual loose object file in the first place. We should create a new shared helper that does what the function currently does, make finalize_object_file() call that new shared helper, and make sure finalize_object_file() is called only on a newly created loose object file. The codepath that creates a new packfile and other things and moves them to the final name should not call finalize_object_file() but the new shared helper instead. That way, we could later implement the "collision? check" alluded by the in-code comment in finailize_object_file(), and we won't have to worry about affecting callers other than the one that creates a loose object file with such an enhancement.
