Re: [RFC 1/4] ref-filter: start adding strbufs with errors

2018-03-14 Thread Оля Тележная 
2018-03-13 22:12 GMT+03:00 Martin Ågren :
> On 13 March 2018 at 11:16, Olga Telezhnaya  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 
>> ---
>>  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, );
>>
>> for (i = 0; i < array.nr; i++) {
>> -   format_ref_array_item(array.items[i], format, );
>> +   struct strbuf out = STRBUF_INIT;
>> +   struct strbuf err = STRBUF_INIT;
>> +   if (format_ref_array_item(array.items[i], format, , 
>> ))
>> +   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();
>> strbuf_release();
>> }
>>
>> 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(, );
>> }
>> -   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, >output);
>> pop_stack_element();
>> +   return 0;
>>  }
>>
>>  void show_ref_array_item(struct ref_array_item *info,
>>  const struct ref_format *format)
>>  {
>> struct

Re: [RFC 1/4] ref-filter: start adding strbufs with errors

2018-03-13 Thread Martin Ågren 
On 13 March 2018 at 11:16, Olga Telezhnaya  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. :-)

> Signed-off-by: Olga Telezhnaia 
> ---
>  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."

> 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, );
>
> for (i = 0; i < array.nr; i++) {
> -   format_ref_array_item(array.items[i], format, );
> +   struct strbuf out = STRBUF_INIT;
> +   struct strbuf err = STRBUF_INIT;
> +   if (format_ref_array_item(array.items[i], format, , ))
> +   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();
> strbuf_release();
> }
>
> 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(, );
> }
> -   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, >output);
> pop_stack_element();
> +   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, _buf);
> +   if (format_ref_array_item(info, format, _buf, _buf))
> +   die("%s", error_buf.buf);
> fwrite(final_buf.buf, 1, final_buf.len, stdout);
> +   strbuf_release(_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.

Possible counterargument: We might want this sort of "error-handling by
strbufs" to follow this simple rule: return an error if and only if

[RFC 1/4] ref-filter: start adding strbufs with errors

2018-03-13 Thread Olga Telezhnaya 
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.

Signed-off-by: Olga Telezhnaia 
---
 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;
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, );
 
for (i = 0; i < array.nr; i++) {
-   format_ref_array_item(array.items[i], format, );
+   struct strbuf out = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
+   if (format_ref_array_item(array.items[i], format, , ))
+   die("%s", err.buf);
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();
strbuf_release();
}
 
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(, );
}
-   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, >output);
pop_stack_element();
+   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, _buf);
+   if (format_ref_array_item(info, format, _buf, _buf))
+   die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
strbuf_release(_buf);
putchar('\n');
 }
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
-  const struct ref_format *format,
-  struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const struct ref_format 
*format);
 /*  Parse a single sort specifier and add it to the list */

--
https://github.com/git/git/pull/466