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

From: Jeff King <peff@peff.net>
Date: Nov 3, 2017, 18:57 UTC
Message-ID: <20171103185732.jr6a5wo2bmvh4onb@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.21.1.1711031425420.6482@virtualbox>

On Fri, Nov 03, 2017 at 03:46:44PM +0100, Johannes Schindelin wrote:

Show 20 quoted lines

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

FWIW, I sketched this out a bit here:

  https://public-inbox.org/git/20160928085841.aoisson3fnuke47q@sigill.intra.peff.net/

And you can see the patches I played with while writing that here:

  https://github.com/peff/git/compare/cb5918aa0d50f50e83787f65c2ddc3dcb10159fe...4d61927e66dcfdbdb6cc6c88ec4018e2142e826c

(but note they don't quite compile, as some of the conversions are half-done; it was really just to get a sense of the flavor of the thing).

One of the complaints was that it makes it harder to see when we are calling die() (because it's now happening via an error callback). That maybe confusing for users, but it may also affect generated code since the code paths that hit the NORETURN function are obscured.

But we could stop short of adding error_die, and just have error_silent, error_warn, and error_print (and callers can turn error_print into a die() themselves).

-Peff