Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>>
>> Add test and documentation for the same.
>> ---
>> Documentation/git-for-each-ref.txt | 6 ++++--
>> ref-filter.c | 28 +++++++++++++++++++++++-----
>> t/t6300-for-each-ref.sh | 9 +++++++++
>> 3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index c713ec0..a38cbf6 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -114,8 +114,10 @@ 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.
>> + or "=" (in sync). 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.
>>
>> push::
>> The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6a38089..6044eb0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>> if (!strcmp(formatp, "short"))
>> refname = shorten_unambiguous_ref(refname,
>> warn_ambiguous_refs);
>> - else if (!strcmp(formatp, "track") &&
>> + else if (skip_prefix(formatp, "track", &valp) &&
>> + strcmp(formatp, "trackshort") &&
>> (starts_with(name, "upstream") ||
>> starts_with(name, "push"))) {
>
> If you see here, we detect "track" first for
> %(upstream:track,nobracket)
Yes, but I still think that this was a bad idea. If you want nobracket
to apply to "track", then the syntax should be
%(upstream:track=nobracket). I think the "nobracket" should apply to
"upstream" (i.e. be global to the atom), hence
%(upstream:nobracket,track) and %(upstream:track,nobracket) should both
be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
but just ignoring "nobracket" in this case would make sense to me.
Special-casing the implementation of "nobracket" also means you're
special-casing its user-visible behavior. And as a user, I hate it when
the behavior is subtely different for things that look like each other
(in this case, %(align:...) and %(upstream:...) ).
> %(upstream:nobracket,track) to be supported then, I think we'll have
> to change this whole layout and have the detection done up where we
> locat "upstream" / "push", what would be a nice way to go around this?
You mean, below
else if (starts_with(name, "upstream")) {
within populate_value()?
I think it would, yes.
> What I could think of:
> 1. Do the cleanup that Junio and Matthieu suggested, where we
> basically parse the
> atoms and store them into a used_atom struct. I could either work on
> those patches
> before this and then rebase this on top.
> 2. Let this be and come back on it when implementing the above series.
> After reading Matthieu's and Junio's discussion, I lean towards the latter.
Leaving it as-is does not fit in my arguments to do the refactoring
later. It's not introducing "another instance of an existing pattern",
but actually a new pattern.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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