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

From: "Оля Тележная" <olyatelezhnaya@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH v3 02/23] ref-filter: add return value to some functions
Date: Thu, 15 Feb 2018 12:59:00 +0300	[thread overview]
Message-ID: <CAL21BmkQJTGYwR2BywNtsDVuMSGQVO+eWDTqTJL-WoiuAjmg0w@mail.gmail.com> (raw)
In-Reply-To: <20180215051658.GB18780@sigill.intra.peff.net>

2018-02-15 8:16 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> 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.
>
> OK. The source of these errors would eventually be calls in
> populate_value(), but we don't flag any errors there yet (well, we do,
> but they all end up in die() for now). So I'd expect to see later in the
> series those die() calls converted to errors (I haven't looked further
> yet; just making a note to myself).
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>>
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>> + * Return 0 if everything was successful, -1 otherwise.
>>   */
>
> We discussed off-list the concept that the caller may want to know one
> of three outcomes:
>
>   - we completed the request, having accessed the object
>   - we completed the request, but it didn't require accessing any
>     objects
>   - an error occurred accessing the object
>
> Since callers like "cat-file" would need to check has_sha1_file()
> manually in the second case. Should this return value actually be an
> enum, which would make it easier to convert later to a tri-state?

I decided not to implement this particular scenario because all other
callers are waiting that everything will be printed inside ref-filter.
We just add support for cat-file there. I don't think that I need to
re-think all printing process and move printing logic to all other
callers so that cat-file will behave fine. In my opinion, in the final
version cat-file must accept all ref-filter logic parts and adapt to
them.

>
>> -static void populate_value(struct ref_array_item *ref)
>> +static int populate_value(struct ref_array_item *ref)
>>  {
>>       void *buf;
>>       struct object *obj;
>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>>               }
>>       }
>>       if (used_atom_cnt <= i)
>> -             return;
>> +             return 0;
>
> Most of these conversions are obviously correct, because they just turn
> a void return into one with a value. But this one is trickier:
>
>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
>>               ep = strchr(sp, ')');
>>               if (cp < sp)
>>                       append_literal(cp, sp, &state);
>> -             get_ref_atom_value(info,
>> -                                parse_ref_filter_atom(format, sp + 2, ep),
>> -                                &atomv);
>> +             if (get_ref_atom_value(info,
>> +                                    parse_ref_filter_atom(format, sp + 2, ep),
>> +                                    &atomv))
>> +                     return -1;
>>               atomv->handler(atomv, &state);
>>       }
>
> since it affects the control flow. Might we be skipping any necessary
> cleanup in the function if we see an error?
>
> It looks like we may have called push_stack_element(), but we'd never
> get to the end of the function where we call pop_stack_element(),
> causing us to leak.

Agree,  I will fix this.

>
> -Peff