On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>
> 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?
> Add test and documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
> Mentored-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> 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?
> 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(¶ms, 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?
> + 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);
> + }
> +
> + string_list_clear(¶ms, 0);
> }
>
> static void body_atom_parser(struct used_atom *atom, const char *arg)
> @@ -1045,25 +1064,27 @@ 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 == RR_SHORTEN)
> + if (atom->u.remote_ref.option == RR_SHORTEN)
> *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> - else if (atom->u.remote_ref == RR_TRACK) {
> + else if (atom->u.remote_ref.option == RR_TRACK) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> - *s = "[gone]";
> - return;
> - }
> -
> - if (!num_ours && !num_theirs)
> + *s = xstrdup("gone");
> + } else if (!num_ours && !num_theirs)
> *s = "";
> else if (!num_ours)
> - *s = xstrfmt("[behind %d]", num_theirs);
> + *s = xstrfmt("behind %d", num_theirs);
> else if (!num_theirs)
> - *s = xstrfmt("[ahead %d]", num_ours);
> + *s = xstrfmt("ahead %d", num_ours);
> else
> - *s = xstrfmt("[ahead %d, behind %d]",
> + *s = xstrfmt("ahead %d, behind %d",
> num_ours, num_theirs);
> - } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> + if (!atom->u.remote_ref.nobracket && *s[0]) {
> + const char *to_free = *s;
> + *s = xstrfmt("[%s]", *s);
> + free((void *)to_free);
> + }
> + } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL))
> return;
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index a92b36f..2c5f177 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -372,6 +372,8 @@ test_expect_success 'setup for upstream:track[short]' '
>
> test_atom head upstream:track '[ahead 1]'
> test_atom head upstream:trackshort '>'
> +test_atom head upstream:track,nobracket 'ahead 1'
> +test_atom head upstream:nobracket,track 'ahead 1'
> test_atom head push:track '[ahead 1]'
> test_atom head push:trackshort '>'
>
> --
> 2.10.2
>