git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Оля Тележная" <olyatelezhnaya@gmail.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [RFC 1/4] ref-filter: start adding strbufs with errors
Date: Wed, 14 Mar 2018 16:30:04 +0300	[thread overview]
Message-ID: <CAL21BmmQvjzpXGCjuHdPFiQ07P7VcV-E=mQbmM+35Y1u8acD6Q@mail.gmail.com> (raw)
In-Reply-To: <CAN0heSoJKxqSb0q5SR2SVfEzh6QjeiNNnESorZyYHTSC71jZuQ@mail.gmail.com>

2018-03-13 22:12 GMT+03:00 Martin Ågren <martin.agren@gmail.com>:
> On 13 March 2018 at 11:16, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:
>> This is a first step in removing any printing from
>> ref-filter formatting logic, so that it could be more general.
>> Everything would be the same for show_ref_array_item() users.
>> But, if you want to deal with errors by your own, you could invoke
>> format_ref_array_item(). It means that you need to print everything
>> (the result and errors) on your side.
>>
>> This commit changes signature of format_ref_array_item() by adding
>> return value and strbuf parameter for errors, and fixes
>> its callers.
>
> Minor nit: Maybe s/fixes its callers/adjusts its callers/. They are not
> broken or need to be fixed. They were simply playing the game according
> to the old rules, and now they need to learn the new ways. :-)

Agree.

>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> ---
>>  builtin/branch.c |  7 +++++--
>>  ref-filter.c     | 17 ++++++++++++-----
>>  ref-filter.h     |  7 ++++---
>>  3 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 8dcc2ed058be6..f86709ca42d5e 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>>         struct ref_array array;
>>         int maxwidth = 0;
>>         const char *remote_prefix = "";
>> -       struct strbuf out = STRBUF_INIT;
>
> You move this variable into the loop to reduce its scope. At first I
> suspected that this might mean we now start allocating+releasing in each
> run of the loop, which might be a performance-regression. But it turns
> out, we already did that, so this tightening of the scope has no such
> downsides. :-) From the commit message, I wasn't expecting this move,
> though. Maybe "While at it, reduce the scope of the out-variable."

Added this to commit message. I just wanted to unify code style. I
added another strbuf and tried to reduce its scope (not sure that it
will cause a performance regression, I guess compiler is smart enough
- but, who knows). I saw another strbuf, and I just decided to put
them together. In my opinion, it's easier to read the code where
variables are created in the smallest possible scope.
But, anyway, you are right, I needed to mention that in the commit message.

