git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4 15/16] branch: use ref-filter printing APIs

From
Karthik Nayak <karthik.188@gmail.com>
Date
Apr 15, 2016, 20:27 UTC
Message-ID
<CAOLa=ZSdnDFR52BJB1dm-D=gyUo=oB0aoh5pKAvwyNG5+u0P_A@mail.gmail.com>
In-Reply-To
<20160414203615.GA31504@sigill.intra.peff.net>
On Fri, Apr 15, 2016 at 2:06 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> On Thu, Apr 14, 2016 at 04:05:30PM -0400, Jeff King wrote:
>
>> It looks like that's a little tricky for %(upstream) and %(push), which
>> have extra tracking options, but it's pretty trivial for %(symref):
>> [...]
>> I suspect it could work for the remote_ref_atom_parser, too, if you did
>> something like:
>
> So here that is (handling both %(symref) and %(upstream), so replacing
> what I sent a few minutes ago). It's only lightly tested by me, so there
> may be more corner cases to think about. I kept things where they were
> to create a minimal diff, but if it gets squashed in, it might be worth
> re-ordering a little to avoid the need for forward declarations.
>

I had a look at your patch and even tested it, seems solid, I like how you integrated all these atoms together under refname_atom_parser_internal(). I'm squashing this in, for my re-roll. Thanks.

Show 15 quoted lines
>> I don't know if that would create weird corner cases with RR_SHORTEN
>> and RR_TRACK, though, which are currently in the same enum (and thus
>> cannot be used at the same time). But it's not like we parse
>> "%(upstream:short:track)" anyway, I don't think. For each "%()"
>> placeholder you have to choose one or the other: showing the tracking
>> info, or showing the refname (possibly with modifiers). So ":track"
>> isn't so much a modifier as "upstream:track" is this totally other
>> thing.
>
> So actually, we _do_ accept "%(upstream:short,track)" with your patch,
> which is somewhat nonsensical. It just ignores "short" and takes
> whatever option came last. Which is reasonable, though flagging an error
> would also be reasonable (and I think is what existing git does). I
> don't think it matters much either way.
>

I think it was decided a while ago that it'd be best to ignore all and accept the last argument without barfing, I couldn't find the exact link. But I'm open to both. But if you look at the %(align) atom, even that accepts mutually exclusive arguments and accepts the last argument without reporting an error.

