Re: [PATCH 5/5] [GSOC] ref-filter: add %(rest) atom

[ZheNing Hu <adlternative@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年7月22日周四 下午4:24写道：
>
>
> On Thu, Jul 22 2021, ZheNing Hu via GitGitGadget wrote:
>
>
> > +             } else if (atom_type == ATOM_REST) {
> > +                     if (ref->rest)
> > +                             v->s = xstrdup(ref->rest);
> > +                     else
> > +                             v->s = xstrdup("");
> > +                     continue;
> >               } else
> >                       continue;
>
> Another light reading nit, maybe more readable as:
>
>     } else if (atom_type == ATOM_REST && ref->rest) {
>         ...
>     } else if (atom_type == ATOM_REST) {
>         ...
>     }
>     continue;
>
> But we can't do that "continue" at the end because there's two cases
> where we don't continue, I wondered if elimanting that special case
> would make it easier, patch below. Probably not worth it & feel free to
> ignore:

Yeah, the readability here is really bad, so many "continue" just to not execute
part of the operation on refname. In fact, I have a similar patch
(which will not
be sent to the mailing list for the time being), it use switch and
case to replace if
and else. [1]

>
>  ref-filter.c | 63 +++++++++++++++++++++++++-----------------------------------
>  1 file changed, 26 insertions(+), 37 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 81e77b13ad2..189244fed6f 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1890,18 +1890,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>                         name++;
>                 }
>
> -               if (atom_type == ATOM_REFNAME)
> +               if (atom_type == ATOM_REFNAME) {
>                         refname = get_refname(atom, ref);
> -               else if (atom_type == ATOM_WORKTREEPATH) {
> -                       if (ref->kind == FILTER_REFS_BRANCHES)
> -                               v->s = get_worktree_path(atom, ref);
> +                       if (!deref)
> +                               v->s = xstrdup(refname);
>                         else
> -                               v->s = xstrdup("");
> -                       continue;
> -               }
> -               else if (atom_type == ATOM_SYMREF)
> +                               v->s = xstrfmt("%s^{}", refname);
> +                       free((char *)refname);
> +               } else if (atom_type == ATOM_WORKTREEPATH &&
> +                          ref->kind == FILTER_REFS_BRANCHES) {
> +                       v->s = get_worktree_path(atom, ref);
> +               } else if (atom_type == ATOM_WORKTREEPATH) {
> +                       v->s = xstrdup("");
> +               } else if (atom_type == ATOM_SYMREF) {
>                         refname = get_symref(atom, ref);
> -               else if (atom_type == ATOM_UPSTREAM) {
> +                       if (!deref)
> +                               v->s = xstrdup(refname);
> +                       else
> +                               v->s = xstrfmt("%s^{}", refname);
> +                       free((char *)refname);
> +               } else if (atom_type == ATOM_UPSTREAM) {
>                         const char *branch_name;
>                         /* only local branches may have an upstream */
>                         if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -1916,7 +1924,6 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>                                 fill_remote_ref_details(atom, refname, branch, &v->s);
>                         else
>                                 v->s = xstrdup("");
> -                       continue;
>                 } else if (atom_type == ATOM_PUSH && atom->u.remote_ref.push) {
>                         const char *branch_name;
>                         v->s = xstrdup("");
> @@ -1935,10 +1942,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>                         /* We will definitely re-init v->s on the next line. */
>                         free((char *)v->s);
>                         fill_remote_ref_details(atom, refname, branch, &v->s);
> -                       continue;
>                 } else if (atom_type == ATOM_COLOR) {
>                         v->s = xstrdup(atom->u.color);
> -                       continue;
>                 } else if (atom_type == ATOM_FLAG) {
>                         char buf[256], *cp = buf;
>                         if (ref->flag & REF_ISSYMREF)
> @@ -1951,24 +1956,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>                                 *cp = '\0';
>                                 v->s = xstrdup(buf + 1);
>                         }
> -                       continue;
>                 } else if (!deref && atom_type == ATOM_OBJECTNAME) {
>                            v->s = xstrdup(do_grab_oid("objectname", &ref->objectname, atom));
> -                          continue;
> +               } else if (atom_type == ATOM_HEAD &&
> +                          atom->u.head &&
> +                          !strcmp(ref->refname, atom->u.head)) {
> +                       v->s = xstrdup("*");
>                 } else if (atom_type == ATOM_HEAD) {
> -                       if (atom->u.head && !strcmp(ref->refname, atom->u.head))
> -                               v->s = xstrdup("*");
> -                       else
> -                               v->s = xstrdup(" ");
> -                       continue;
> +                       v->s = xstrdup(" ");
>                 } else if (atom_type == ATOM_ALIGN) {
>                         v->handler = align_atom_handler;
>                         v->s = xstrdup("");
> -                       continue;
>                 } else if (atom_type == ATOM_END) {
>                         v->handler = end_atom_handler;
>                         v->s = xstrdup("");
> -                       continue;
>                 } else if (atom_type == ATOM_IF) {
>                         const char *s;
>                         if (skip_prefix(name, "if:", &s))
> @@ -1976,29 +1977,17 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>                         else
>                                 v->s = xstrdup("");
>                         v->handler = if_atom_handler;
> -                       continue;
>                 } else if (atom_type == ATOM_THEN) {
>                         v->handler = then_atom_handler;
>                         v->s = xstrdup("");
> -                       continue;
>                 } else if (atom_type == ATOM_ELSE) {
>                         v->handler = else_atom_handler;
>                         v->s = xstrdup("");
> -                       continue;
> +               } else if (atom_type == ATOM_REST && ref->rest) {
> +                       v->s = xstrdup(ref->rest);
>                 } else if (atom_type == ATOM_REST) {
> -                       if (ref->rest)
> -                               v->s = xstrdup(ref->rest);
> -                       else
> -                               v->s = xstrdup("");
> -                       continue;
> -               } else
> -                       continue;
> -
> -               if (!deref)
> -                       v->s = xstrdup(refname);
> -               else
> -                       v->s = xstrfmt("%s^{}", refname);
> -               free((char *)refname);
> +                       v->s = xstrdup("");
> +               }
>         }
>
>         for (i = 0; i < used_atom_cnt; i++) {

[1]: https://github.com/adlternative/git/commit/bbc6ecba4c0f65e7328e571b0d38ff99f800dc09