# ref-filter: how to improve the code 4 messages from 2018-02-25 to 2018-03-01. Participants: Оля Тележная, Jeff King. Thread: https://gitlist.dev/t/47924 ## Оля Тележная, 2018-02-25 18:28 Subject: ref-filter: how to improve the code Message-ID: URL: https://gitlist.dev/e/CAL21Bmnz%3DH96EE%3DyerdigujYQ6M7Y_U-RkDF7oR-UPDU%2BcZNFA%40mail.gmail.com ``` Hi everyone, 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. 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. The draft of the code is here: https://github.com/telezhnaya/git/commits/p2 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. If you have another ideas - please share them with me. It is really important step to make formatting logic more general and easier to reuse. Thanks! Olga ``` ## Jeff King, 2018-02-28 13:25 Subject: Re: ref-filter: how to improve the code Message-ID: <20180228132550.GB32272@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180228132550.GB32272%40sigill.intra.peff.net In-Reply-To: ``` On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote: > 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). > 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 ``` ## Оля Тележная, 2018-03-01 11:17 Subject: Re: ref-filter: how to improve the code Message-ID: URL: https://gitlist.dev/e/CAL21BmkVq4j%3DhgupPvZqigSGQ-%3DrgwKVvzTD_X-_Z__8qmeKJg%40mail.gmail.com In-Reply-To: <20180228132550.GB32272@sigill.intra.peff.net> ``` 2018-02-28 16:25 GMT+03:00 Jeff King : > On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote: > >> 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). 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). > >> 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 ``` ## Jeff King, 2018-03-01 15:04 Subject: Re: ref-filter: how to improve the code Message-ID: <20180301150451.GC24907@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180301150451.GC24907%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Mar 01, 2018 at 02:17:09PM +0300, Оля Тележная wrote: > >> 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 ```