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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jeff King <peff@xxxxxxxx> writes:

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



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]