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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 3, 2017, 14:46 UTC
Message-ID: <alpine.DEB.2.21.1.1711031425420.6482@virtualbox>
In-Reply-To: <20171103103248.4p45r4klojk5cf2g@ruderich.org>

Hi Simon,

On Fri, 3 Nov 2017, Simon Ruderich wrote:

Show 21 quoted lines

> 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