Re: ref-filter: how to improve the code

From: Jeff King <peff@peff.net>
Date: Mar 1, 2018, 15:04 UTC
Message-ID: <20180301150451.GC24907@sigill.intra.peff.net>
In-Reply-To: <CAL21BmkVq4j=hgupPvZqigSGQ-=rgwKVvzTD_X-_Z__8qmeKJg@mail.gmail.com>

On Thu, Mar 01, 2018 at 02:17:09PM +0300, Оля Тележная wrote:

Show 14 quoted lines

> >> I tried to replace all die("...") with `return error("...")` and
> >> finally exit(), but actual problem is that we print "error:..."
> >> instead of "fatal:...", and it looks funny.
> >
> > If you do that, then format_ref_array_item() is still going to print
> > things, even if it doesn't die(). But for "cat-file --batch", we usually
> > do not print errors at all, but instead just say "... missing" (although
> > it depends on the error; syntactic errors in the format string would
> > still cause us to write to stderr).
> 
> Not sure if you catch my idea. format_ref_array_item() will not print
> anything, it will just return an error code. And if there was an error
> - we will print it in show_ref_array_item() (or go back to cat-file
> and print what we want).

OK, I think I misunderstood. It seems like there are three possible strategies on the table:

  - low-level functions call error() and return -1, that gets passed up
    through mid-level functions like format_ref_array_item(), and then
    higher-level functions like show_ref_array_item() act on the error
    code and call die(). The user sees something like:

      error: unable to parse object 1234abcd
      fatal: unable to format object

  - low-level functions return a numeric error code, which is then
    formatted by higher-level functions like show_ref_array_item() to
    produce a specific message

  - low-level functions stuff an error code into a strbuf and return -1,
    and then higher-level functions like show_ref_array_item() will feed
    that message to die("%s", err.buf).

I think the first one, besides changing the output, is going to produce error() messages even for cases where we're calling format_ref_array_item() directly, because error() writes its output immediately.

The second is a pain in practice, because it doubles the work: you have to come up with a list of error codes, and then translate it them into strings. And there's no room to mention variable strings (like the name of the object).

So I think the third is really the only viable option.

-Peff