diff mbox series

[v2,5/9,GSOC] ref-filter: teach get_object() return useful value

Message ID 49063372e0035c5384f834d78854da56f5726d13.1623763747.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series cat-file: reuse ref-filter logic | expand

Commit Message

ZheNing Hu June 15, 2021, 1:29 p.m. UTC 
From: ZheNing Hu <adlternative@gmail.com>

Let `populate_value()`, `get_ref_atom_value()` and
`format_ref_array_item()` get the return value of `get_object()`
correctly. This can help us later let `cat-file --batch` get the
correct error message and return value of `get_object()`.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Hariom Verma <hariom18599@gmail.com>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 ref-filter.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Comments

Junio C Hamano June 16, 2021, 7:36 a.m. UTC | #1 
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> Let `populate_value()`, `get_ref_atom_value()` and
> `format_ref_array_item()` get the return value of `get_object()`
> correctly.

The "get" the value correctly, I think.  What you are teaching them
is to pass the return value from get_object() through the callchain
to their callers.

The readers will be helped if you say what kind of errors
get_object() wants to tell its callers, not just "-1" is for error,
which is what populate_value() assumes to be sufficient.  In other
words, which non-zero returns from get_object() are interesting and
why?

> @@ -1997,9 +1997,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  static int get_ref_atom_value(struct ref_array_item *ref, int atom,
>  			      struct atom_value **v, struct strbuf *err)
>  {
> +	int ret = 0;
> +
>  	if (!ref->value) {
> -		if (populate_value(ref, err))
> -			return -1;
> +		if ((ret = populate_value(ref, err)))
> +			return ret;

The new variable only needs to be in this scope, and does not have
to be shown to the entire function.

> @@ -2573,6 +2575,7 @@ int format_ref_array_item(struct ref_array_item *info,
>  {
>  	const char *cp, *sp, *ep;
>  	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
> +	int ret = 0;

This is dubious...

>  	state.quote_style = format->quote_style;
>  	push_stack_element(&state.stack);
> @@ -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)) ||

Here, if "ret" gets assigned any non-zero value, the condition is
satisfied, and ...

>  		    atomv->handler(atomv, &state, error_buf)) {
>  			pop_stack_element(&state.stack);
> -			return -1;
> +			return ret ? ret : -1;

... the control flow will leave this function.  Therefore, ...

>  		}
>  	}
>  	if (*cp) {
> @@ -2610,7 +2613,7 @@ int format_ref_array_item(struct ref_array_item *info,
>  	}
>  	strbuf_addbuf(final_buf, &state.stack->output);
>  	pop_stack_element(&state.stack);
> -	return 0;
> +	return ret;

... at this point, "ret" can never be anything other than zero.  Am
I misreading the patch?

If I am not misreading the patch, then "ret" does not have to be
globally visible in this function---it can have the same scope as
"pos".

>  }
>  
>  void pretty_print_ref(const char *name, const struct object_id *oid,
ZheNing Hu June 17, 2021, 7:23 a.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> 于2021年6月16日周三 下午3:36写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Let `populate_value()`, `get_ref_atom_value()` and
> > `format_ref_array_item()` get the return value of `get_object()`
> > correctly.
>
> The "get" the value correctly, I think.  What you are teaching them
> is to pass the return value from get_object() through the callchain
> to their callers.
>

Yes, this is exactly what I meant.

> The readers will be helped if you say what kind of errors
> get_object() wants to tell its callers, not just "-1" is for error,
> which is what populate_value() assumes to be sufficient.  In other
> words, which non-zero returns from get_object() are interesting and
> why?
>

As stated in 765337a, We can just print the error without exiting if the
return value of format_ref_array_item() is greater than 0. Therefore,
the current patch is to make get_object() return a value other than
-1 when an error occurs.

> > @@ -1997,9 +1997,11 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
> >  static int get_ref_atom_value(struct ref_array_item *ref, int atom,
> >                             struct atom_value **v, struct strbuf *err)
> >  {
> > +     int ret = 0;
> > +
> >       if (!ref->value) {
> > -             if (populate_value(ref, err))
> > -                     return -1;
> > +             if ((ret = populate_value(ref, err)))
> > +                     return ret;
>
> The new variable only needs to be in this scope, and does not have
> to be shown to the entire function.
>

Makes sense.

> > @@ -2573,6 +2575,7 @@ int format_ref_array_item(struct ref_array_item *info,
> >  {
> >       const char *cp, *sp, *ep;
> >       struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
> > +     int ret = 0;
>
> This is dubious...
>
> >       state.quote_style = format->quote_style;
> >       push_stack_element(&state.stack);
> > @@ -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)) ||
>
> Here, if "ret" gets assigned any non-zero value, the condition is
> satisfied, and ...
>
> >                   atomv->handler(atomv, &state, error_buf)) {
> >                       pop_stack_element(&state.stack);
> > -                     return -1;
> > +                     return ret ? ret : -1;
>
> ... the control flow will leave this function.  Therefore, ...
>
> >               }
> >       }
> >       if (*cp) {
> > @@ -2610,7 +2613,7 @@ int format_ref_array_item(struct ref_array_item *info,
> >       }
> >       strbuf_addbuf(final_buf, &state.stack->output);
> >       pop_stack_element(&state.stack);
> > -     return 0;
> > +     return ret;
>
> ... at this point, "ret" can never be anything other than zero.  Am
> I misreading the patch?
>
> If I am not misreading the patch, then "ret" does not have to be
> globally visible in this function---it can have the same scope as
> "pos".
>

You are right, It is correct to only return 0 here at the moment.

Thanks.
--
ZheNing Hu
diff mbox series

Patch

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;
 	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;
 
 	/*
 	 * If there is no atom that wants to know about tagged
@@ -1997,9 +1997,11 @@  static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 static int get_ref_atom_value(struct ref_array_item *ref, int atom,
 			      struct atom_value **v, struct strbuf *err)
 {
+	int ret = 0;
+
 	if (!ref->value) {
-		if (populate_value(ref, err))
-			return -1;
+		if ((ret = populate_value(ref, err)))
+			return ret;
 		fill_missing_values(ref->value);
 	}
 	*v = &ref->value[atom];
@@ -2573,6 +2575,7 @@  int format_ref_array_item(struct ref_array_item *info,
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+	int ret = 0;
 
 	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
@@ -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);
-			return -1;
+			return ret ? ret : -1;
 		}
 	}
 	if (*cp) {
@@ -2610,7 +2613,7 @@  int format_ref_array_item(struct ref_array_item *info,
 	}
 	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
-	return 0;
+	return ret;
 }
 
 void pretty_print_ref(const char *name, const struct object_id *oid,