Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 于2021年6月17日周四 下午3:25写道：
>
>
> On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Mentored-by: Hariom Verma <hariom18599@xxxxxxxxx>
> > Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx>
> > ---
> > 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