>
>>         char *to_free = NULL;
>>
>>         /*
>> @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>>         ref_array_sort(sorting, &array);
>>
>>         for (i = 0; i < array.nr; i++) {
>> -               format_ref_array_item(array.items[i], format, &out);
>> +               struct strbuf out = STRBUF_INIT;
>> +               struct strbuf err = STRBUF_INIT;
>> +               if (format_ref_array_item(array.items[i], format, &out, &err))
>> +                       die("%s", err.buf);
>
> Using "%s", good.
>
>>                 if (column_active(colopts)) {
>>                         assert(!filter->verbose && "--column and --verbose are incompatible");
>>                          /* format to a string_list to let print_columns() do its job */
>> @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>>                         fwrite(out.buf, 1, out.len, stdout);
>>                         putchar('\n');
>>                 }
>> +               strbuf_release(&err);
>>                 strbuf_release(&out);
>>         }
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 45fc56216aaa8..54fae00bdd410 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
>>         }
>>  }
>>
>> -void format_ref_array_item(struct ref_array_item *info,
>> +int format_ref_array_item(struct ref_array_item *info,
>>                            const struct ref_format *format,
>> -                          struct strbuf *final_buf)
>> +                          struct strbuf *final_buf,
>> +                          struct strbuf *error_buf)
>>  {
>>         const char *cp, *sp, *ep;
>>         struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
>> @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,
>>                 resetv.s = GIT_COLOR_RESET;
>>                 append_atom(&resetv, &state);
>>         }
>> -       if (state.stack->prev)
>> -               die(_("format: %%(end) atom missing"));
>> +       if (state.stack->prev) {
>> +               strbuf_addstr(error_buf, _("format: %(end) atom missing"));
>> +               return -1;
>> +       }
>>         strbuf_addbuf(final_buf, &state.stack->output);
>>         pop_stack_element(&state.stack);
>> +       return 0;
>>  }
>>
>>  void show_ref_array_item(struct ref_array_item *info,
>>                          const struct ref_format *format)
>>  {
>>         struct strbuf final_buf = STRBUF_INIT;
>> +       struct strbuf error_buf = STRBUF_INIT;
>>
>> -       format_ref_array_item(info, format, &final_buf);
>> +       if (format_ref_array_item(info, format, &final_buf, &error_buf))
>> +               die("%s", error_buf.buf);
>>         fwrite(final_buf.buf, 1, final_buf.len, stdout);
>> +       strbuf_release(&error_buf);
>
> I think this `strbuf_release()` will never actually do anything. If we
> get here, we had no error. But it makes sense (to me) to always be clear
> about releasing this. In this case it is easy enough.

I was thinking same as you (I want to be sure that we release everything).

>
> Possible counterargument: We might want this sort of "error-handling by
> strbufs" to follow this simple rule: return an error if and only if you
> add some error-string to the buffer. If this rule is universal enough,
> it might be ok to skip releasing these sort of buffers if you do not
> have an error.

I just think that it's more intuitive to release everything in all
cases, even if this line seems useless. Anyway, it's not super hard to
remove this line in any moment, so I guess we could wait with this
till the final review.

>
> Martin

  reply	other threads:[~2018-03-14 13:30 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 10:16 [RFC 1/4] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-13 10:16 ` [RFC 3/4] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-13 19:18   ` Martin Ågren
2018-03-14 13:36     ` Оля Тележная
2018-03-13 10:16 ` [RFC 4/4] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-13 10:16 ` [RFC 2/4] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-13 19:13   ` Martin Ågren
2018-03-13 19:12 ` [RFC 1/4] ref-filter: start adding strbufs with errors Martin Ågren
2018-03-14 13:30   ` Оля Тележная [this message]
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
2018-03-14 19:04   ` [PATCH v2 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-14 19:04   ` [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
2018-03-15 20:47     ` Martin Ågren
2018-03-15 21:01       ` Eric Sunshine
2018-03-16  7:20         ` Оля Тележная
2018-03-15 21:05       ` Junio C Hamano
2018-03-16  7:17       ` Оля Тележная
2018-03-14 19:04   ` [PATCH v2 4/5] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-14 19:04   ` [PATCH v2 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-15 22:48     ` Junio C Hamano
2018-03-16  7:22       ` Оля Тележная
2018-03-19 13:01   ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-19 13:01     ` [PATCH v3 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-19 19:41       ` Junio C Hamano
2018-03-19 13:01     ` [PATCH v3 4/5] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-19 13:01     ` [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
2018-03-19 13:01     ` [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-19 19:24       ` Junio C Hamano
2018-03-20 16:05     ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-20 16:05       ` [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
2018-03-20 18:19         ` Eric Sunshine
2018-03-20 22:30           ` Junio C Hamano
2018-03-20 22:50             ` Eric Sunshine
2018-03-21 19:30               ` Junio C Hamano
2018-03-21 19:58                 ` Eric Sunshine
2018-03-20 16:05       ` [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-20 18:18         ` Eric Sunshine
2018-03-20 16:05       ` [PATCH v4 4/5] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-20 16:05       ` [PATCH v4 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-21 18:28       ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
2018-03-21 18:28         ` [PATCH v5 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-21 18:28         ` [PATCH v5 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
2018-03-21 18:28         ` [PATCH v5 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-21 18:28         ` [PATCH v5 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-21 20:36           ` Junio C Hamano
2018-03-23  6:56             ` Оля Тележная
2018-03-21 18:28         ` [PATCH v5 5/6] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-21 20:20         ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Junio C Hamano
2018-03-23  6:48           ` Оля Тележная
2018-03-29 12:49         ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
2018-03-29 12:49           ` [PATCH v6 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-29 12:49           ` [PATCH v6 5/6] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-29 12:49           ` [PATCH v6 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-29 12:49           ` [PATCH v6 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
2018-03-29 12:49           ` [PATCH v6 4/6] ref-filter: change parsing function error handling Olga Telezhnaya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL21BmmQvjzpXGCjuHdPFiQ07P7VcV-E=mQbmM+35Y1u8acD6Q@mail.gmail.com' \
    --to=olyatelezhnaya@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).