Re: ref-filter: how to improve the code

From: Jeff King <peff@peff.net>
Date: Feb 28, 2018, 13:25 UTC
Message-ID: <20180228132550.GB32272@sigill.intra.peff.net>
In-Reply-To: <CAL21Bmnz=H96EE=yerdigujYQ6M7Y_U-RkDF7oR-UPDU+cZNFA@mail.gmail.com>

On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote:

Show 13 quoted lines

> I am trying to remove cat-file formatting part and reuse same
> functionality from ref-filter.
> I have a problem that cat-file sometimes needs to continue running
> even if the request is broken, while in ref-filter we invoke die() in
> many places everywhere during formatting process. I write this email
> because I want to discuss how to implement the solution better.
> 
> ref-filter has 2 functions which could be interesting for us:
> format_ref_array_item() and show_ref_array_item(). I guess it's a good
> idea to print everything in show_ref_array_item(), including all
> errors. In that case all current users of ref-filter will invoke
> show_ref_array_item() (as they did it before), and cat-file could use
> format_ref_array_item() and work with the result in its own logic.

Yes, that arrangement makes sense to me.

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

Show 9 quoted lines

> One of the easiest solutions is to add strbuf parameter for errors to
> all functions that we use during formatting process, fill it in and
> print in show_ref_array_item() if necessary. What do you think about
> this idea?
> 
> Another way is to change the resulting error message, print current
> message with "error" prefix and then print something like "fatal:
> could not format the output". It is easier but I am not sure that it's
> a good idea to change the interface.

For reference, the first one is what we've been switching to in the refs code. And I think it's been fairly successful there.

-Peff