Subject: Re: [PATCH 5/8] [GSOC] ref-filter: teach get_object() return useful value

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 17 Jun 2021 09:22:39 +0200

On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <[email-masked]>
>
> Let `populate_value()`, `get_ref_atom_value()` and
> `format_ref_array_item()` get the return value of `get_value()`
> correctly. This can help us later let `cat-file --batch` get the
> correct error message and return value of `get_value()`.
>
> Mentored-by: Christian Couder <[email-masked]>
> Mentored-by: Hariom Verma <[email-masked]>
> Signed-off-by: ZheNing Hu <[email-masked]>
> ---
>  ref-filter.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 8868cf98f090..420c0bf9384f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1808,7 +1808,7 @@ static char *get_worktree_path(const struct used_atom *atom, const struct ref_ar
>  static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  {
>  	struct object *obj;
> -	int i;
> +	int i, ret = 0;

The usual style is to not bunch up variables based on type, but only if
they're related, i.e. we'd do:

    if i, j; /* proceed to use i and j in two for-loops */

But:

    int i; /* for the for-loop */
    int ret = 0; /* for our return value */

(Without the comments)

>  	struct object_info empty = OBJECT_INFO_INIT;
>  
>  	CALLOC_ARRAY(ref->value, used_atom_cnt);
> @@ -1965,8 +1965,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  
>  
>  	oi.oid = ref->objectname;
> -	if (get_object(ref, 0, &obj, &oi, err))
> -		return -1;
> +	if ((ret = get_object(ref, 0, &obj, &oi, err)))
> +		return ret;

Maybe more personal style, I'd just write this as:

    ret = x();
    if (!ret)
        return ret;

Makes it easier to read and balance parens in your head for the common
case...

> @@ -2585,10 +2588,10 @@ int format_ref_array_item(struct ref_array_item *info,
>  		if (cp < sp)
>  			append_literal(cp, sp, &state);
>  		pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
> -		if (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) ||
> +		if (pos < 0 || (ret = get_ref_atom_value(info, pos, &atomv, error_buf)) ||
>  		    atomv->handler(atomv, &state, error_buf)) {
>  			pop_stack_element(&state.stack);

... and use that mental energy on readist stuff like this, which I'd
just leave as the inline assignment.