Show 219 quoted lines
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 3435df1..951c57e 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -50,6 +50,11 @@ struct if_then_else {
>                 condition_satisfied : 1;
>  };
>
> +struct refname_atom {
> +       enum { R_BASE, R_DIR, R_NORMAL, R_SHORT, R_STRIP } option;
> +       unsigned int strip;
> +};
> +
>  /*
>   * An atom is a valid field atom listed below, possibly prefixed with
>   * a "*" to denote deref_tag().
> @@ -67,7 +72,8 @@ static struct used_atom {
>                 char color[COLOR_MAXLEN];
>                 struct align align;
>                 struct {
> -                       enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
> +                       enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
> +                       struct refname_atom refname;
>                         unsigned int nobracket: 1;
>                 } remote_ref;
>                 struct {
> @@ -82,16 +88,14 @@ static struct used_atom {
>                         enum { O_FULL, O_LENGTH, O_SHORT } option;
>                         unsigned int length;
>                 } objectname;
> -               enum { S_FULL, S_SHORT } symref;
> -               struct {
> -                       enum { R_BASE, R_DIR, R_NORMAL, R_SHORT, R_STRIP } option;
> -                       unsigned int strip;
> -               } refname;
> +               struct refname_atom refname;
>         } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
>  static int need_color_reset_at_eol;
>
> +static const char *show_ref(struct refname_atom *atom, const char *refname);
> +
>  static void color_atom_parser(struct used_atom *atom, const char *color_value)
>  {
>         if (!color_value)
> @@ -100,13 +104,34 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)
>                 die(_("unrecognized color: %%(color:%s)"), color_value);
>  }
>
> +static void refname_atom_parser_internal(struct refname_atom *atom,
> +                                        const char *arg, const char *name)
> +{
> +       if (!arg)
> +               atom->option = R_NORMAL;
> +       else if (!strcmp(arg, "short"))
> +               atom->option = R_SHORT;
> +       else if (skip_prefix(arg, "strip=", &arg)) {
> +               atom->option = R_STRIP;
> +               if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
> +                       die(_("positive value expected refname:strip=%s"), arg);
> +       } else if (!strcmp(arg, "dir"))
> +               atom->option = R_DIR;
> +       else if (!strcmp(arg, "base"))
> +               atom->option = R_BASE;
> +       else
> +               die(_("unrecognized %%(%s) argument: %s"), name, arg);
> +}
> +
>  static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>  {
>         struct string_list params = STRING_LIST_INIT_DUP;
>         int i;
>
>         if (!arg) {
> -               atom->u.remote_ref.option = RR_NORMAL;
> +               atom->u.remote_ref.option = RR_REF;
> +               refname_atom_parser_internal(&atom->u.remote_ref.refname,
> +                                            arg, atom->name);
>                 return;
>         }
>
> @@ -116,16 +141,17 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>         for (i = 0; i < params.nr; i++) {
>                 const char *s = params.items[i].string;
>
> -               if (!strcmp(s, "short"))
> -                       atom->u.remote_ref.option = RR_SHORTEN;
> -               else if (!strcmp(s, "track"))
> +               if (!strcmp(s, "track"))
>                         atom->u.remote_ref.option = RR_TRACK;
>                 else if (!strcmp(s, "trackshort"))
>                         atom->u.remote_ref.option = RR_TRACKSHORT;
>                 else if (!strcmp(s, "nobracket"))
>                         atom->u.remote_ref.nobracket = 1;
> -               else
> -                       die(_("unrecognized format: %%(%s)"), atom->name);
> +               else {
> +                       atom->u.remote_ref.option = RR_REF;
> +                       refname_atom_parser_internal(&atom->u.remote_ref.refname,
> +                                                    s, atom->name);
> +               }
>         }
>
>         string_list_clear(&params, 0);
> @@ -244,31 +270,12 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)
>
>  static void symref_atom_parser(struct used_atom *atom, const char *arg)
>  {
> -       if (!arg)
> -               atom->u.symref = S_FULL;
> -       else if (!strcmp(arg, "short"))
> -               atom->u.symref = S_SHORT;
> -       else
> -               die(_("unrecognized %%(symref) argument: %s"), arg);
> +       return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
>  }
>
>  static void refname_atom_parser(struct used_atom *atom, const char *arg)
>  {
> -       if (!arg)
> -               atom->u.refname.option = R_NORMAL;
> -       else if (!strcmp(arg, "short"))
> -               atom->u.refname.option = R_SHORT;
> -       else if (skip_prefix(arg, "strip=", &arg)) {
> -               atom->u.contents.option = R_STRIP;
> -               if (strtoul_ui(arg, 10, &atom->u.refname.strip) ||
> -                       atom->u.refname.strip <= 0)
> -                       die(_("positive value expected refname:strip=%s"), arg);
> -       } else if (!strcmp(arg, "dir"))
> -               atom->u.contents.option = R_DIR;
> -       else if (!strcmp(arg, "base"))
> -               atom->u.contents.option = R_BASE;
> -       else
> -               die(_("unrecognized %%(refname) argument: %s"), arg);
> +       return refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
>  }
>
>  static struct {
> @@ -1112,8 +1119,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                                     struct branch *branch, const char **s)
>  {
>         int num_ours, num_theirs;
> -       if (atom->u.remote_ref.option == RR_SHORTEN)
> -               *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +       if (atom->u.remote_ref.option == RR_REF)
> +               *s = show_ref(&atom->u.remote_ref.refname, refname);
>         else if (atom->u.remote_ref.option == RR_TRACK) {
>                 if (stat_tracking_info(branch, &num_ours,
>                                        &num_theirs, NULL)) {
> @@ -1145,8 +1152,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>                         *s = ">";
>                 else
>                         *s = "<>";
> -       } else /* RR_NORMAL */
> -               *s = refname;
> +       } else
> +               die("BUG: unhandled RR_* enum");
>  }
>
>  char *get_head_description(void)
> @@ -1184,41 +1191,43 @@ static const char *get_symref(struct used_atom *atom, struct ref_array_item *ref
>  {
>         if (!ref->symref)
>                 return "";
> -       else if (atom->u.symref == S_SHORT)
> -               return shorten_unambiguous_ref(ref->symref,
> -                                              warn_ambiguous_refs);
>         else
> -               return ref->symref;
> +               return show_ref(&atom->u.refname, ref->symref);
>  }
>
>  static const char *get_refname(struct used_atom *atom, struct ref_array_item *ref)
>  {
>         if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>                 return get_head_description();
> -       if (atom->u.refname.option == R_SHORT)
> -               return shorten_unambiguous_ref(ref->refname, warn_ambiguous_refs);
> -       else if (atom->u.refname.option == R_STRIP)
> -               return strip_ref_components(ref->refname, atom->u.refname.strip);
> -       else if (atom->u.refname.option == R_BASE) {
> +       return show_ref(&atom->u.refname, ref->refname);
> +}
> +
> +static const char *show_ref(struct refname_atom *atom, const char *refname)
> +{
> +       if (atom->option == R_SHORT)
> +               return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +       else if (atom->option == R_STRIP)
> +               return strip_ref_components(refname, atom->strip);
> +       else if (atom->option == R_BASE) {
>                 const char *sp, *ep;
>
> -               if (skip_prefix(ref->refname, "refs/", &sp)) {
> +               if (skip_prefix(refname, "refs/", &sp)) {
>                         ep = strchr(sp, '/');
>                         if (!ep)
>                                 return "";
>                         return xstrndup(sp, ep - sp);
>                 }
>                 return "";
> -       } else if (atom->u.refname.option == R_DIR) {
> +       } else if (atom->option == R_DIR) {
>                 const char *sp, *ep;
>
> -               sp = ref->refname;
> +               sp = refname;
>                 ep = strrchr(sp, '/');
>                 if (!ep)
>                         return "";
>                 return xstrndup(sp, ep - sp);
>         } else
> -               return ref->refname;
> +               return refname;
>  }
>
>  /*
-- 
Regards,
Karthik Nayak
Previous: Jeff KingNext: Jeff King
Message 30 of 31 in “port branch.c to use ref-filter's printing options”
  1. 00/16 port branch.c to use ref-filter's printing optionsKarthik Nayak, Apr 9, 2016
  2. 01/16 ref-filter: implement %(if), %(then), and %(else) atomsKarthik Nayak, Apr 9, 2016
  3. 02/16 ref-filter: include reference to 'used_atom' within 'atom_value'Karthik Nayak, Apr 9, 2016
  4. 03/16 ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)Karthik Nayak, Apr 9, 2016
  5. 04/16 ref-filter: modify "%(objectname:short)" to take lengthKarthik Nayak, Apr 9, 2016
  6. 05/16 ref-filter: move get_head_description() from branch.cKarthik Nayak, Apr 9, 2016
  7. 06/16 ref-filter: introduce format_ref_array_item()Karthik Nayak, Apr 9, 2016
  8. 07/16 ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreamsKarthik Nayak, Apr 9, 2016
  9. 08/16 ref-filter: add support for %(upstream:track,nobracket)Karthik Nayak, Apr 9, 2016
  10. 09/16 ref-filter: make "%(symref)" atom work with the ':short' modifierKarthik Nayak, Apr 9, 2016
  11. 10/16 ref-filter: introduce symref_atom_parser()Karthik Nayak, Apr 9, 2016
  12. 11/16 ref-filter: introduce refname_atom_parser()Karthik Nayak, Apr 9, 2016
  13. Jeff KingApr 14, 2016
  14. Karthik NayakApr 15, 2016
  15. 12/16 ref-filter: add support for %(refname:dir) and %(refname:base)Karthik Nayak, Apr 9, 2016
  16. 13/16 ref-filter: allow porcelain to translate messages in the outputKarthik Nayak, Apr 9, 2016
  17. 14/16 branch, tag: use porcelain outputKarthik Nayak, Apr 9, 2016
  18. 15/16 branch: use ref-filter printing APIsKarthik Nayak, Apr 9, 2016
  19. Junio C HamanoApr 12, 2016
  20. Junio C HamanoApr 12, 2016
  21. Karthik NayakApr 13, 2016
  22. Stefan BellerApr 16, 2016
  23. Karthik NayakApr 17, 2016
  24. 16/16 branch: implement '--format' optionKarthik Nayak, Apr 9, 2016
  25. Karthik NayakApr 13, 2016
  26. Jeff KingApr 13, 2016
  27. Karthik NayakApr 14, 2016
  28. Jeff KingApr 14, 2016
  29. Jeff KingApr 14, 2016
  30. Karthik NayakApr 15, 2016
  31. Jeff KingApr 15, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.