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

Wesley Smith [off-list ref] 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.