Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Simon,

On Fri, 3 Nov 2017, Simon Ruderich wrote:

> On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
> >> I spent substantial time on making the sequencer code libified (it was far
> >> from it). That die() call may look okay now, but it is not at all okay if
> >> we want to make Git's source code cleaner and more reusable. And I want
> >> to.
> >>
> >> So my suggestion is to clean up write_file_buf() first, to stop behaving
> >> like a drunk lemming, and to return an error value already, and only then
> >> use it in sequencer.c.
> >
> > That would be fine with me, too.
> 
> I tried looking into this by adding a new write_file_buf_gently()
> (or maybe renaming write_file_buf to write_file_buf_or_die) and
> using it from write_file_buf() but I don't know the proper way to
> handle the error-case in write_file_buf(). Just calling
> die("write_file_buf") feels ugly, as the real error was already
> printed on screen by error_errno() and I didn't find any function
> to just exit without writing a message (which still respects
> die_routine). Suggestions welcome.

In my ideal world, we could use all those fancy refactoring tools that are
currently en vogue and simply turn *all* error()/error_errno() calls into
context-aware functions that can be told to die() right away, or to return
the error in an error buffer, depending hwhat the caller (or the call
chain, really) wants.

This is quite a bit more object-oriented than Git's code base, though, and
besides, I am not aware of any refactoring tool that would make this
painless (it's not just a matter of adding a parameter, you also have to
pass it through all of the call chain, something you get for free when
working with an object-oriented language).

So what I did in the sequencer when faced with the same conundrum was to
simply return -1 if the function I called returned a negative value. The
top-level builtin (in that case, `rebase--helper`) simply returns !!ret as
exit code (so that `-1` gets translated into the exit code `1`).

BTW I would not use the `_or_die()` convention, as it suggests that that
function will *always* die() in the error case. Instead, what I would
follow is the `, int die_on_error` pattern e.g. of `real_pathdup()`, and
simply *add* that parameter to the signature (and changing the return
value to `int`).

Ciao,
Dscho