Re: [PATCH v3] sha1_file: pass empty buffer to index empty file

On Tue, May 19, 2015 at 11:11:38AM -0700, Junio C Hamano wrote:

Subject: [PATCH] copy.c: make copy_fd() report its status silently

When copy_fd() function encounters errors, it emits error messages
itself, which makes it impossible for callers to take responsibility
for reporting errors, especially when they want to ignore certaion
errors.

Move the error reporting to its callers in preparation.
[...]

Looks good to me. And thank you for being thorough in analyzing the
impact on all the callers.

 - hold_lock_file_for_append(), when told to die on error, used to
   exit(128) relying on the error message from copy_fd(), but now it
   does its own die() instead.  Note that the callers that do not
   pass LOCK_DIE_ON_ERROR need to be adjusted for this change, but
   fortunately there is none ;-)

Not related to your patch, but I've often wondered if we can just get
rid of hold_lock_file_for_append. There's exactly one caller, and I
think it is doing the wrong thing. It is add_to_alternates_file(), but
shouldn't it probably read the existing lines to make sure it is not
adding a duplicate? IOW, I think hold_lock_file_for_append is a
fundamentally bad interface, because almost nobody truly wants to _just_
append.

And I have not investigated it carefully, but I suspect that we do not
even have to be that careful. The only time we write the file is during
clone, and I suspect we could just use a string_list, and then write it
out. We probably don't even need to lock (it's not like we take a lock
before creating the "objects" directory in the first place).

Anyway, end mini-rant. It is probably not hurting anyone and does not
need to be dealt with anytime soon.

-Peff