Re: Is finalize_object_file in sha1_file.c handling errno from "rename" correctly?

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2017, 06:50 UTC
Message-ID: <xmqq4ls4xxok.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c9b3bc17110048f0b7943704cfbd8f68@APEXC04.auctionpay.com>

Wesley Smith <wsmith@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.

Show 9 quoted lines

> 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.