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

[Jeff King <peff@peff.net>]

On Fri, Apr 15, 2016 at 12:47:15AM +0530, Karthik Nayak wrote:

> That does make sense, I guess then I'll stick to shortening all symref's
> by default and allowing the user to change this if needed via the '--format'
> option.

Thanks.

> About %(symref) not getting enough formatting options, I don't see anything
> else in %(upstream) which would be required in %(symref), maybe the 'strip=X'
> option could be added. But for now I see 'short' to be the only needed option.
> If anyone feels that something else would be useful, I'd be glad to
> add it along.

"strip" was the one I was most interested in. I thought "%(upstream)"
and "%(push)" would also take "dir" and "base", which "%(refname)" does.
I'm not sure when those are useful in the first place, but it seems like
they should apply equally well to any instance where we show a ref:
%(refname), %(upstream), %(push), or %(symref).

IOW, I'd expect the logic for handling atom->u.refname to be in a common
function that just gets fed ref->refname, or ref->symref, or whatever.

It looks like that's a little tricky for %(upstream) and %(push), which
have extra tracking options, but it's pretty trivial for %(symref):

diff --git a/ref-filter.c b/ref-filter.c
index 3435df1..816f8c0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -82,7 +82,6 @@ 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;
@@ -242,16 +241,6 @@ static void if_atom_parser(struct used_atom *atom, const char *arg)
 		die(_("unrecognized %%(if) argument: %s"), 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);
-}
-
 static void refname_atom_parser(struct used_atom *atom, const char *arg)
 {
 	if (!arg)
@@ -305,7 +294,7 @@ static struct {
 	{ "contents", FIELD_STR, contents_atom_parser },
 	{ "upstream", FIELD_STR, remote_ref_atom_parser },
 	{ "push", FIELD_STR, remote_ref_atom_parser },
-	{ "symref", FIELD_STR, symref_atom_parser },
+	{ "symref", FIELD_STR, refname_atom_parser },
 	{ "flag" },
 	{ "HEAD" },
 	{ "color", FIELD_STR, color_atom_parser },
@@ -1180,29 +1169,33 @@ char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
+static const char *show_ref(struct used_atom *atom, const char *refname);
+
 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, 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();
+	return show_ref(atom, ref->refname);
+}
+
+static const char *show_ref(struct used_atom *atom, const char *refname)
+{
 	if (atom->u.refname.option == R_SHORT)
-		return shorten_unambiguous_ref(ref->refname, warn_ambiguous_refs);
+		return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
 	else if (atom->u.refname.option == R_STRIP)
-		return strip_ref_components(ref->refname, atom->u.refname.strip);
+		return strip_ref_components(refname, atom->u.refname.strip);
 	else if (atom->u.refname.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 "";
@@ -1212,13 +1205,13 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	} else if (atom->u.refname.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;
 }
 
 /*


I suspect it could work for the remote_ref_atom_parser, too, if you did
something like:

  struct refname_parser_atom {
    enum { R_BASE, R_DIR, R_NORMAL, R_SHORT, R_STRIP } option;
    unsigned int strip;
  };

  struct used_atom {
    ...
    union {
      struct refname_parser_atom refname;
      struct {
        struct refname_parser_atom refname;
	enum { RR_TRACK, ... } option;
      } remote_ref;
      ...
  };

and then just passed the refname_parser_atom to show_ref() above. 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.

-Peff