{"thread":{"id":"47924","subject":"ref-filter: how to improve the code","startedAt":"2018-02-25T18:28:32Z","lastAt":"2018-03-01T15:04:58Z","messageCount":4,"participants":["Оля Тележная","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"340209","messageId":"CAL21Bmnz=H96EE=yerdigujYQ6M7Y_U-RkDF7oR-UPDU+cZNFA@mail.gmail.com","threadId":"47924","inReplyTo":null,"subject":"ref-filter: how to improve the code","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-25T18:28:25Z","receivedAt":"2018-02-25T18:28:32Z","isPatch":false,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Hi everyone,\nI am trying to remove cat-file formatting part and reuse same\nfunctionality from ref-filter.\nI have a problem that cat-file sometimes needs to continue running\neven if the request is broken, while in ref-filter we invoke die() in\nmany places everywhere during formatting process. I write this email\nbecause I want to discuss how to implement the solution better.\n\nref-filter has 2 functions which could be interesting for us:\nformat_ref_array_item() and show_ref_array_item(). I guess it's a good\nidea to print everything in show_ref_array_item(), including all\nerrors. In that case all current users of ref-filter will invoke\nshow_ref_array_item() (as they did it before), and cat-file could use\nformat_ref_array_item() and work with the result in its own logic.\n\nI tried to replace all die(\"...\") with `return error(\"...\")` and\nfinally exit(), but actual problem is that we print \"error:...\"\ninstead of \"fatal:...\", and it looks funny.\nThe draft of the code is here: https://github.com/telezhnaya/git/commits/p2\n\nOne of the easiest solutions is to add strbuf parameter for errors to\nall functions that we use during formatting process, fill it in and\nprint in show_ref_array_item() if necessary. What do you think about\nthis idea?\n\nAnother way is to change the resulting error message, print current\nmessage with \"error\" prefix and then print something like \"fatal:\ncould not format the output\". It is easier but I am not sure that it's\na good idea to change the interface.\n\nIf you have another ideas - please share them with me. It is really\nimportant step to make formatting logic more general and easier to\nreuse.\n\nThanks!\nOlga\n"},{"id":"340561","messageId":"20180228132550.GB32272@sigill.intra.peff.net","threadId":"47924","inReplyTo":"CAL21Bmnz=H96EE=yerdigujYQ6M7Y_U-RkDF7oR-UPDU+cZNFA@mail.gmail.com","subject":"Re: ref-filter: how to improve the code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-28T13:25:50Z","receivedAt":"2018-02-28T13:26:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote:\n\n> I am trying to remove cat-file formatting part and reuse same\n> functionality from ref-filter.\n> I have a problem that cat-file sometimes needs to continue running\n> even if the request is broken, while in ref-filter we invoke die() in\n> many places everywhere during formatting process. I write this email\n> because I want to discuss how to implement the solution better.\n> \n> ref-filter has 2 functions which could be interesting for us:\n> format_ref_array_item() and show_ref_array_item(). I guess it's a good\n> idea to print everything in show_ref_array_item(), including all\n> errors. In that case all current users of ref-filter will invoke\n> show_ref_array_item() (as they did it before), and cat-file could use\n> format_ref_array_item() and work with the result in its own logic.\n\nYes, that arrangement makes sense to me.\n\n> I tried to replace all die(\"...\") with `return error(\"...\")` and\n> finally exit(), but actual problem is that we print \"error:...\"\n> instead of \"fatal:...\", and it looks funny.\n\nIf you do that, then format_ref_array_item() is still going to print\nthings, even if it doesn't die(). But for \"cat-file --batch\", we usually\ndo not print errors at all, but instead just say \"... missing\" (although\nit depends on the error; syntactic errors in the format string would\nstill cause us to write to stderr).\n\n> One of the easiest solutions is to add strbuf parameter for errors to\n> all functions that we use during formatting process, fill it in and\n> print in show_ref_array_item() if necessary. What do you think about\n> this idea?\n> \n> Another way is to change the resulting error message, print current\n> message with \"error\" prefix and then print something like \"fatal:\n> could not format the output\". It is easier but I am not sure that it's\n> a good idea to change the interface.\n\nFor reference, the first one is what we've been switching to in the refs\ncode. And I think it's been fairly successful there.\n\n-Peff\n"},{"id":"340720","messageId":"CAL21BmkVq4j=hgupPvZqigSGQ-=rgwKVvzTD_X-_Z__8qmeKJg@mail.gmail.com","threadId":"47924","inReplyTo":"20180228132550.GB32272@sigill.intra.peff.net","subject":"Re: ref-filter: how to improve the code","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-01T11:17:09Z","receivedAt":"2018-03-01T11:17:18Z","isPatch":false,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-28 16:25 GMT+03:00 Jeff King <peff@peff.net>:\n> On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote:\n>\n>> I am trying to remove cat-file formatting part and reuse same\n>> functionality from ref-filter.\n>> I have a problem that cat-file sometimes needs to continue running\n>> even if the request is broken, while in ref-filter we invoke die() in\n>> many places everywhere during formatting process. I write this email\n>> because I want to discuss how to implement the solution better.\n>>\n>> ref-filter has 2 functions which could be interesting for us:\n>> format_ref_array_item() and show_ref_array_item(). I guess it's a good\n>> idea to print everything in show_ref_array_item(), including all\n>> errors. In that case all current users of ref-filter will invoke\n>> show_ref_array_item() (as they did it before), and cat-file could use\n>> format_ref_array_item() and work with the result in its own logic.\n>\n> Yes, that arrangement makes sense to me.\n>\n>> I tried to replace all die(\"...\") with `return error(\"...\")` and\n>> finally exit(), but actual problem is that we print \"error:...\"\n>> instead of \"fatal:...\", and it looks funny.\n>\n> If you do that, then format_ref_array_item() is still going to print\n> things, even if it doesn't die(). But for \"cat-file --batch\", we usually\n> do not print errors at all, but instead just say \"... missing\" (although\n> it depends on the error; syntactic errors in the format string would\n> still cause us to write to stderr).\n\nNot sure if you catch my idea. format_ref_array_item() will not print\nanything, it will just return an error code. And if there was an error\n- we will print it in show_ref_array_item() (or go back to cat-file\nand print what we want).\n\n>\n>> One of the easiest solutions is to add strbuf parameter for errors to\n>> all functions that we use during formatting process, fill it in and\n>> print in show_ref_array_item() if necessary. What do you think about\n>> this idea?\n>>\n>> Another way is to change the resulting error message, print current\n>> message with \"error\" prefix and then print something like \"fatal:\n>> could not format the output\". It is easier but I am not sure that it's\n>> a good idea to change the interface.\n>\n> For reference, the first one is what we've been switching to in the refs\n> code. And I think it's been fairly successful there.\n>\n> -Peff\n"},{"id":"340729","messageId":"20180301150451.GC24907@sigill.intra.peff.net","threadId":"47924","inReplyTo":"CAL21BmkVq4j=hgupPvZqigSGQ-=rgwKVvzTD_X-_Z__8qmeKJg@mail.gmail.com","subject":"Re: ref-filter: how to improve the code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-01T15:04:52Z","receivedAt":"2018-03-01T15:04:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 01, 2018 at 02:17:09PM +0300, Оля Тележная wrote:\n\n> >> I tried to replace all die(\"...\") with `return error(\"...\")` and\n> >> finally exit(), but actual problem is that we print \"error:...\"\n> >> instead of \"fatal:...\", and it looks funny.\n> >\n> > If you do that, then format_ref_array_item() is still going to print\n> > things, even if it doesn't die(). But for \"cat-file --batch\", we usually\n> > do not print errors at all, but instead just say \"... missing\" (although\n> > it depends on the error; syntactic errors in the format string would\n> > still cause us to write to stderr).\n> \n> Not sure if you catch my idea. format_ref_array_item() will not print\n> anything, it will just return an error code. And if there was an error\n> - we will print it in show_ref_array_item() (or go back to cat-file\n> and print what we want).\n\nOK, I think I misunderstood. It seems like there are three possible\nstrategies on the table:\n\n  - low-level functions call error() and return -1, that gets passed up\n    through mid-level functions like format_ref_array_item(), and then\n    higher-level functions like show_ref_array_item() act on the error\n    code and call die(). The user sees something like:\n\n      error: unable to parse object 1234abcd\n      fatal: unable to format object\n\n  - low-level functions return a numeric error code, which is then\n    formatted by higher-level functions like show_ref_array_item() to\n    produce a specific message\n\n  - low-level functions stuff an error code into a strbuf and return -1,\n    and then higher-level functions like show_ref_array_item() will feed\n    that message to die(\"%s\", err.buf).\n\nI think the first one, besides changing the output, is going to produce\nerror() messages even for cases where we're calling\nformat_ref_array_item() directly, because error() writes its output\nimmediately.\n\nThe second is a pain in practice, because it doubles the work: you have\nto come up with a list of error codes, and then translate it them into\nstrings. And there's no room to mention variable strings (like the name\nof the object).\n\nSo I think the third is really the only viable option.\n\n-Peff\n"}]}