Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Oct 13, 2015, 18:54 UTC
- Message-ID
- <vpqeggy61ma.fsf@grenoble-inp.fr>
- In-Reply-To
- <CAOLa=ZRZszOqHqJfOHhFqa-XDZbrcPCuSdvM_zMEYHURv8kW_A@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 44 quoted lines
> On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@gmail.com> 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.
Show 8 quoted lines
> 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/