git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* ref-filter: how to improve the code
@ 2018-02-25 18:28 Оля Тележная
  2018-02-28 13:25 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Оля Тележная @ 2018-02-25 18:28 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: Jeff King, Christian Couder

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: ref-filter: how to improve the code
  2018-02-25 18:28 ref-filter: how to improve the code Оля Тележная
@ 2018-02-28 13:25 ` Jeff King
  2018-03-01 11:17   ` Оля Тележная
  0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2018-02-28 13:25 UTC (permalink / raw)
  To: Оля
	Тележная
  Cc: git, Junio C Hamano, Christian Couder

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: ref-filter: how to improve the code
  2018-02-28 13:25 ` Jeff King
@ 2018-03-01 11:17   ` Оля Тележная
  2018-03-01 15:04     ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Оля Тележная @ 2018-03-01 11:17 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Christian Couder

2018-02-28 16:25 GMT+03:00 Jeff King <peff@peff.net>:
> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: ref-filter: how to improve the code
  2018-03-01 11:17   ` Оля Тележная
@ 2018-03-01 15:04     ` Jeff King
  0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2018-03-01 15:04 UTC (permalink / raw)
  To: Оля
	Тележная
  Cc: git, Junio C Hamano, Christian Couder

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-03-01 15:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-25 18:28 ref-filter: how to improve the code Оля Тележная
2018-02-28 13:25 ` Jeff King
2018-03-01 11:17   ` Оля Тележная
2018-03-01 15:04     ` Jeff King

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).