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

[Karthik Nayak]

 Wed, Oct 14, 2015 at 12:24 AM, Matthieu Moy
 wrote:
> 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:,nobracket) could complain,
> but just ignoring "nobracket" in this case would make sense to me.
>

Oh okay, was thinking only WRT the "track" option.

> 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:...) ).
>

Makes sense, was just looking for opinions.

>> %(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.
>

Yes, that's what I meant.

>> 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.
>

I meant after changing whatever we discussed above.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Matthieu Moy  writes:

>> 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.

That makes sense to me, as I think what is being discussed would be
%(upstream:track=yes,bracket=no) when it is fully spelled out.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Karthik Nayak]

On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak  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", ) &&
> +strcmp(formatp, "trackshort") &&
>  (starts_with(name, "upstream") ||
>   starts_with(name, "push"))) {

If you see here, we detect "track" first for
%(upstream:track,nobracket) so although
the idea was to use something similar to %(align:...) I don't see a
good way of going
about this. If we want %(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?

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.

> char buf[40];
> +   unsigned int nobracket = 0;
> +
> +   if (!strcmp(valp, ",nobracket"))
> +   nobracket = 1;
>
> if (stat_tracking_info(branch, _ours,
>_theirs, NULL)) {
> -   v->s = "[gone]";
> +   if (nobracket)
> +   v->s = "gone";
> +   else
> +   v->s = "[gone]";
> continue;
> }
>
> if (!num_ours && !num_theirs)
> v->s = "";
> else if (!num_ours) {
> -   sprintf(buf, "[behind %d]", 
> num_theirs);
> +   if (nobracket)
> +   sprintf(buf, "behind %d", 
> num_theirs);
> +   else
> +   sprintf(buf, "[behind %d]", 
> num_theirs);
> v->s = xstrdup(buf);
> } else if (!num_theirs) {
> -   sprintf(buf, "[ahead %d]", num_ours);
> +   if (nobracket)
> +   sprintf(buf, "ahead %d", 
> num_ours);
> +   else
> +   sprintf(buf, "[ahead %d]", 
> num_ours);
> v->s = xstrdup(buf);
> } else {
> -   sprintf(buf, "[ahead %d, behind %d]",
> +   if (nobracket)
> +

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

[Matthieu Moy]

Karthik Nayak  writes:

> On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak  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", ) &&
>> +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:,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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

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", ) &&
+strcmp(formatp, "trackshort") &&
 (starts_with(name, "upstream") ||
  starts_with(name, "push"))) {
char buf[40];
+   unsigned int nobracket = 0;
+
+   if (!strcmp(valp, ",nobracket"))
+   nobracket = 1;
 
if (stat_tracking_info(branch, _ours,
   _theirs, NULL)) {
-   v->s = "[gone]";
+   if (nobracket)
+   v->s = "gone";
+   else
+   v->s = "[gone]";
continue;
}
 
if (!num_ours && !num_theirs)
v->s = "";
else if (!num_ours) {
-   sprintf(buf, "[behind %d]", num_theirs);
+   if (nobracket)
+   sprintf(buf, "behind %d", 
num_theirs);
+   else
+   sprintf(buf, "[behind %d]", 
num_theirs);
v->s = xstrdup(buf);
} else if (!num_theirs) {
-   sprintf(buf, "[ahead %d]", num_ours);
+   if (nobracket)
+   sprintf(buf, "ahead %d", 
num_ours);
+   else
+   sprintf(buf, "[ahead %d]", 
num_ours);
v->s = xstrdup(buf);
} else {
-   sprintf(buf, "[ahead %d, behind %d]",
+   if (nobracket)
+   sprintf(buf, "ahead %d, behind 
%d",
+   num_ours, num_theirs);
+   else
+   sprintf(buf, "[ahead %d, behind 
%d]",
num_ours, num_theirs);
v->s = xstrdup(buf);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 4f620bf..7ab8bf8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
 '
 
 cat >expected expected <
 EOF
 
-- 
2.6.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matthieu Moy]

Karthik Nayak  writes:

> 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", ) &&
> +  strcmp(formatp, "trackshort") &&
>(starts_with(name, "upstream") ||
> starts_with(name, "push"))) {
>   char buf[40];
> + unsigned int nobracket = 0;
> +
> + if (!strcmp(valp, ",nobracket"))
> + nobracket = 1;
>  
>   if (stat_tracking_info(branch, _ours,
>  _theirs, NULL)) {
> - v->s = "[gone]";
> + if (nobracket)
> + v->s = "gone";
> + else
> + v->s = "[gone]";
>   continue;
>   }
>  
>   if (!num_ours && !num_theirs)
>   v->s = "";
>   else if (!num_ours) {
> - sprintf(buf, "[behind %d]", num_theirs);
> + if (nobracket)
> + sprintf(buf, "behind %d", 
> num_theirs);
> + else
> + sprintf(buf, "[behind %d]", 
> num_theirs);
>   v->s = xstrdup(buf);
>   } else if (!num_theirs) {
> - sprintf(buf, "[ahead %d]", num_ours);
> + if (nobracket)
> + sprintf(buf, "ahead %d", 
> num_ours);
> + else
> + sprintf(buf, "[ahead %d]", 
> num_ours);
>   v->s = xstrdup(buf);
>   } else {
> - sprintf(buf, "[ahead %d, behind %d]",
> + if (nobracket)
> + sprintf(buf, "ahead %d, behind 
> %d",
> + num_ours, num_theirs);
> + else
> + sprintf(buf, "[ahead %d, behind 
> %d]",
>   num_ours, num_theirs);
>   v->s = xstrdup(buf);
>   }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 4f620bf..7ab8bf8 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>  '
>  
>  cat >expected < +ahead 1
> +EOF
> +
> +test_expect_success 'Check upstream:track,nobracket format' '
> + git for-each-ref --format="%(upstream:track,nobracket)" refs/heads 
> >actual &&
> + test_cmp expected actual
> 

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

[Matthieu Moy]

Oops, sorry, I sent the wrong message, this one is empty. Please ignore.

Matthieu Moy  writes:

> Karthik Nayak  writes:
>
>> 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", ) &&
>> + strcmp(formatp, "trackshort") &&
>>   (starts_with(name, "upstream") ||
>>starts_with(name, "push"))) {
>>  char buf[40];
>> +unsigned int nobracket = 0;
>> +
>> +if (!strcmp(valp, ",nobracket"))
>> +nobracket = 1;
>>  
>>  if (stat_tracking_info(branch, _ours,
>> _theirs, NULL)) {
>> -v->s = "[gone]";
>> +if (nobracket)
>> +v->s = "gone";
>> +else
>> +v->s = "[gone]";
>>  continue;
>>  }
>>  
>>  if (!num_ours && !num_theirs)
>>  v->s = "";
>>  else if (!num_ours) {
>> -sprintf(buf, "[behind %d]", num_theirs);
>> +if (nobracket)
>> +sprintf(buf, "behind %d", 
>> num_theirs);
>> +else
>> +sprintf(buf, "[behind %d]", 
>> num_theirs);
>>  v->s = xstrdup(buf);
>>  } else if (!num_theirs) {
>> -sprintf(buf, "[ahead %d]", num_ours);
>> +if (nobracket)
>> +sprintf(buf, "ahead %d", 
>> num_ours);
>> +else
>> +sprintf(buf, "[ahead %d]", 
>> num_ours);
>>  v->s = xstrdup(buf);
>>  } else {
>> -sprintf(buf, "[ahead %d, behind %d]",
>> +if (nobracket)
>> +sprintf(buf, "ahead %d, behind 
>> %d",
>> +num_ours, num_theirs);
>> +else
>> +sprintf(buf, "[ahead %d, behind 
>> %d]",
>>  num_ours, num_theirs);
>>  v->s = xstrdup(buf);
>>  }
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 4f620bf..7ab8bf8 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>>  '
>>  
>>  cat >expected <> +ahead 1
>> +EOF
>> +
>>