diff mbox series

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

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

Commit Message

ZheNing Hu June 12, 2021, 11:14 a.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_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 <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

Christian Couder June 12, 2021, 8:09 p.m. UTC | #1 
On Sat, Jun 12, 2021 at 1:14 PM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ZheNing Hu <adlternative@gmail.com>
>
> 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()`.

Is it get_object() or get_value()?
ZheNing Hu June 13, 2021, 10:11 a.m. UTC | #2 
Christian Couder <christian.couder@gmail.com> 于2021年6月13日周日 上午4:09写道：
>
> On Sat, Jun 12, 2021 at 1:14 PM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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()`.
>
> Is it get_object() or get_value()?

Oh, it's get_object().

Thanks for pointing out:)
--
ZheNing Hu
Ævar Arnfjörð Bjarmason June 17, 2021, 7:22 a.m. UTC | #3 
On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>
>
> 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 <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(-)
>
> 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.
ZheNing Hu June 17, 2021, 10:01 a.m. UTC | #4 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年6月17日周四 下午3:25写道：
>
>
> On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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 <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(-)
> >
> > 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)
>

I agree.

> >       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...
>

Yeah. This way it will be more readable.

> > @@ -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.

Indeed, inline assignment must be used in this case.

Thanks. I will pay attention to these style issues later.
--
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,