Re: [PATCH RFC 02/24] ref-filter: add return value to some functions

[Оля Тележная]

2018-01-27 0:05 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Add return flag to format_ref_array_item, show_ref_array_item,
>> get_ref_array_info and populate_value for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>>  ref-filter.c | 21 +
>>  ref-filter.h |  4 ++--
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>
> Makes sense as a preparatory step to pass the return status
> throughout the call chain.  The functions that actually will detect
> issues should probably gain
>
> /*
>  * a comment before the function
>  * that documents what it does and what values it returns
>  * to signal the failure.
>  */
>
> before them; those that only pass the errorcode through to the
> caller do not necessarily have to, though.

I will add comments, agree, thank you.

>
>> -void show_ref_array_item(struct ref_array_item *info,
>> +int show_ref_array_item(struct ref_array_item *info,
>>const struct ref_format *format)
>>  {
>>   struct strbuf final_buf = STRBUF_INIT;
>> + int retval = format_ref_array_item(info, format, _buf);
>>
>> - format_ref_array_item(info, format, _buf);
>>   fwrite(final_buf.buf, 1, final_buf.len, stdout);
>>   strbuf_release(_buf);
>> - putchar('\n');
>> + if (!retval)
>> + putchar('\n');
>> + return retval;
>
> This is questionable.  Why does it write final_buf out regardless of
> the return value, even though it refrains from writing terminating LF
> upon non-zero return from the same function that prepared final_buf?
>
> "Because final_buf is left unmodified when formatter returns an
> error, and calling fwrite on an empty buffer ends up being a no-op"
> is a wrong answer---it relies on having too intimate knowledge on
> how the callee happens to work currently.

I will change that piece of code by putting fwrite into if statement
also, thank you. We really do not put anything to buffer if retval is
not equal to zero (checked that).

Thank you for all your comments, waiting for further review.
Olga

Re: [PATCH RFC 02/24] ref-filter: add return value to some functions

[Junio C Hamano]

Olga Telezhnaya  writes:

> Add return flag to format_ref_array_item, show_ref_array_item,
> get_ref_array_info and populate_value for further using.
> Need it to handle situations when item is broken but we can not invoke
> die() because we are in batch mode and all items need to be processed.
>
> Signed-off-by: Olga Telezhnaia 
> Mentored-by: Christian Couder 
> Mentored by: Jeff King 
> ---
>  ref-filter.c | 21 +
>  ref-filter.h |  4 ++--
>  2 files changed, 15 insertions(+), 10 deletions(-)

Makes sense as a preparatory step to pass the return status
throughout the call chain.  The functions that actually will detect
issues should probably gain

/*
 * a comment before the function
 * that documents what it does and what values it returns
 * to signal the failure.
 */

before them; those that only pass the errorcode through to the
caller do not necessarily have to, though.

> -void show_ref_array_item(struct ref_array_item *info,
> +int show_ref_array_item(struct ref_array_item *info,
>const struct ref_format *format)
>  {
>   struct strbuf final_buf = STRBUF_INIT;
> + int retval = format_ref_array_item(info, format, _buf);
>  
> - format_ref_array_item(info, format, _buf);
>   fwrite(final_buf.buf, 1, final_buf.len, stdout);
>   strbuf_release(_buf);
> - putchar('\n');
> + if (!retval)
> + putchar('\n');
> + return retval;

This is questionable.  Why does it write final_buf out regardless of
the return value, even though it refrains from writing terminating LF
upon non-zero return from the same function that prepared final_buf?

"Because final_buf is left unmodified when formatter returns an
error, and calling fwrite on an empty buffer ends up being a no-op"
is a wrong answer---it relies on having too intimate knowledge on
how the callee happens to work currently.

[PATCH RFC 02/24] ref-filter: add return value to some functions

[Olga Telezhnaya]

Add return flag to format_ref_array_item, show_ref_array_item,
get_ref_array_info and populate_value for further using.
Need it to handle situations when item is broken but we can not invoke
die() because we are in batch mode and all items need to be processed.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 21 +
 ref-filter.h |  4 ++--
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 37337b57aacf4..7030d35c84a81 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1405,7 +1405,7 @@ static void need_object(struct ref_array_item *ref) {
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
 {
int i;
 
@@ -1526,20 +1526,22 @@ static void populate_value(struct ref_array_item *ref)
break;
}
}
-   return;
+   return 0;
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct 
atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom, struct 
atom_value **v)
 {
+   int retval = 0;
if (!ref->value) {
-   populate_value(ref);
+   retval = populate_value(ref);
fill_missing_values(ref->value);
}
*v = >value[atom];
+   return retval;
 }
 
 /*
@@ -2124,7 +2126,7 @@ 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)
 {
@@ -2158,17 +2160,20 @@ void format_ref_array_item(struct ref_array_item *info,
die(_("format: %%(end) atom missing"));
strbuf_addbuf(final_buf, >output);
pop_stack_element();
+   return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
+int show_ref_array_item(struct ref_array_item *info,
 const struct ref_format *format)
 {
struct strbuf final_buf = STRBUF_INIT;
+   int retval = format_ref_array_item(info, format, _buf);
 
-   format_ref_array_item(info, format, _buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
strbuf_release(_buf);
-   putchar('\n');
+   if (!retval)
+   putchar('\n');
+   return retval;
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..ff416b733b4b1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,11 +110,11 @@ 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,
+int format_ref_array_item(struct ref_array_item *info,
   const struct ref_format *format,
   struct strbuf *final_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);
+int 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 */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */

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