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

From: Jeff King <peff@peff.net>
Date: Aug 24, 2015, 18:35 UTC
Message-ID: <20150824183554.GA5883@sigill.intra.peff.net>
In-Reply-To: <xmqqlhd04ko4.fsf@gitster.dls.corp.google.com>

On Mon, Aug 24, 2015 at 11:15:55AM -0700, Junio C Hamano wrote:

Show 11 quoted lines

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

-Peff