Re: [PATCH 0/5] "am" state file fix with write_file() clean-up

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2015, 19:57 UTC
Message-ID: <xmqqzj1g31e5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150824183554.GA5883@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 27 quoted lines

> On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:
>
>> > This all looks good to me. The topics-in-flight compatibility stuff in
>> > patches 3 and 5 is neatly done. Usually I would just cheat and change
>> > the order of arguments to make the compiler notice such problems, but
>> > that's hard to do here because of the varargs (you cannot just bump
>> > "flags" to the end).
>> 
>> Actually, I think my compatibility stuff is worthless.  It would not
>> catch new callers that wants to only probe and do their own error
>> handling by passing 0 (and besides, assert() is a shoddy way to do
>> this---there is no guarantee that tests will trigger all the
>> codepaths in the first place).
>
> Oh, hrm, you're right. I was focused on making sure the common 1-passers
> were not broken, but patch 3 does break 0-passers (obviously, because
> they needed updated in the same patch ;) ).
>
> And I do agree that build-time assertions are much better than run-time
> ones.
>
>> We should deprecate and remove write_file() by renaming the one with
>> the updated semantics to something else, possibly with a backward
>> compatiblity thin wrapper around it that is called write_file(), or
>> without it to force a link-time error.
>
> That sounds reasonable. Maybe "format_to_file" or something?

I am going into a slightly different tangent. Binary support is not something we need right now, so I'll keep the door open for that in the future by

 - drop the "int fatal" altogether from write_file() without adding
   "unsigned flags";

 - add write_file_gently(), again without "unsigned flags";

 - make them call write_file_v() that takes "unsigned flags" and
   va_list.

I earlier said there were 2 oddball callers, one of them being suspicious, but it turns out that there are 3 callers that want write_file_gently(). Only the one in the setup codepath is asking for "fatal=0" while discarding the error return, which is suspicious; other two are handling an error themselves and are OK.