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

[Junio C Hamano <gitster@pobox.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.

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