Re: [PATCH v7 08/17] ref-filter: add support for %(upstream:track,nobracket)

On Wed, Nov 9, 2016 at 5:15 AM, Jacob Keller [off-list ref] wrote:

On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak [off-list ref] wrote:

quoted

From: Karthik Nayak <redacted>

Add support for %(upstream:track,nobracket) which will print the
tracking information without the brackets (i.e. "ahead N, behind M").
This is needed when we port branch.c to use ref-filter's printing APIs.

Makes sense. Seems a bit weird that we have the brackets normally
rather than adding them as an option, but I think this is ok. We don't
want to change all previous uses in this case.

My only suggestion here would be to add code so that the options die()
when we use nobracket along with trackshort or without track. This
ensures that the nobracket option only applies to track mode?

Sure, seems reasonable. There's also the fact that even though nobracket
actually only supports the 'track' option. It might make sense to
leave it as it
is, i.e not die() when used with the other options cause, it kinda stays true to
it being used. e.g. %(upstream:nobracket,trackshort) does give 'trackshort'
without brackets, even though that's the default option and only way we print
'trackshort' information, it seems to make sense from the users point of view.

quoted

Add test and documentation for the same.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Matthieu Moy [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
 Documentation/git-for-each-ref.txt |  8 +++--
 ref-filter.c                       | 67 +++++++++++++++++++++++++-------------
 t/t6300-for-each-ref.sh            |  2 ++
 3 files changed, 51 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index fd365eb..3953431 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt

@@ -118,9 +118,11 @@ upstream::
        `refname` above.  Additionally respects `:track` to show
        "[ahead N, behind M]" and `:trackshort` to show the terse
        version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-       or "=" (in sync).  Has no effect if the ref does not have
-       tracking information associated with it. `:track` also prints
-       "[gone]" whenever unknown upstream ref is encountered.
+       or "=" (in sync). `:track` also prints "[gone]" whenever
+       unknown upstream ref is encountered. Append `:track,nobracket`
+       to show tracking information without brackets (i.e "ahead N,
+       behind M").  Has no effect if the ref does not have tracking
+       information associated with it.

Ok so my comment on the previous patch is fixed here, the new wording
makes it much more clear that [gone] is not the same thing as no
information. So I don't think we should bother changing the previous
patch in the series. This might want to document that nobracket works
even without track, even if it doesn't actually do anything? Or make
the code more strict in that we die() if the values are put together
that make no sense?

Speaking from what I said above, maybe it makes sense to leave it like it is?

quoted

 push::
        The name of a local ref which represents the `@{push}` location

diff --git a/ref-filter.c b/ref-filter.c
index 6d51b80..4d7e414 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -46,8 +46,10 @@ static struct used_atom {
        union {
                char color[COLOR_MAXLEN];
                struct align align;
-               enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
-                       remote_ref;
+               struct {
+                       enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } option;
+                       unsigned int nobracket: 1;
+               } remote_ref;
                struct {
                        enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
                        unsigned int nlines;

@@ -75,16 +77,33 @@ static void color_atom_parser(struct used_atom *atom, const char *color_value)

 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-       if (!arg)
-               atom->u.remote_ref = RR_NORMAL;
-       else if (!strcmp(arg, "short"))
-               atom->u.remote_ref = RR_SHORTEN;
-       else if (!strcmp(arg, "track"))
-               atom->u.remote_ref = RR_TRACK;
-       else if (!strcmp(arg, "trackshort"))
-               atom->u.remote_ref = RR_TRACKSHORT;
-       else
-               die(_("unrecognized format: %%(%s)"), atom->name);
+       struct string_list params = STRING_LIST_INIT_DUP;
+       int i;
+
+       if (!arg) {
+               atom->u.remote_ref.option = RR_NORMAL;
+               return;
+       }
+
+       atom->u.remote_ref.nobracket = 0;
+       string_list_split(&params, arg, ',', -1);
+
+       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"))

Should we add die()s here to disallow setting the remote_ref option
multiple times? Otherwise, we should document that the last one wins?
Not sure it's really a big deal here, but we could do it to ensure
consistency for options which don't make sense together?

It makes sense to add a small note that the last option wins, I suppose


-- 
Regards,
Karthik Nayak