On Fri, Apr 15, 2016 at 2:06 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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.
>> 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.
> ---
> 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(¶ms, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html