Hi, On Fri, 15 Sep 2017, Wesley Smith wrote:
> Thanks for your response. I'm glad to see that you've been able to > understand the problem. I'm working with the Windows git team to properly > return EACCESS when "rename" fails due to access permissions, but it also > sounds like there will need to be a fix to finalize_object_file to better > handle the case of an existing file when renaming. The Windows part of the problem was fixed in v2.14.2. Ciao, Johannes [ leaving the rest of the quoted mail intact intentionally, to give readers a chance to read up on the context ] > Wesley Smith > T: 503.597.0556 | wsm...@greatergiving.com > > -----Original Message----- > From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Thursday, September 14, 2017 11:51 PM > To: Wesley Smith > Cc: git@vger.kernel.org > Subject: Re: Is finalize_object_file in sha1_file.c handling errno from > "rename" correctly? > > Wesley Smith <wsm...@greatergiving.com> 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. > >
