* [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 8:45 ` Matthieu Moy
` (2 more replies)
2015-07-28 6:56 ` [RFC/PATCH 03/11] ref-filter: add option to filter only branches Karthik Nayak
` (9 subsequent siblings)
10 siblings, 3 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
The 'colornext' atom allows us to color only the succeeding atom with
a particular color. This resets any previous color preferences set. It
is not compatible with `padright`. This is required for printing
formats like `git branch -vv` where the upstream is colored in blue.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-for-each-ref.txt | 5 +++++
ref-filter.c | 20 +++++++++++++++++++-
ref-filter.h | 4 +++-
t/t6302-for-each-ref-filter.sh | 16 ++++++++++++++++
4 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2b60aee..9dc02aa 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,6 +133,11 @@ padright::
padding. If the `value` is greater than the atom length, then
no padding is performed.
+colornext::
+ Change output color only for the next atom. Followed by
+ `<:colorname>`. Not compatible with `padright` and resets any
+ previous `color`, if set.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 4c5e3f8..3ab4ab9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -57,6 +57,7 @@ static struct {
{ "HEAD" },
{ "color" },
{ "padright" },
+ { "colornext" },
};
/*
@@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
v->modifier_atom = 1;
v->pad_to_right = 1;
continue;
+ } else if (starts_with(name, "colornext:")) {
+ char color[COLOR_MAXLEN] = "";
+
+ if (color_parse(name + 10, color) < 0)
+ die(_("unable to parse format"));
+ v->s = xstrdup(color);
+ v->modifier_atom = 1;
+ v->color_next = 1;
+ continue;
} else
continue;
@@ -1256,7 +1266,13 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
static void apply_formatting_state(struct ref_formatting_state *state,
struct atom_value *v, struct strbuf *value)
{
- if (state->pad_to_right) {
+ if (state->color_next && state->pad_to_right)
+ die(_("cannot use `colornext` and `padright` together"));
+ if (state->color_next) {
+ strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
+ return;
+ }
+ else if (state->pad_to_right) {
if (!is_utf8(v->s))
strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
else {
@@ -1346,6 +1362,8 @@ static void emit(const char *cp, const char *ep)
static void store_formatting_state(struct ref_formatting_state *state,
struct atom_value *atomv)
{
+ if (atomv->color_next)
+ state->color_next = atomv->s;
if (atomv->pad_to_right)
state->pad_to_right = atomv->ul;
}
diff --git a/ref-filter.h b/ref-filter.h
index fcf469e..8c82fd1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -21,12 +21,14 @@ struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
unsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */
- pad_to_right : 1;
+ pad_to_right : 1,
+ color_next : 1;
};
struct ref_formatting_state {
int quote_style;
unsigned int pad_to_right;
+ const char *color_next;
};
struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 68688a9..6aad069 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
test_cmp expect actual
'
+get_color ()
+{
+ git config --get-color no.such.slot "$1"
+}
+
+cat >expect <<EOF &&
+$(get_color green)foo1.10$(get_color reset)||
+$(get_color green)foo1.3$(get_color reset)||
+$(get_color green)foo1.6$(get_color reset)||
+EOF
+
+test_expect_success 'check `colornext` format option' '
+ git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 6:56 ` [RFC/PATCH 02/11] ref-filter: add 'colornext' atom Karthik Nayak
@ 2015-07-28 8:45 ` Matthieu Moy
2015-07-28 16:03 ` Karthik Nayak
2015-07-28 9:13 ` Christian Couder
2015-07-29 20:10 ` Eric Sunshine
2 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 8:45 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
> test_cmp expect actual
> '
>
> +get_color ()
> +{
> + git config --get-color no.such.slot "$1"
> +}
> +
> +cat >expect <<EOF &&
> +$(get_color green)foo1.10$(get_color reset)||
> +$(get_color green)foo1.3$(get_color reset)||
> +$(get_color green)foo1.6$(get_color reset)||
> +EOF
> +
> +test_expect_success 'check `colornext` format option' '
> + git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
> + test_cmp expect actual
> +'
This is not a very good test: you're not checking that colornext applies
to the next and only this one. Similarly to what I suggested for
padright, I'd suggest
--format="%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|"
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 8:45 ` Matthieu Moy
@ 2015-07-28 16:03 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 16:03 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 2:15 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
>> test_cmp expect actual
>> '
>>
>> +get_color ()
>> +{
>> + git config --get-color no.such.slot "$1"
>> +}
>> +
>> +cat >expect <<EOF &&
>> +$(get_color green)foo1.10$(get_color reset)||
>> +$(get_color green)foo1.3$(get_color reset)||
>> +$(get_color green)foo1.6$(get_color reset)||
>> +EOF
>> +
>> +test_expect_success 'check `colornext` format option' '
>> + git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
>> + test_cmp expect actual
>> +'
>
> This is not a very good test: you're not checking that colornext applies
> to the next and only this one. Similarly to what I suggested for
> padright, I'd suggest
>
> --format="%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|"
>
That was the purpose of the "||" but that doesn't check the color of next atom,
Thanks for the example will use that :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 6:56 ` [RFC/PATCH 02/11] ref-filter: add 'colornext' atom Karthik Nayak
2015-07-28 8:45 ` Matthieu Moy
@ 2015-07-28 9:13 ` Christian Couder
2015-07-28 16:04 ` Karthik Nayak
2015-07-29 20:10 ` Eric Sunshine
2 siblings, 1 reply; 88+ messages in thread
From: Christian Couder @ 2015-07-28 9:13 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
> v->modifier_atom = 1;
> v->pad_to_right = 1;
> continue;
> + } else if (starts_with(name, "colornext:")) {
> + char color[COLOR_MAXLEN] = "";
> +
> + if (color_parse(name + 10, color) < 0)
> + die(_("unable to parse format"));
Maybe use skip_prefix(), and die() with a more helpful message.
> + v->s = xstrdup(color);
> + v->modifier_atom = 1;
> + v->color_next = 1;
> + continue;
> } else
> continue;
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 9:13 ` Christian Couder
@ 2015-07-28 16:04 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 16:04 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 2:43 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
>> v->modifier_atom = 1;
>> v->pad_to_right = 1;
>> continue;
>> + } else if (starts_with(name, "colornext:")) {
>> + char color[COLOR_MAXLEN] = "";
>> +
>> + if (color_parse(name + 10, color) < 0)
>> + die(_("unable to parse format"));
>
> Maybe use skip_prefix(), and die() with a more helpful message.
>
Okay will do. Thanks!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-28 6:56 ` [RFC/PATCH 02/11] ref-filter: add 'colornext' atom Karthik Nayak
2015-07-28 8:45 ` Matthieu Moy
2015-07-28 9:13 ` Christian Couder
@ 2015-07-29 20:10 ` Eric Sunshine
2015-07-29 21:30 ` Matthieu Moy
2 siblings, 1 reply; 88+ messages in thread
From: Eric Sunshine @ 2015-07-29 20:10 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, Matthieu.Moy, gitster
On Tuesday, July 28, 2015, Karthik Nayak <karthik.188@gmail.com> wrote:
> The 'colornext' atom allows us to color only the succeeding atom with
> a particular color. This resets any previous color preferences set. It
> is not compatible with `padright`. This is required for printing
> formats like `git branch -vv` where the upstream is colored in blue.
Can you explain here and in the commit message why %(colornext) is not
compatible with %(padright:)? Is it an implementation limitation, or a
syntax or semantic limitation, or what? Can the implementation be
fixed to make it compatible or does it require a redesign?
Also, please explain here and in the commit message why this highly
specialized colorizer ('colornext'), is needed even though a more
general purpose one ('color') is already available.
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-29 20:10 ` Eric Sunshine
@ 2015-07-29 21:30 ` Matthieu Moy
2015-07-30 4:27 ` Jacob Keller
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 21:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Karthik Nayak, git, christian.couder, gitster
Eric Sunshine <sunshine@sunshineco.com> writes:
> Also, please explain here and in the commit message why this highly
> specialized colorizer ('colornext'), is needed even though a more
> general purpose one ('color') is already available.
It is needed in the current form to allow
%(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
and not the [].
But I now think that this would be more elegantly solved by Junio's
%(if) %(endif) idea:
%(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
(I added spaces for clarity)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-29 21:30 ` Matthieu Moy
@ 2015-07-30 4:27 ` Jacob Keller
2015-07-30 16:17 ` Junio C Hamano
0 siblings, 1 reply; 88+ messages in thread
From: Jacob Keller @ 2015-07-30 4:27 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Eric Sunshine, Karthik Nayak, git, christian.couder, gitster
On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Also, please explain here and in the commit message why this highly
>> specialized colorizer ('colornext'), is needed even though a more
>> general purpose one ('color') is already available.
>
> It is needed in the current form to allow
> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
> and not the [].
>
> But I now think that this would be more elegantly solved by Junio's
> %(if) %(endif) idea:
>
> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>
> (I added spaces for clarity)
>
I agree, this style seems a lot more elegant and expressive while much
easier to understand. Same for doing this to the alignment atoms and
such as it solves the same problem(s) there.
I can't speak to how easy it is to implement tho.
Regards,
Jake
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-30 4:27 ` Jacob Keller
@ 2015-07-30 16:17 ` Junio C Hamano
2015-08-01 13:06 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Junio C Hamano @ 2015-07-30 16:17 UTC (permalink / raw)
To: Jacob Keller
Cc: Matthieu Moy, Eric Sunshine, Karthik Nayak, git, christian.couder
Jacob Keller <jacob.keller@gmail.com> writes:
> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Also, please explain here and in the commit message why this highly
>>> specialized colorizer ('colornext'), is needed even though a more
>>> general purpose one ('color') is already available.
>>
>> It is needed in the current form to allow
>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
>> and not the [].
>>
>> But I now think that this would be more elegantly solved by Junio's
>> %(if) %(endif) idea:
>>
>> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>>
>> (I added spaces for clarity)
>
> I agree, this style seems a lot more elegant and expressive while much
> easier to understand. Same for doing this to the alignment atoms and
> such as it solves the same problem(s) there.
Do you mean something like these?
%(align:left,20) branch %(refname:short) %(end)
%(align:middle,20) branch %(refname:short) %(end)
%(align:right,20) branch %(refname:short) %(end)
to replace and enhance %(padright:20)?
> I can't speak to how easy it is to implement tho.
Perhaps it would go like this:
* Instead of always emitting to the standard output, emit() and
print_value() will gain an option to append into a strbuf that is
passed as argument. Alternatively, appending to strbuf could be
made the only output channel for them; show_ref_array_item() can
prepare an empty strbuf, call them repeatedly to fill it, and
then print the resulting strbuf itself.
* Things like %(if) and %(align) would do the following:
(1) Push the currently active strbuf it got from the calling
show_ref_array_item() to the formatting state;
(2) Create a new strbuf and arrange so that further output would
be diverted to this new one; and
(3) Push the fact that the diverted output will be processed by
them (i.e. %(if), %(align), etc.) when the diversion finishes
to the formatting state.
* When %(end) is seen, the currently active strbuf (i.e. the new
one created in (2) above for diversion) holds the output made
since the previously seen %(if), %(align), etc. The formatting
state knows what processing needs to be performed on that from
(3) above.
- Pop the strbuf where the output of the entire %(if)...%(end)
construct needs to go from the formatting state;
- Have the processing popped from (3) above, e.g. %(if:atom) or
%(align:left,20), do whatever they need to do on the diverted
output, and emit their result.
Both %(if) and the hypotherical %(align) can use this same
"diversion" mechanism. And the above would properly nest,
e.g.
%(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
2015-07-30 16:17 ` Junio C Hamano
@ 2015-08-01 13:06 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-08-01 13:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Matthieu Moy, Eric Sunshine, git, christian.couder
On Thu, Jul 30, 2015 at 9:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>> Also, please explain here and in the commit message why this highly
>>>> specialized colorizer ('colornext'), is needed even though a more
>>>> general purpose one ('color') is already available.
>>>
>>> It is needed in the current form to allow
>>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
>>> and not the [].
>>>
>>> But I now think that this would be more elegantly solved by Junio's
>>> %(if) %(endif) idea:
>>>
>>> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>>>
>>> (I added spaces for clarity)
>>
>> I agree, this style seems a lot more elegant and expressive while much
>> easier to understand. Same for doing this to the alignment atoms and
>> such as it solves the same problem(s) there.
>
> Do you mean something like these?
>
> %(align:left,20) branch %(refname:short) %(end)
> %(align:middle,20) branch %(refname:short) %(end)
> %(align:right,20) branch %(refname:short) %(end)
>
> to replace and enhance %(padright:20)?
>
>> I can't speak to how easy it is to implement tho.
>
> Perhaps it would go like this:
>
> * Instead of always emitting to the standard output, emit() and
> print_value() will gain an option to append into a strbuf that is
> passed as argument. Alternatively, appending to strbuf could be
> made the only output channel for them; show_ref_array_item() can
> prepare an empty strbuf, call them repeatedly to fill it, and
> then print the resulting strbuf itself.
>
> * Things like %(if) and %(align) would do the following:
>
> (1) Push the currently active strbuf it got from the calling
> show_ref_array_item() to the formatting state;
>
> (2) Create a new strbuf and arrange so that further output would
> be diverted to this new one; and
>
> (3) Push the fact that the diverted output will be processed by
> them (i.e. %(if), %(align), etc.) when the diversion finishes
> to the formatting state.
>
> * When %(end) is seen, the currently active strbuf (i.e. the new
> one created in (2) above for diversion) holds the output made
> since the previously seen %(if), %(align), etc. The formatting
> state knows what processing needs to be performed on that from
> (3) above.
>
> - Pop the strbuf where the output of the entire %(if)...%(end)
> construct needs to go from the formatting state;
>
> - Have the processing popped from (3) above, e.g. %(if:atom) or
> %(align:left,20), do whatever they need to do on the diverted
> output, and emit their result.
>
> Both %(if) and the hypotherical %(align) can use this same
> "diversion" mechanism. And the above would properly nest,
> e.g.
>
> %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)
>
This actually looks neat and good, I'll try implementing this :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 03/11] ref-filter: add option to filter only branches
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
2015-07-28 6:56 ` [RFC/PATCH 02/11] ref-filter: add 'colornext' atom Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 13:38 ` Matthieu Moy
2015-07-28 6:56 ` [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom Karthik Nayak
` (8 subsequent siblings)
10 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak
From: Karthik Nayak <karthik.188@gmail.com>
Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
and filter refs. This type checking is done by adding a
'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
Add an option in 'ref_filter_handler()' to filter different
types of branches by calling 'filter_branch_kind()' which
checks for the type of branch needed.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
ref-filter.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
ref-filter.h | 10 ++++++++--
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 3ab4ab9..3f40144 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1054,6 +1054,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
return NULL;
}
+/*
+ * Checks if a given refname is a branch and returns the kind of
+ * branch it is. If not a branch, 0 is returned.
+ */
+static int filter_branch_kind(struct ref_filter *filter, const char *refname)
+{
+ int kind, i;
+
+ static struct {
+ int kind;
+ const char *prefix;
+ } ref_kind[] = {
+ { REF_LOCAL_BRANCH, "refs/heads/" },
+ { REF_REMOTE_BRANCH, "refs/remotes/" },
+ };
+
+ /* If no kind is specified, no need to filter */
+ if (!filter->branch_kind)
+ return REF_NO_BRANCH_FILTERING;
+
+ for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+ if (starts_with(refname, ref_kind[i].prefix)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(ref_kind) <= i) {
+ if (!strcmp(refname, "HEAD"))
+ kind = REF_DETACHED_HEAD;
+ else
+ return 0;
+ }
+
+ if ((filter->branch_kind & kind) == 0)
+ return 0;
+
+ return kind;
+}
+
/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
static struct ref_array_item *new_ref_array_item(const char *refname,
const unsigned char *objectname,
@@ -1079,6 +1119,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
struct ref_filter *filter = ref_cbdata->filter;
struct ref_array_item *ref;
struct commit *commit = NULL;
+ unsigned int kind;
if (flag & REF_BAD_NAME) {
warning("ignoring ref with broken name %s", refname);
@@ -1090,6 +1131,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
return 0;
}
+ if (!(kind = filter_branch_kind(filter, refname)))
+ return 0;
+
if (!filter_pattern_match(filter, refname))
return 0;
@@ -1118,6 +1162,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
*/
ref = new_ref_array_item(refname, oid->hash, flag);
ref->commit = commit;
+ ref->kind = kind;
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
@@ -1208,6 +1253,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
ret = for_each_ref(ref_filter_handler, &ref_cbdata);
else if (type & FILTER_REFS_TAGS)
ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
+ else if (type & FILTER_REFS_BRANCHES)
+ ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
else if (type)
die("filter_refs: invalid type");
diff --git a/ref-filter.h b/ref-filter.h
index 8c82fd1..a021b04 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,6 +16,12 @@
#define FILTER_REFS_INCLUDE_BROKEN 0x1
#define FILTER_REFS_ALL 0x2
#define FILTER_REFS_TAGS 0x4
+#define FILTER_REFS_BRANCHES 0x8
+
+#define REF_DETACHED_HEAD 0x01
+#define REF_LOCAL_BRANCH 0x02
+#define REF_REMOTE_BRANCH 0x04
+#define REF_NO_BRANCH_FILTERING 0x08
struct atom_value {
const char *s;
@@ -40,7 +46,7 @@ struct ref_sorting {
struct ref_array_item {
unsigned char objectname[20];
- int flag;
+ int flag, kind;
const char *symref;
struct commit *commit;
struct atom_value *value;
@@ -66,7 +72,7 @@ struct ref_filter {
unsigned int with_commit_tag_algo : 1,
match_as_path : 1;
- unsigned int lines;
+ unsigned int lines, branch_kind;
};
struct ref_filter_cbdata {
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches
2015-07-28 6:56 ` [RFC/PATCH 03/11] ref-filter: add option to filter only branches Karthik Nayak
@ 2015-07-28 13:38 ` Matthieu Moy
2015-07-28 16:42 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 13:38 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
> +{
> + int kind, i;
> +
> + static struct {
> + int kind;
> + const char *prefix;
> + } ref_kind[] = {
> + { REF_LOCAL_BRANCH, "refs/heads/" },
> + { REF_REMOTE_BRANCH, "refs/remotes/" },
> + };
Nit: I would swap the order of fields, to make it a bit clearer that
this is a kind of dictionary key -> value (I think it's more common to
write it in this order than value <- key).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches
2015-07-28 13:38 ` Matthieu Moy
@ 2015-07-28 16:42 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 16:42 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 7:08 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
>> +{
>> + int kind, i;
>> +
>> + static struct {
>> + int kind;
>> + const char *prefix;
>> + } ref_kind[] = {
>> + { REF_LOCAL_BRANCH, "refs/heads/" },
>> + { REF_REMOTE_BRANCH, "refs/remotes/" },
>> + };
>
> Nit: I would swap the order of fields, to make it a bit clearer that
> this is a kind of dictionary key -> value (I think it's more common to
> write it in this order than value <- key).
>
This was borrowed from branch.c, but ok will change it!
Thanks
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
2015-07-28 6:56 ` [RFC/PATCH 02/11] ref-filter: add 'colornext' atom Karthik Nayak
2015-07-28 6:56 ` [RFC/PATCH 03/11] ref-filter: add option to filter only branches Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 7:54 ` Jacob Keller
` (2 more replies)
2015-07-28 6:56 ` [RFC/PATCH 05/11] branch: fix width computation Karthik Nayak
` (7 subsequent siblings)
10 siblings, 3 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
The 'ifexists' atom allows us to print a required format if the
preceeding atom has a value. If the preceeding atom has no value then
the format given is not printed. e.g. to print "[<refname>]" we can
now use the format "%(ifexists:[%s])%(refname)".
Add documentation and test for the same.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-for-each-ref.txt | 8 ++++++++
ref-filter.c | 37 ++++++++++++++++++++++++++++++++++---
ref-filter.h | 5 +++--
t/t6302-for-each-ref-filter.sh | 21 +++++++++++++++++++++
4 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 9dc02aa..4424020 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -138,6 +138,14 @@ colornext::
`<:colorname>`. Not compatible with `padright` and resets any
previous `color`, if set.
+ifexists::
+ Print required string only if the next atom specified in the
+ '--format' option exists.
+ e.g. --format="%(ifexists:[%s])%(symref)" prints the symref
+ like "[<symref>]" only if the ref has a symref. This was
+ incorporated to simulate the output of 'git branch -vv', where
+ we need to display the upstream branch in square brackets.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 3f40144..ff5a16b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -58,6 +58,7 @@ static struct {
{ "color" },
{ "padright" },
{ "colornext" },
+ { "ifexists" },
};
/*
@@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
v->modifier_atom = 1;
v->color_next = 1;
continue;
+ } else if (starts_with(name, "ifexists:")) {
+ skip_prefix(name, "ifexists:", &v->s);
+ if (!*v->s)
+ die(_("no string given with 'ifexists:'"));
+ v->modifier_atom = 1;
+ v->ifexists = 1;
+ continue;
} else
continue;
@@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,
{
if (state->color_next && state->pad_to_right)
die(_("cannot use `colornext` and `padright` together"));
- if (state->color_next) {
+ if (state->pad_to_right && state->ifexists)
+ die(_("cannot use 'align' and 'ifexists' together"));
+ if (state->color_next && !state->ifexists) {
strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
return;
- }
- else if (state->pad_to_right) {
+ } else if (state->ifexists) {
+ const char *sp = state->ifexists;
+
+ while (*sp) {
+ if (*sp != '%') {
+ strbuf_addch(value, *sp++);
+ continue;
+ } else if (sp[1] == '%') {
+ strbuf_addch(value, *sp++);
+ continue;
+ } else if (sp[1] == 's') {
+ if (state->color_next)
+ strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
+ else
+ strbuf_addstr(value, v->s);
+ sp += 2;
+ }
+ }
+
+ return;
+ } else if (state->pad_to_right) {
if (!is_utf8(v->s))
strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
else {
@@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,
state->color_next = atomv->s;
if (atomv->pad_to_right)
state->pad_to_right = atomv->ul;
+ if (atomv->ifexists)
+ state->ifexists = atomv->s;
}
static void reset_formatting_state(struct ref_formatting_state *state)
diff --git a/ref-filter.h b/ref-filter.h
index a021b04..7d1871d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,13 +28,14 @@ struct atom_value {
unsigned long ul; /* used for sorting when not FIELD_STR */
unsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */
pad_to_right : 1,
- color_next : 1;
+ color_next : 1,
+ ifexists : 1;
};
struct ref_formatting_state {
int quote_style;
unsigned int pad_to_right;
- const char *color_next;
+ const char *color_next, *ifexists;
};
struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 6aad069..29ed97b 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
test_cmp expect actual
'
+test_expect_success 'check `ifexists` format option' '
+ cat >expect <<-\EOF &&
+ [foo1.10]
+ [foo1.3]
+ [foo1.6]
+ EOF
+ git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
+ test_cmp expect actual
+'
+
+cat >expect <<EOF &&
+[$(get_color green)foo1.10$(get_color reset)]||foo1.10
+[$(get_color green)foo1.3$(get_color reset)]||foo1.3
+[$(get_color green)foo1.6$(get_color reset)]||foo1.6
+EOF
+
+test_expect_success 'check `ifexists` with `colornext` format option' '
+ git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 6:56 ` [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom Karthik Nayak
@ 2015-07-28 7:54 ` Jacob Keller
2015-07-28 16:47 ` Karthik Nayak
2015-07-28 8:50 ` Matthieu Moy
2015-07-28 17:57 ` Junio C Hamano
2 siblings, 1 reply; 88+ messages in thread
From: Jacob Keller @ 2015-07-28 7:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Christian Couder, Matthieu Moy, Junio C Hamano
On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> The 'ifexists' atom allows us to print a required format if the
> preceeding atom has a value. If the preceeding atom has no value then
Don't you mean "following atom" here? since you do document it as "the
next atom" below you should fix the commit message as well to match.
In any respect, this is a useful formatting atom :)
%(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?
> the format given is not printed. e.g. to print "[<refname>]" we can
> now use the format "%(ifexists:[%s])%(refname)".
>
> Add documentation and test for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-for-each-ref.txt | 8 ++++++++
> ref-filter.c | 37 ++++++++++++++++++++++++++++++++++---
> ref-filter.h | 5 +++--
> t/t6302-for-each-ref-filter.sh | 21 +++++++++++++++++++++
> 4 files changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 9dc02aa..4424020 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -138,6 +138,14 @@ colornext::
> `<:colorname>`. Not compatible with `padright` and resets any
> previous `color`, if set.
>
> +ifexists::
> + Print required string only if the next atom specified in the
> + '--format' option exists.
> + e.g. --format="%(ifexists:[%s])%(symref)" prints the symref
> + like "[<symref>]" only if the ref has a symref. This was
> + incorporated to simulate the output of 'git branch -vv', where
> + we need to display the upstream branch in square brackets.
> +
I suggest documenting that the atom will be placed into the contents
of ifexists via the %s indicator, as you do show an example but don't
explicitely say %s is the formatting character.
> In addition to the above, for commit and tag objects, the header
> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> be used to specify the value in the header field.
> diff --git a/ref-filter.c b/ref-filter.c
> index 3f40144..ff5a16b 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -58,6 +58,7 @@ static struct {
> { "color" },
> { "padright" },
> { "colornext" },
> + { "ifexists" },
> };
>
> /*
> @@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
> v->modifier_atom = 1;
> v->color_next = 1;
> continue;
> + } else if (starts_with(name, "ifexists:")) {
> + skip_prefix(name, "ifexists:", &v->s);
> + if (!*v->s)
> + die(_("no string given with 'ifexists:'"));
> + v->modifier_atom = 1;
> + v->ifexists = 1;
> + continue;
> } else
> continue;
>
> @@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,
> {
> if (state->color_next && state->pad_to_right)
> die(_("cannot use `colornext` and `padright` together"));
> - if (state->color_next) {
> + if (state->pad_to_right && state->ifexists)
> + die(_("cannot use 'align' and 'ifexists' together"));
> + if (state->color_next && !state->ifexists) {
> strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
> return;
> - }
> - else if (state->pad_to_right) {
> + } else if (state->ifexists) {
> + const char *sp = state->ifexists;
> +
> + while (*sp) {
> + if (*sp != '%') {
> + strbuf_addch(value, *sp++);
> + continue;
> + } else if (sp[1] == '%') {
> + strbuf_addch(value, *sp++);
> + continue;
> + } else if (sp[1] == 's') {
> + if (state->color_next)
> + strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
> + else
> + strbuf_addstr(value, v->s);
> + sp += 2;
> + }
> + }
> +
> + return;
> + } else if (state->pad_to_right) {
> if (!is_utf8(v->s))
> strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
> else {
> @@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,
> state->color_next = atomv->s;
> if (atomv->pad_to_right)
> state->pad_to_right = atomv->ul;
> + if (atomv->ifexists)
> + state->ifexists = atomv->s;
> }
>
> static void reset_formatting_state(struct ref_formatting_state *state)
> diff --git a/ref-filter.h b/ref-filter.h
> index a021b04..7d1871d 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -28,13 +28,14 @@ struct atom_value {
> unsigned long ul; /* used for sorting when not FIELD_STR */
> unsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */
> pad_to_right : 1,
> - color_next : 1;
> + color_next : 1,
> + ifexists : 1;
> };
>
> struct ref_formatting_state {
> int quote_style;
> unsigned int pad_to_right;
> - const char *color_next;
> + const char *color_next, *ifexists;
> };
>
> struct ref_sorting {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 6aad069..29ed97b 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
> test_cmp expect actual
> '
>
> +test_expect_success 'check `ifexists` format option' '
> + cat >expect <<-\EOF &&
> + [foo1.10]
> + [foo1.3]
> + [foo1.6]
> + EOF
> + git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
> + test_cmp expect actual
> +'
> +
> +cat >expect <<EOF &&
> +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
> +EOF
> +
> +test_expect_success 'check `ifexists` with `colornext` format option' '
> + git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.4.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 7:54 ` Jacob Keller
@ 2015-07-28 16:47 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 16:47 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git List, Christian Couder, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 1:24 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> The 'ifexists' atom allows us to print a required format if the
>> preceeding atom has a value. If the preceeding atom has no value then
>
> Don't you mean "following atom" here? since you do document it as "the
> next atom" below you should fix the commit message as well to match.
> In any respect, this is a useful formatting atom :)
>
That should have been `succeeding` atom. My bad! thanks :)
> %(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?
>
> I suggest documenting that the atom will be placed into the contents
> of ifexists via the %s indicator, as you do show an example but don't
> explicitely say %s is the formatting character.
>
Yeah! I should have explicitly mentioned that, thanks!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 6:56 ` [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom Karthik Nayak
2015-07-28 7:54 ` Jacob Keller
@ 2015-07-28 8:50 ` Matthieu Moy
2015-07-28 17:39 ` Karthik Nayak
2015-07-28 17:57 ` Junio C Hamano
2 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 8:50 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
> test_cmp expect actual
> '
>
> +test_expect_success 'check `ifexists` format option' '
> + cat >expect <<-\EOF &&
> + [foo1.10]
> + [foo1.3]
> + [foo1.6]
> + EOF
> + git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
> + test_cmp expect actual
> +'
You're testing only the positive case of ifexists, right? You need a
test for the negative case (i.e. the attribute does not exist) too.
Ideally, check that the ifexist actually applies to the right attribute,
like
--format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'
and manage to get an output like
<foo>
[bar]
<foobar> [barfoo]
> +cat >expect <<EOF &&
> +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
> +EOF
> +
> +test_expect_success 'check `ifexists` with `colornext` format option' '
> + git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
> + test_cmp expect actual
> +'
You have a double || that is not useful. Not really harmful either, but
it may distract the reader. You may want to s/||/|/.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 8:50 ` Matthieu Moy
@ 2015-07-28 17:39 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 17:39 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 2:20 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'check `ifexists` format option' '
>> + cat >expect <<-\EOF &&
>> + [foo1.10]
>> + [foo1.3]
>> + [foo1.6]
>> + EOF
>> + git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
>> + test_cmp expect actual
>> +'
>
> You're testing only the positive case of ifexists, right? You need a
> test for the negative case (i.e. the attribute does not exist) too.
> Ideally, check that the ifexist actually applies to the right attribute,
> like
>
> --format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'
>
> and manage to get an output like
>
> <foo>
> [bar]
> <foobar> [barfoo]
>
Yes! this seems like an important test, thanks :)
>
> You have a double || that is not useful. Not really harmful either, but
> it may distract the reader. You may want to s/||/|/.
>
Will change!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 6:56 ` [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom Karthik Nayak
2015-07-28 7:54 ` Jacob Keller
2015-07-28 8:50 ` Matthieu Moy
@ 2015-07-28 17:57 ` Junio C Hamano
2015-07-29 17:48 ` Karthik Nayak
2 siblings, 1 reply; 88+ messages in thread
From: Junio C Hamano @ 2015-07-28 17:57 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, Matthieu.Moy
Karthik Nayak <karthik.188@gmail.com> writes:
> The 'ifexists' atom allows us to print a required format if the
> preceeding atom has a value. If the preceeding atom has no value then
> the format given is not printed. e.g. to print "[<refname>]" we can
> now use the format "%(ifexists:[%s])%(refname)".
A handful of "huh?" on the design.
- The atom says "if *exists*" and explanation says "has a value".
How are they related? Does an atom whose value is an empty
string has a value? Or is "ifexists" meant to be used only to
ignore meaningless atom, e.g. %(*objectname) applied to a ref that
refers to an object that is not an annotated tag?
- That %s looks ugly. Are there cases where a user may want to say
%(ifexists:[%i]) or something other than 's' after that per-cent?
. Is it allowed to have more than one %s there?
. Is it allowed to have no %s there?
- The syntax makes the reader wonder if [] is part of the
construct, or just an example of any arbitrary string, i.e. is
"%(ifexists:the %s can be part of arbitrary string)" valid?
- If an arbitrary string is allowed, is there any quoting mechanism
to allow ")" to be part of that arbitrary string?
- What, if anything, is allowed to come between %(ifexists...) and
the next atom like %(refname)? For example, are these valid
constructs?
. %(ifexists...)%(padright:20)%(refname)
. %(ifexists...) %(refname) [%(subject)]
- This syntax does not seem to allow switching on an attribute to
show or not to show another, e.g. "if %(*objectname) makes sense,
then show '%(padright:20)%(refname:short) %(*subject)' for it".
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-28 17:57 ` Junio C Hamano
@ 2015-07-29 17:48 ` Karthik Nayak
2015-07-29 18:00 ` Junio C Hamano
0 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 17:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Christian Couder, Matthieu Moy
On Tue, Jul 28, 2015 at 11:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The 'ifexists' atom allows us to print a required format if the
>> preceeding atom has a value. If the preceeding atom has no value then
>> the format given is not printed. e.g. to print "[<refname>]" we can
>> now use the format "%(ifexists:[%s])%(refname)".
>
> A handful of "huh?" on the design.
>
> - The atom says "if *exists*" and explanation says "has a value".
> How are they related? Does an atom whose value is an empty
> string has a value? Or is "ifexists" meant to be used only to
> ignore meaningless atom, e.g. %(*objectname) applied to a ref that
> refers to an object that is not an annotated tag?
It's meant to ignore meaningless atom. atom's whose values are empty
strings are ignored.
>
> - That %s looks ugly. Are there cases where a user may want to say
> %(ifexists:[%i]) or something other than 's' after that per-cent?
>
Couldn't think of a better replacer, any suggestions would be welcome :)
> . Is it allowed to have more than one %s there?
> . Is it allowed to have no %s there?
>
1. yes its allowed to have multiple %s
2. yes no %s is also allowed.
> - The syntax makes the reader wonder if [] is part of the
> construct, or just an example of any arbitrary string, i.e. is
> "%(ifexists:the %s can be part of arbitrary string)" valid?
>
Its given as example, is that misleading?
> - If an arbitrary string is allowed, is there any quoting mechanism
> to allow ")" to be part of that arbitrary string?
Nope. Haven't done anything for that. I'll look into that :)
>
> - What, if anything, is allowed to come between %(ifexists...) and
> the next atom like %(refname)? For example, are these valid
> constructs?
>
> . %(ifexists...)%(padright:20)%(refname)
Doesn't work with padright, maybe we could eventually support that.
> . %(ifexists...) %(refname) [%(subject)]
>
Not sure what this is.
> - This syntax does not seem to allow switching on an attribute to
> show or not to show another, e.g. "if %(*objectname) makes sense,
> then show '%(padright:20)%(refname:short) %(*subject)' for it".
Yes this doesn't do that, I can say this is a pretty basic version, we could
probably work on and implement more things?
This is currently to support 'git branch -vv' where we have the upstream
in square brackets.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 17:48 ` Karthik Nayak
@ 2015-07-29 18:00 ` Junio C Hamano
2015-07-29 18:56 ` Junio C Hamano
2015-08-01 6:44 ` Karthik Nayak
0 siblings, 2 replies; 88+ messages in thread
From: Junio C Hamano @ 2015-07-29 18:00 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Matthieu Moy
Karthik Nayak <karthik.188@gmail.com> writes:
>> A handful of "huh?" on the design.
>>
>> - The atom says "if *exists*" and explanation says "has a value".
>> How are they related? Does an atom whose value is an empty
>> string has a value? Or is "ifexists" meant to be used only to
>> ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>> refers to an object that is not an annotated tag?
>
> It's meant to ignore meaningless atom. atom's whose values are empty
> strings are ignored.
That is a self-contradicting answer.
If you ask for "%(*objectname)" on a commit, that request truly is
meaningless, as a commit is not an annotated tag that points at another
object whose objectname is being asked for.
But if a commit has an empty log message (you should be able to
create such an object with commit-tree), then "%(subject)" would be
an empty string. The fact that the commit happens to have an empty
string as its message is far from meaningless.
Either you ignore an empty string, or you ignore meaningless one.
Which does "ifexists" mean?
>> - That %s looks ugly. Are there cases where a user may want to say
>> %(ifexists:[%i]) or something other than 's' after that per-cent?
>
> Couldn't think of a better replacer, any suggestions would be welcome :)
See below.
> Its given as example, is that misleading?
Othewise I wouldn't be asking.
>> - What, if anything, is allowed to come between %(ifexists...) and
>> the next atom like %(refname)? For example, are these valid
>> constructs?
>>
>> . %(ifexists...)%(padright:20)%(refname)
>
> Doesn't work ...
> ...
>> - This syntax does not seem to allow switching on an attribute to
>> show or not to show another, e.g. "if %(*objectname) makes sense,
>> then show '%(padright:20)%(refname:short) %(*subject)' for it".
>
> Yes this doesn't do that,
One way to do all of the above is to make it
%(ifexists:atom:expansionString)
That is, for example:
"%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))"
would give you a string "tag v1.0" with "v1.0" painted in blue for
refs/tags/v1.0 but nothing for refs/heads/master.
Obviously expansionString part needs some escaping mechanism to
allow it to include an unmatched ")".
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 18:00 ` Junio C Hamano
@ 2015-07-29 18:56 ` Junio C Hamano
2015-07-29 21:21 ` Matthieu Moy
2015-08-01 6:44 ` Karthik Nayak
1 sibling, 1 reply; 88+ messages in thread
From: Junio C Hamano @ 2015-07-29 18:56 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Matthieu Moy
Junio C Hamano <gitster@pobox.com> writes:
>> Couldn't think of a better replacer, any suggestions would be welcome :)
>
> See below.
> ...
> One way to do all of the above is ...
Note that is just "one way", not the only or not necessarily the
best. It certainly is not the easiest, I think.
%(if:atom)...%(endif)
might be easier to implement.
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 18:56 ` Junio C Hamano
@ 2015-07-29 21:21 ` Matthieu Moy
2015-07-29 22:05 ` Junio C Hamano
2015-08-01 6:46 ` Karthik Nayak
0 siblings, 2 replies; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git, Christian Couder
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>
>> See below.
>> ...
>> One way to do all of the above is ...
>
> Note that is just "one way", not the only or not necessarily the
> best. It certainly is not the easiest, I think.
>
> %(if:atom)...%(endif)
>
> might be easier to implement.
And I find it easier to read or write too. Nested parenthesis in a
format string make them really tricky. That removes the need for
escaping since the content of the if/endif is a format string like the
others, it can use the same escaping rules (IIRC, %% to escape a %).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 21:21 ` Matthieu Moy
@ 2015-07-29 22:05 ` Junio C Hamano
2015-08-01 6:46 ` Karthik Nayak
1 sibling, 0 replies; 88+ messages in thread
From: Junio C Hamano @ 2015-07-29 22:05 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Karthik Nayak, Git, Christian Couder
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>
>>> See below.
>>> ...
>>> One way to do all of the above is ...
>>
>> Note that is just "one way", not the only or not necessarily the
>> best. It certainly is not the easiest, I think.
>>
>> %(if:atom)...%(endif)
>>
>> might be easier to implement.
>
> And I find it easier to read or write too. Nested parenthesis in a
> format string make them really tricky. That removes the need for
> escaping since the content of the if/endif is a format string like the
> others, it can use the same escaping rules (IIRC, %% to escape a %).
Yeah, that is also a good point.
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 21:21 ` Matthieu Moy
2015-07-29 22:05 ` Junio C Hamano
@ 2015-08-01 6:46 ` Karthik Nayak
2015-08-01 7:05 ` Jacob Keller
1 sibling, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-08-01 6:46 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Git, Christian Couder
On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>
>>> See below.
>>> ...
>>> One way to do all of the above is ...
>>
>> Note that is just "one way", not the only or not necessarily the
>> best. It certainly is not the easiest, I think.
>>
>> %(if:atom)...%(endif)
>>
>> might be easier to implement.
>
> And I find it easier to read or write too. Nested parenthesis in a
> format string make them really tricky. That removes the need for
> escaping since the content of the if/endif is a format string like the
> others, it can use the same escaping rules (IIRC, %% to escape a %).
>
THat's a really good point. Will work on this :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-08-01 6:46 ` Karthik Nayak
@ 2015-08-01 7:05 ` Jacob Keller
0 siblings, 0 replies; 88+ messages in thread
From: Jacob Keller @ 2015-08-01 7:05 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Junio C Hamano, Git, Christian Couder
On Fri, Jul 31, 2015 at 11:46 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>>
>>>> See below.
>>>> ...
>>>> One way to do all of the above is ...
>>>
>>> Note that is just "one way", not the only or not necessarily the
>>> best. It certainly is not the easiest, I think.
>>>
>>> %(if:atom)...%(endif)
>>>
>>> might be easier to implement.
>>
>> And I find it easier to read or write too. Nested parenthesis in a
>> format string make them really tricky. That removes the need for
>> escaping since the content of the if/endif is a format string like the
>> others, it can use the same escaping rules (IIRC, %% to escape a %).
>>
>
> THat's a really good point. Will work on this :)
>
> --
> Regards,
> Karthik Nayak
> --
Not sure how much work it would take to extent other atoms to this
behavior, such as %(padright) and %(align) and such, that way they
could operate on literals and so forth.
Maybe not worth it as part of this GSoC project, as it may be too
complicated, but maybe something to mark as a "TODO" for future or
something?
The only issue being that it might mean we have to keep the old
implementation too for backwards compatibility .. Maybe it's easier to
implement that I think, or maybe it's far more challenging than I
think....
Regards,
Jake
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
2015-07-29 18:00 ` Junio C Hamano
2015-07-29 18:56 ` Junio C Hamano
@ 2015-08-01 6:44 ` Karthik Nayak
1 sibling, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-08-01 6:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Christian Couder, Matthieu Moy
On Wed, Jul 29, 2015 at 11:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> A handful of "huh?" on the design.
>>>
>>> - The atom says "if *exists*" and explanation says "has a value".
>>> How are they related? Does an atom whose value is an empty
>>> string has a value? Or is "ifexists" meant to be used only to
>>> ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>>> refers to an object that is not an annotated tag?
>>
>> It's meant to ignore meaningless atom. atom's whose values are empty
>> strings are ignored.
>
> That is a self-contradicting answer.
>
> If you ask for "%(*objectname)" on a commit, that request truly is
> meaningless, as a commit is not an annotated tag that points at another
> object whose objectname is being asked for.
>
> But if a commit has an empty log message (you should be able to
> create such an object with commit-tree), then "%(subject)" would be
> an empty string. The fact that the commit happens to have an empty
> string as its message is far from meaningless.
>
> Either you ignore an empty string, or you ignore meaningless one.
> Which does "ifexists" mean?
I meant ignore atom values which are empty, sorry for the confusion.
>
>>> - That %s looks ugly. Are there cases where a user may want to say
>>> %(ifexists:[%i]) or something other than 's' after that per-cent?
>>
>> Couldn't think of a better replacer, any suggestions would be welcome :)
>
> See below.
>
>> Its given as example, is that misleading?
>
> Othewise I wouldn't be asking.
>
>>> - What, if anything, is allowed to come between %(ifexists...) and
>>> the next atom like %(refname)? For example, are these valid
>>> constructs?
>>>
>>> . %(ifexists...)%(padright:20)%(refname)
>>
>> Doesn't work ...
>> ...
>>> - This syntax does not seem to allow switching on an attribute to
>>> show or not to show another, e.g. "if %(*objectname) makes sense,
>>> then show '%(padright:20)%(refname:short) %(*subject)' for it".
>>
>> Yes this doesn't do that,
>
> One way to do all of the above is to make it
>
> %(ifexists:atom:expansionString)
>
> That is, for example:
>
> "%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))"
>
> would give you a string "tag v1.0" with "v1.0" painted in blue for
> refs/tags/v1.0 but nothing for refs/heads/master.
>
> Obviously expansionString part needs some escaping mechanism to
> allow it to include an unmatched ")".
I liked your other idea of if and endif better :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 05/11] branch: fix width computation
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (2 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 9:47 ` Matthieu Moy
2015-07-28 6:56 ` [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list Karthik Nayak
` (6 subsequent siblings)
10 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak
From: Karthik Nayak <karthik.188@gmail.com>
Remove unnecessary variables from ref_list and ref_item
which were used for width computation. Make other changes
accordingly. This patch is a precursor for porting branch.c
to use ref-filter APIs.
Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 61 +++++++++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc8beb..b058b74 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
struct ref_item {
char *name;
char *dest;
- unsigned int kind, width;
+ unsigned int kind;
struct commit *commit;
int ignore;
};
struct ref_list {
struct rev_info revs;
- int index, alloc, maxwidth, verbose, abbrev;
+ int index, alloc, verbose, abbrev;
struct ref_item *list;
struct commit_list *with_commit;
int kinds;
@@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
newitem->name = xstrdup(refname);
newitem->kind = kind;
newitem->commit = commit;
- newitem->width = utf8_strwidth(refname);
newitem->dest = resolve_symref(orig_refname, prefix);
newitem->ignore = 0;
- /* adjust for "remotes/" */
- if (newitem->kind == REF_REMOTE_BRANCH &&
- ref_list->kinds != REF_REMOTE_BRANCH)
- newitem->width += 8;
- if (newitem->width > ref_list->maxwidth)
- ref_list->maxwidth = newitem->width;
return 0;
}
@@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
}
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
- int abbrev, int current, char *prefix)
+ int abbrev, int current, const char *remote_prefix)
{
char c;
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
+ const char *prefix = "";
if (item->ignore)
return;
@@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
break;
case REF_REMOTE_BRANCH:
color = BRANCH_COLOR_REMOTE;
+ prefix = remote_prefix;
break;
default:
color = BRANCH_COLOR_PLAIN;
@@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
strbuf_release(&out);
}
-static int calc_maxwidth(struct ref_list *refs)
+static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
{
- int i, w = 0;
+ int i, max = 0;
for (i = 0; i < refs->index; i++) {
+ struct ref_item *it = &refs->list[i];
+ int w = utf8_strwidth(it->name);
+
if (refs->list[i].ignore)
continue;
- if (refs->list[i].width > w)
- w = refs->list[i].width;
+ if (it->kind == REF_REMOTE_BRANCH)
+ w += remote_bonus;
+ if (w > max)
+ max = w;
}
- return w;
+ return max;
}
static char *get_head_description(void)
@@ -600,21 +600,18 @@ static char *get_head_description(void)
return strbuf_detach(&desc, NULL);
}
-static void show_detached(struct ref_list *ref_list)
+static void show_detached(struct ref_list *ref_list, int maxwidth)
{
struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
struct ref_item item;
item.name = get_head_description();
- item.width = utf8_strwidth(item.name);
item.kind = REF_LOCAL_BRANCH;
item.dest = NULL;
item.commit = head_commit;
item.ignore = 0;
- if (item.width > ref_list->maxwidth)
- ref_list->maxwidth = item.width;
- print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
+ print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
free(item.name);
}
}
@@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
int i;
struct append_ref_cb cb;
struct ref_list ref_list;
+ int maxwidth = 0;
+ const char *remote_prefix = "";
+
+ /*
+ * If we are listing more than just remote branches,
+ * then remote branches will have a "remotes/" prefix.
+ * We need to account for this in the width.
+ */
+ if (kinds != REF_REMOTE_BRANCH)
+ remote_prefix = "remotes/";
memset(&ref_list, 0, sizeof(ref_list));
ref_list.kinds = kinds;
@@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
clear_commit_marks(item->commit, ALL_REV_FLAGS);
}
clear_commit_marks(filter, ALL_REV_FLAGS);
-
- if (verbose)
- ref_list.maxwidth = calc_maxwidth(&ref_list);
}
+ if (verbose)
+ maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
detached = (detached && (kinds & REF_LOCAL_BRANCH));
if (detached && match_patterns(pattern, "HEAD"))
- show_detached(&ref_list);
+ show_detached(&ref_list, maxwidth);
for (i = 0; i < ref_list.index; i++) {
int current = !detached &&
(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
!strcmp(ref_list.list[i].name, head);
- char *prefix = (kinds != REF_REMOTE_BRANCH &&
- ref_list.list[i].kind == REF_REMOTE_BRANCH)
- ? "remotes/" : "";
- print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
- abbrev, current, prefix);
+ print_ref_item(&ref_list.list[i], maxwidth, verbose,
+ abbrev, current, remote_prefix);
}
free_ref_list(&ref_list);
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 05/11] branch: fix width computation
2015-07-28 6:56 ` [RFC/PATCH 05/11] branch: fix width computation Karthik Nayak
@ 2015-07-28 9:47 ` Matthieu Moy
2015-07-28 18:16 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 9:47 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
Why did send-email add this From: header? Strange, it has the same
content as your actual From: field.
> Remove unnecessary variables from ref_list and ref_item
> which were used for width computation. Make other changes
> accordingly. This patch is a precursor for porting branch.c
> to use ref-filter APIs.
You can explain better why this is needed. I guess something like "we're
making struct ref_item similar to ref-filter's ref_array_item", but the
reader shouldn't have to guess.
You should adujst the subject like BTW, I don't think you are "fixing"
anything.
On a side note: on the "tag" series, see how explaining better and
splitting patches led not only to a better history, but also to better
final code, and to finding a actual bugs (the %(color) thing and the
absence of reset on the state variable) even after sereval rounds of
review? I'm being picky and demanding, but don't see that as a complain
from me, just hints on getting even better ;-).
> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
> newitem->name = xstrdup(refname);
> newitem->kind = kind;
> newitem->commit = commit;
> - newitem->width = utf8_strwidth(refname);
> newitem->dest = resolve_symref(orig_refname, prefix);
> newitem->ignore = 0;
> - /* adjust for "remotes/" */
> - if (newitem->kind == REF_REMOTE_BRANCH &&
> - ref_list->kinds != REF_REMOTE_BRANCH)
> - newitem->width += 8;
> - if (newitem->width > ref_list->maxwidth)
> - ref_list->maxwidth = newitem->width;
>
> return 0;
> }
OK, in the old code, the width computation is done when inserting the
ref into the array. In the new code, you build the array and then do the
width computation. You can explain this better in the commit message I
think (instead of "Make other changes accordingly" which doesn't bring
much).
> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
> }
>
> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> - int abbrev, int current, char *prefix)
> + int abbrev, int current, const char *remote_prefix)
> {
> char c;
> int color;
> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> + const char *prefix = "";
>
> if (item->ignore)
> return;
> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> break;
> case REF_REMOTE_BRANCH:
> color = BRANCH_COLOR_REMOTE;
> + prefix = remote_prefix;
> break;
Why do you need these two hunks? I did not investigate in details, but
it seems you're calling print_ref_item either with prefix="" or with
prefix=remote_prefix so it would seem that keeping "prefix" as argument
would work. I guess I missed something.
> -static int calc_maxwidth(struct ref_list *refs)
> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
> {
> - int i, w = 0;
> + int i, max = 0;
> for (i = 0; i < refs->index; i++) {
> + struct ref_item *it = &refs->list[i];
> + int w = utf8_strwidth(it->name);
> +
> if (refs->list[i].ignore)
> continue;
> - if (refs->list[i].width > w)
> - w = refs->list[i].width;
> + if (it->kind == REF_REMOTE_BRANCH)
> + w += remote_bonus;
> + if (w > max)
> + max = w;
> }
> - return w;
> + return max;
> }
The old code was using 'w' for the max and no variable for the value at
the current iteration. You're using 'max' for the max and 'w' at the
current iteration. Using the same name 'w' for different things in the
pre- and post-image of the patch distracts the reader.
It may make sense to s/w/max/ in a separate preparatory patch. Or maybe
it's overkill.
> @@ -600,21 +600,18 @@ static char *get_head_description(void)
> return strbuf_detach(&desc, NULL);
> }
>
> -static void show_detached(struct ref_list *ref_list)
> +static void show_detached(struct ref_list *ref_list, int maxwidth)
> {
> struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>
> if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
> struct ref_item item;
> item.name = get_head_description();
> - item.width = utf8_strwidth(item.name);
> item.kind = REF_LOCAL_BRANCH;
> item.dest = NULL;
> item.commit = head_commit;
> item.ignore = 0;
> - if (item.width > ref_list->maxwidth)
> - ref_list->maxwidth = item.width;
> - print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
> + print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
> free(item.name);
> }
> }
...
> + int maxwidth = 0;
...
> + if (verbose)
> + maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>
> qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>
> detached = (detached && (kinds & REF_LOCAL_BRANCH));
> if (detached && match_patterns(pattern, "HEAD"))
> - show_detached(&ref_list);
> + show_detached(&ref_list, maxwidth);
This hunks could ideally go in a preparatory patch that would just move
the place where maxwidth is computed. This preparatory patch would just
say
maxwidth = ref_list->maxwidth;
and then you would do the actual change to
if (verbose)
maxwidth = calc_maxwidth(...)
without the distracting changes in the function's argument list.
I won't insist on that, again it may be overkill. But reading the patch,
I found it both rather trivial and hard to read, so there's room for
improvement.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 05/11] branch: fix width computation
2015-07-28 9:47 ` Matthieu Moy
@ 2015-07-28 18:16 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 18:16 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 3:17 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> Why did send-email add this From: header? Strange, it has the same
> content as your actual From: field.
>
Not sure why, everything else came out fine. Idk what happened here.
>> Remove unnecessary variables from ref_list and ref_item
>> which were used for width computation. Make other changes
>> accordingly. This patch is a precursor for porting branch.c
>> to use ref-filter APIs.
>
> You can explain better why this is needed. I guess something like "we're
> making struct ref_item similar to ref-filter's ref_array_item", but the
> reader shouldn't have to guess.
>
Will explain like you suggested.
> You should adujst the subject like BTW, I don't think you are "fixing"
> anything.
>
I guess refactor would be a better word here.
> On a side note: on the "tag" series, see how explaining better and
> splitting patches led not only to a better history, but also to better
> final code, and to finding a actual bugs (the %(color) thing and the
> absence of reset on the state variable) even after sereval rounds of
> review? I'm being picky and demanding, but don't see that as a complain
> from me, just hints on getting even better ;-).
>
Haha, I look forward to reviews, they show things I usually miss out on,
and help me get better. so keep them coming, I'll keep improving :)
Thanks
>> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>> newitem->name = xstrdup(refname);
>> newitem->kind = kind;
>> newitem->commit = commit;
>> - newitem->width = utf8_strwidth(refname);
>> newitem->dest = resolve_symref(orig_refname, prefix);
>> newitem->ignore = 0;
>> - /* adjust for "remotes/" */
>> - if (newitem->kind == REF_REMOTE_BRANCH &&
>> - ref_list->kinds != REF_REMOTE_BRANCH)
>> - newitem->width += 8;
>> - if (newitem->width > ref_list->maxwidth)
>> - ref_list->maxwidth = newitem->width;
>>
>> return 0;
>> }
>
> OK, in the old code, the width computation is done when inserting the
> ref into the array. In the new code, you build the array and then do the
> width computation. You can explain this better in the commit message I
> think (instead of "Make other changes accordingly" which doesn't bring
> much).
Okay I guess will do, just didn't want to explain the whole thing in the commit
message.
>
>> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>> }
>>
>> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> - int abbrev, int current, char *prefix)
>> + int abbrev, int current, const char *remote_prefix)
>> {
>> char c;
>> int color;
>> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>> + const char *prefix = "";
>>
>> if (item->ignore)
>> return;
>> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> break;
>> case REF_REMOTE_BRANCH:
>> color = BRANCH_COLOR_REMOTE;
>> + prefix = remote_prefix;
>> break;
>
> Why do you need these two hunks? I did not investigate in details, but
> it seems you're calling print_ref_item either with prefix="" or with
> prefix=remote_prefix so it would seem that keeping "prefix" as argument
> would work. I guess I missed something.
This is needed as whenever we do "git branch", show_detached() calls
print_ref_item()
with remote_prefix="". But print_ref_list() calls print_ref_item()
with remote_prefix="remotes"
(only when `git branch -a` is called remote_prefix=""). But only
remote branches should be
given the remotes prefix.
>
>> -static int calc_maxwidth(struct ref_list *refs)
>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>> {
>> - int i, w = 0;
>> + int i, max = 0;
>> for (i = 0; i < refs->index; i++) {
>> + struct ref_item *it = &refs->list[i];
>> + int w = utf8_strwidth(it->name);
>> +
>> if (refs->list[i].ignore)
>> continue;
>> - if (refs->list[i].width > w)
>> - w = refs->list[i].width;
>> + if (it->kind == REF_REMOTE_BRANCH)
>> + w += remote_bonus;
>> + if (w > max)
>> + max = w;
>> }
>> - return w;
>> + return max;
>> }
>
> The old code was using 'w' for the max and no variable for the value at
> the current iteration. You're using 'max' for the max and 'w' at the
> current iteration. Using the same name 'w' for different things in the
> pre- and post-image of the patch distracts the reader.
>
> It may make sense to s/w/max/ in a separate preparatory patch. Or maybe
> it's overkill.
>
Since the change was minimal and easily understandable I think it's ok.
But if you still feel otherwise, I'll be happy to introduce a preparatory patch.
>> @@ -600,21 +600,18 @@ static char *get_head_description(void)
>> return strbuf_detach(&desc, NULL);
>> }
>>
>> -static void show_detached(struct ref_list *ref_list)
>> +static void show_detached(struct ref_list *ref_list, int maxwidth)
>> {
>> struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>
>> if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>> struct ref_item item;
>> item.name = get_head_description();
>> - item.width = utf8_strwidth(item.name);
>> item.kind = REF_LOCAL_BRANCH;
>> item.dest = NULL;
>> item.commit = head_commit;
>> item.ignore = 0;
>> - if (item.width > ref_list->maxwidth)
>> - ref_list->maxwidth = item.width;
>> - print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>> + print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>> free(item.name);
>> }
>> }
> ...
>> + int maxwidth = 0;
> ...
>> + if (verbose)
>> + maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>> qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>>
>> detached = (detached && (kinds & REF_LOCAL_BRANCH));
>> if (detached && match_patterns(pattern, "HEAD"))
>> - show_detached(&ref_list);
>> + show_detached(&ref_list, maxwidth);
>
> This hunks could ideally go in a preparatory patch that would just move
> the place where maxwidth is computed. This preparatory patch would just
> say
>
> maxwidth = ref_list->maxwidth;
>
> and then you would do the actual change to
>
> if (verbose)
> maxwidth = calc_maxwidth(...)
>
> without the distracting changes in the function's argument list.
>
> I won't insist on that, again it may be overkill. But reading the patch,
> I found it both rather trivial and hard to read, so there's room for
> improvement.
>
I find it too small to make a preparatory patch again, but if you really feel
so, like I said, I'll change :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (3 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 05/11] branch: fix width computation Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 13:01 ` Matthieu Moy
2015-07-28 6:56 ` [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer Karthik Nayak
` (5 subsequent siblings)
10 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Remove show_detached() and make detached HEAD to be rolled into
regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
supporting the same in append_ref().
Bump get_head_description() to the top.
Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 122 ++++++++++++++++++++++++++++---------------------------
1 file changed, 63 insertions(+), 59 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index b058b74..f3d83d0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -30,6 +30,7 @@ static const char * const builtin_branch_usage[] = {
#define REF_LOCAL_BRANCH 0x01
#define REF_REMOTE_BRANCH 0x02
+#define REF_DETACHED_HEAD 0x04
static const char *head;
static unsigned char head_sha1[20];
@@ -352,8 +353,12 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
break;
}
}
- if (ARRAY_SIZE(ref_kind) <= i)
- return 0;
+ if (ARRAY_SIZE(ref_kind) <= i) {
+ if (!strcmp(refname, "HEAD"))
+ kind = REF_DETACHED_HEAD;
+ else
+ return 0;
+ }
/* Don't add types the caller doesn't want */
if ((kind & ref_list->kinds) == 0)
@@ -497,6 +502,37 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
strbuf_release(&subject);
}
+static char *get_head_description(void)
+{
+ struct strbuf desc = STRBUF_INIT;
+ struct wt_status_state state;
+ memset(&state, 0, sizeof(state));
+ wt_status_get_state(&state, 1);
+ if (state.rebase_in_progress ||
+ state.rebase_interactive_in_progress)
+ strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+ state.branch);
+ else if (state.bisect_in_progress)
+ strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
+ state.branch);
+ else if (state.detached_from) {
+ /* TRANSLATORS: make sure these match _("HEAD detached at ")
+ and _("HEAD detached from ") in wt-status.c */
+ if (state.detached_at)
+ strbuf_addf(&desc, _("(HEAD detached at %s)"),
+ state.detached_from);
+ else
+ strbuf_addf(&desc, _("(HEAD detached from %s)"),
+ state.detached_from);
+ }
+ else
+ strbuf_addstr(&desc, _("(no branch)"));
+ free(state.branch);
+ free(state.onto);
+ free(state.detached_from);
+ return strbuf_detach(&desc, NULL);
+}
+
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
int abbrev, int current, const char *remote_prefix)
{
@@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = "";
+ const char *desc = item->name;
if (item->ignore)
return;
@@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
color = BRANCH_COLOR_REMOTE;
prefix = remote_prefix;
break;
+ case REF_DETACHED_HEAD:
+ color = BRANCH_COLOR_CURRENT;
+ desc = get_head_description();
+ break;
default:
color = BRANCH_COLOR_PLAIN;
break;
@@ -527,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
color = BRANCH_COLOR_CURRENT;
}
- strbuf_addf(&name, "%s%s", prefix, item->name);
+ strbuf_addf(&name, "%s%s", prefix, desc);
if (verbose) {
int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
@@ -569,56 +610,9 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
return max;
}
-static char *get_head_description(void)
-{
- struct strbuf desc = STRBUF_INIT;
- struct wt_status_state state;
- memset(&state, 0, sizeof(state));
- wt_status_get_state(&state, 1);
- if (state.rebase_in_progress ||
- state.rebase_interactive_in_progress)
- strbuf_addf(&desc, _("(no branch, rebasing %s)"),
- state.branch);
- else if (state.bisect_in_progress)
- strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
- state.branch);
- else if (state.detached_from) {
- /* TRANSLATORS: make sure these match _("HEAD detached at ")
- and _("HEAD detached from ") in wt-status.c */
- if (state.detached_at)
- strbuf_addf(&desc, _("(HEAD detached at %s)"),
- state.detached_from);
- else
- strbuf_addf(&desc, _("(HEAD detached from %s)"),
- state.detached_from);
- }
- else
- strbuf_addstr(&desc, _("(no branch)"));
- free(state.branch);
- free(state.onto);
- free(state.detached_from);
- return strbuf_detach(&desc, NULL);
-}
-
-static void show_detached(struct ref_list *ref_list, int maxwidth)
-{
- struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
-
- if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
- struct ref_item item;
- item.name = get_head_description();
- item.kind = REF_LOCAL_BRANCH;
- item.dest = NULL;
- item.commit = head_commit;
- item.ignore = 0;
- print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
- free(item.name);
- }
-}
-
static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
{
- int i;
+ int i, index;
struct append_ref_cb cb;
struct ref_list ref_list;
int maxwidth = 0;
@@ -643,6 +637,8 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
cb.pattern = pattern;
cb.ret = 0;
for_each_rawref(append_ref, &cb);
+ if (detached)
+ head_ref(append_ref, &cb);
/*
* The following implementation is currently duplicated in ref-filter. It
* will eventually be removed when we port branch.c to use ref-filter APIs.
@@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
if (verbose)
maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
- qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
+ index = ref_list.index;
+
+ /* Print detached heads before sorting and printing the rest */
+ if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
+ !strcmp(ref_list.list[index - 1].name, head)) {
+ print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
+ 1, remote_prefix);
+ index -= 1;
+ }
- detached = (detached && (kinds & REF_LOCAL_BRANCH));
- if (detached && match_patterns(pattern, "HEAD"))
- show_detached(&ref_list, maxwidth);
+ qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
- for (i = 0; i < ref_list.index; i++) {
- int current = !detached &&
- (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
+ for (i = 0; i < index; i++) {
+ int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
!strcmp(ref_list.list[i].name, head);
print_ref_item(&ref_list.list[i], maxwidth, verbose,
abbrev, current, remote_prefix);
@@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, kinds, quiet);
} else if (list) {
- int ret = print_ref_list(kinds, detached, verbose, abbrev,
+ int ret;
+ if (kinds & REF_LOCAL_BRANCH)
+ kinds |= REF_DETACHED_HEAD;
+ ret = print_ref_list(kinds, detached, verbose, abbrev,
with_commit, argv);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list
2015-07-28 6:56 ` [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list Karthik Nayak
@ 2015-07-28 13:01 ` Matthieu Moy
2015-07-28 19:19 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 13:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> Remove show_detached() and make detached HEAD to be rolled into
> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
> supporting the same in append_ref().
Again, this lacks the "why?" explanation.
> Bump get_head_description() to the top.
Here also: why? And this could easily go to a separate patch to let the
reviewer focus on actual changes.
> @@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> int color;
> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> const char *prefix = "";
> + const char *desc = item->name;
>
> if (item->ignore)
> return;
> @@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> color = BRANCH_COLOR_REMOTE;
> prefix = remote_prefix;
> break;
> + case REF_DETACHED_HEAD:
> + color = BRANCH_COLOR_CURRENT;
> + desc = get_head_description();
I think you're leaking a string here: get_head_description() builds an
strbuf and returns the dynamically allocated string, which you never
free.
> -static void show_detached(struct ref_list *ref_list, int maxwidth)
> -{
> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
> -
> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
I'm not sure what this if was doing, and why you can get rid of it. My
understanding is that with_commit comes from --contains, and in the
previous code the filtering was done at display time (detached HEAD was
not shown if it was not contained in commits specified with --contains).
Eventually, you'll use ref-filter to do this filtering so you won't need
this check at display time.
But am I correct that for a few commits, you ignore --contains on
detached HEAD?
> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
> if (verbose)
> maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>
> - qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
> + index = ref_list.index;
> +
> + /* Print detached heads before sorting and printing the rest */
I think you mean head (no s) or HEAD. It's not Mercurial ;-).
> + if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
> + !strcmp(ref_list.list[index - 1].name, head)) {
> + print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
> + 1, remote_prefix);
> + index -= 1;
> + }
This relies on the assumption that HEAD has to be the last in the array.
The assumption is correct since you call head_ref(append_ref, &cb) after
for_each_rawref(append_ref, &cb). I think this deserves a comment to
remind the reader that HEAD is always the last (here or at the call to
head_ref to make sure nobody try to change the order between head_ref
and for_each_rawref).
It may be more natural to do it the other way around: call head_ref
first and get HEAD first, as you are going to display it first (but in
any case, you'll have to call qsort on a subset of the array so it
doesn't change much).
> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> die(_("branch name required"));
> return delete_branches(argc, argv, delete > 1, kinds, quiet);
> } else if (list) {
> - int ret = print_ref_list(kinds, detached, verbose, abbrev,
> + int ret;
> + if (kinds & REF_LOCAL_BRANCH)
> + kinds |= REF_DETACHED_HEAD;
Perhaps add
/* git branch --local also shows HEAD when it is detached */
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list
2015-07-28 13:01 ` Matthieu Moy
@ 2015-07-28 19:19 ` Karthik Nayak
2015-07-29 9:56 ` Matthieu Moy
0 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 19:19 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Remove show_detached() and make detached HEAD to be rolled into
>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
>> supporting the same in append_ref().
>
> Again, this lacks the "why?" explanation.
Will include a small explanation.
>
>> Bump get_head_description() to the top.
>
> Here also: why? And this could easily go to a separate patch to let the
> reviewer focus on actual changes.
I'll move this to a preparatory patch.
>
> I think you're leaking a string here: get_head_description() builds an
> strbuf and returns the dynamically allocated string, which you never
> free.
>
Ah! good catch, will fix that.
>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>> -{
>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>> -
>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>
> I'm not sure what this if was doing, and why you can get rid of it. My
> understanding is that with_commit comes from --contains, and in the
> previous code the filtering was done at display time (detached HEAD was
> not shown if it was not contained in commits specified with --contains).
>
> Eventually, you'll use ref-filter to do this filtering so you won't need
> this check at display time.
>
> But am I correct that for a few commits, you ignore --contains on
> detached HEAD?
>
No we don't ignore --contains on detached HEAD.
Since detached HEAD now gets its data from append_ref(). The function
also checks for the --contains option.
>> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>> if (verbose)
>> maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>> - qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>> + index = ref_list.index;
>> +
>> + /* Print detached heads before sorting and printing the rest */
>
> I think you mean head (no s) or HEAD. It's not Mercurial ;-).
>
I meant HEAD, lol.
>> + if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
>> + !strcmp(ref_list.list[index - 1].name, head)) {
>> + print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
>> + 1, remote_prefix);
>> + index -= 1;
>> + }
>
> This relies on the assumption that HEAD has to be the last in the array.
> The assumption is correct since you call head_ref(append_ref, &cb) after
> for_each_rawref(append_ref, &cb). I think this deserves a comment to
> remind the reader that HEAD is always the last (here or at the call to
> head_ref to make sure nobody try to change the order between head_ref
> and for_each_rawref).
>
Yeah! makes sense, will add at the comment at the call to head_ref().
> It may be more natural to do it the other way around: call head_ref
> first and get HEAD first, as you are going to display it first (but in
> any case, you'll have to call qsort on a subset of the array so it
> doesn't change much).
That sorta messes things up with qsort, this keeps it clean I feel.
>
>> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>> die(_("branch name required"));
>> return delete_branches(argc, argv, delete > 1, kinds, quiet);
>> } else if (list) {
>> - int ret = print_ref_list(kinds, detached, verbose, abbrev,
>> + int ret;
>> + if (kinds & REF_LOCAL_BRANCH)
>> + kinds |= REF_DETACHED_HEAD;
>
> Perhaps add
>
> /* git branch --local also shows HEAD when it is detached */
>
Yeah definitely!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list
2015-07-28 19:19 ` Karthik Nayak
@ 2015-07-29 9:56 ` Matthieu Moy
2015-07-29 17:54 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 9:56 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>>> -{
>>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>> -
>>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>
>> I'm not sure what this if was doing, and why you can get rid of it. My
>> understanding is that with_commit comes from --contains, and in the
>> previous code the filtering was done at display time (detached HEAD was
>> not shown if it was not contained in commits specified with --contains).
>>
>> Eventually, you'll use ref-filter to do this filtering so you won't need
>> this check at display time.
>>
>> But am I correct that for a few commits, you ignore --contains on
>> detached HEAD?
>>
>
> No we don't ignore --contains on detached HEAD.
>
> Since detached HEAD now gets its data from append_ref(). The function
> also checks for the --contains option.
Ah, OK. Previously, detached HEAD and branches were completely
different, each having its own if (is_descendant_of(...)), and you're
now using only one in append_ref() before removing it completely in
favor of ref-filter.
That would deserve an explanation for other reviewers I think.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list
2015-07-29 9:56 ` Matthieu Moy
@ 2015-07-29 17:54 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 17:54 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Wed, Jul 29, 2015 at 3:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>
>>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>>>> -{
>>>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>>> -
>>>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>>
>>> I'm not sure what this if was doing, and why you can get rid of it. My
>>> understanding is that with_commit comes from --contains, and in the
>>> previous code the filtering was done at display time (detached HEAD was
>>> not shown if it was not contained in commits specified with --contains).
>>>
>>> Eventually, you'll use ref-filter to do this filtering so you won't need
>>> this check at display time.
>>>
>>> But am I correct that for a few commits, you ignore --contains on
>>> detached HEAD?
>>>
>>
>> No we don't ignore --contains on detached HEAD.
>>
>> Since detached HEAD now gets its data from append_ref(). The function
>> also checks for the --contains option.
>
> Ah, OK. Previously, detached HEAD and branches were completely
> different, each having its own if (is_descendant_of(...)), and you're
> now using only one in append_ref() before removing it completely in
> favor of ref-filter.
>
> That would deserve an explanation for other reviewers I think.
>
Will include a small explanation in the commit message :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (4 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 13:09 ` Matthieu Moy
2015-07-28 6:56 ` [RFC/PATCH 08/11] branch: drop non-commit error reporting Karthik Nayak
` (4 subsequent siblings)
10 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
We check if given ref is the current branch in print_ref_list(). Move
this check to print_ref_item() where it is checked right before
printing.
Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index f3d83d0..ba9cbad 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,9 @@ static char *get_head_description(void)
}
static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
- int abbrev, int current, const char *remote_prefix)
+ int abbrev, int detached, const char *remote_prefix)
{
- char c;
+ char c = ' ';
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = "";
@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
switch (item->kind) {
case REF_LOCAL_BRANCH:
- color = BRANCH_COLOR_LOCAL;
+ if (!detached && !strcmp(item->name, head)) {
+ color = BRANCH_COLOR_CURRENT;
+ c = '*';
+ } else
+ color = BRANCH_COLOR_LOCAL;
break;
case REF_REMOTE_BRANCH:
color = BRANCH_COLOR_REMOTE;
@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
case REF_DETACHED_HEAD:
color = BRANCH_COLOR_CURRENT;
desc = get_head_description();
+ c = '*';
break;
default:
color = BRANCH_COLOR_PLAIN;
break;
}
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
-
strbuf_addf(&name, "%s%s", prefix, desc);
if (verbose) {
int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
@@ -677,21 +676,17 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
index = ref_list.index;
/* Print detached heads before sorting and printing the rest */
- if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
- !strcmp(ref_list.list[index - 1].name, head)) {
+ if (detached) {
print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
- 1, remote_prefix);
+ detached, remote_prefix);
index -= 1;
}
qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
- for (i = 0; i < index; i++) {
- int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
- !strcmp(ref_list.list[i].name, head);
+ for (i = 0; i < index; i++)
print_ref_item(&ref_list.list[i], maxwidth, verbose,
- abbrev, current, remote_prefix);
- }
+ abbrev, detached, remote_prefix);
free_ref_list(&ref_list);
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-28 6:56 ` [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer Karthik Nayak
@ 2015-07-28 13:09 ` Matthieu Moy
2015-07-28 20:12 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 13:09 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> We check if given ref is the current branch in print_ref_list(). Move
> this check to print_ref_item() where it is checked right before
> printing.
This means that the '*' and the different color are coded in C, hence
it's not possible to mimick this using "git for-each-ref --format ...".
I do not consider this as blocking, but I think the ultimate goal should
be to allow this, so that all the goodies of "git branch" can be made
available to other ref-listing commands.
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -534,9 +534,9 @@ static char *get_head_description(void)
> }
>
> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> - int abbrev, int current, const char *remote_prefix)
> + int abbrev, int detached, const char *remote_prefix)
> {
> - char c;
> + char c = ' ';
> int color;
> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> const char *prefix = "";
> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>
> switch (item->kind) {
> case REF_LOCAL_BRANCH:
> - color = BRANCH_COLOR_LOCAL;
> + if (!detached && !strcmp(item->name, head)) {
> + color = BRANCH_COLOR_CURRENT;
> + c = '*';
> + } else
> + color = BRANCH_COLOR_LOCAL;
> break;
> case REF_REMOTE_BRANCH:
> color = BRANCH_COLOR_REMOTE;
> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> case REF_DETACHED_HEAD:
> color = BRANCH_COLOR_CURRENT;
> desc = get_head_description();
> + c = '*';
> break;
> default:
> color = BRANCH_COLOR_PLAIN;
> break;
> }
>
> - c = ' ';
> - if (current) {
> - c = '*';
> - color = BRANCH_COLOR_CURRENT;
> - }
I actually prefered the old way: current is a Boolean that says
semantically "this is the current branch", and the Boolean is turned
into presentation directives in a separate piece of code.
I'd write
char c;
int current = 0;
...
if (...)
current = 1;
...
case REF_DETACHED_HEAD:
current = 1;
...
and keep the last hunk.
(IOW, turn current from a parameter into a local variable)
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-28 13:09 ` Matthieu Moy
@ 2015-07-28 20:12 ` Karthik Nayak
2015-07-29 0:46 ` Jacob Keller
2015-07-29 10:01 ` Matthieu Moy
0 siblings, 2 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 20:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We check if given ref is the current branch in print_ref_list(). Move
>> this check to print_ref_item() where it is checked right before
>> printing.
>
> This means that the '*' and the different color are coded in C, hence
> it's not possible to mimick this using "git for-each-ref --format ...".
>
> I do not consider this as blocking, but I think the ultimate goal should
> be to allow this, so that all the goodies of "git branch" can be made
> available to other ref-listing commands.
>
Not sure what you mean here.
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -534,9 +534,9 @@ static char *get_head_description(void)
>> }
>>
>> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> - int abbrev, int current, const char *remote_prefix)
>> + int abbrev, int detached, const char *remote_prefix)
>> {
>> - char c;
>> + char c = ' ';
>> int color;
>> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>> const char *prefix = "";
>> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>
>> switch (item->kind) {
>> case REF_LOCAL_BRANCH:
>> - color = BRANCH_COLOR_LOCAL;
>> + if (!detached && !strcmp(item->name, head)) {
>> + color = BRANCH_COLOR_CURRENT;
>> + c = '*';
>> + } else
>> + color = BRANCH_COLOR_LOCAL;
>> break;
>> case REF_REMOTE_BRANCH:
>> color = BRANCH_COLOR_REMOTE;
>> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> case REF_DETACHED_HEAD:
>> color = BRANCH_COLOR_CURRENT;
>> desc = get_head_description();
>> + c = '*';
>> break;
>> default:
>> color = BRANCH_COLOR_PLAIN;
>> break;
>> }
>>
>> - c = ' ';
>> - if (current) {
>> - c = '*';
>> - color = BRANCH_COLOR_CURRENT;
>> - }
>
> I actually prefered the old way: current is a Boolean that says
> semantically "this is the current branch", and the Boolean is turned
> into presentation directives in a separate piece of code.
>
> I'd write
>
> char c;
> int current = 0;
>
> ...
>
> if (...)
> current = 1;
> ...
> case REF_DETACHED_HEAD:
> current = 1;
> ...
>
> and keep the last hunk.
>
> (IOW, turn current from a parameter into a local variable)
>
Thanks will do this.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-28 20:12 ` Karthik Nayak
@ 2015-07-29 0:46 ` Jacob Keller
2015-07-29 18:44 ` Karthik Nayak
2015-07-29 10:01 ` Matthieu Moy
1 sibling, 1 reply; 88+ messages in thread
From: Jacob Keller @ 2015-07-29 0:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> We check if given ref is the current branch in print_ref_list(). Move
>>> this check to print_ref_item() where it is checked right before
>>> printing.
>>
>> This means that the '*' and the different color are coded in C, hence
>> it's not possible to mimick this using "git for-each-ref --format ...".
>>
>> I do not consider this as blocking, but I think the ultimate goal should
>> be to allow this, so that all the goodies of "git branch" can be made
>> available to other ref-listing commands.
>>
>
> Not sure what you mean here.
>
He means to make sure that any feature that was in tag, branch,
for-each-ref, etc should be available as part of ref-filter and in all
of them
Maybe he misunderstood code or maybe you misunderstood the comment here?
Regards,
Jake
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-29 0:46 ` Jacob Keller
@ 2015-07-29 18:44 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 18:44 UTC (permalink / raw)
To: Jacob Keller; +Cc: Matthieu Moy, Git, Christian Couder, Junio C Hamano
On Wed, Jul 29, 2015 at 6:16 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> We check if given ref is the current branch in print_ref_list(). Move
>>>> this check to print_ref_item() where it is checked right before
>>>> printing.
>>>
>>> This means that the '*' and the different color are coded in C, hence
>>> it's not possible to mimick this using "git for-each-ref --format ...".
>>>
>>> I do not consider this as blocking, but I think the ultimate goal should
>>> be to allow this, so that all the goodies of "git branch" can be made
>>> available to other ref-listing commands.
>>>
>>
>> Not sure what you mean here.
>>
>
> He means to make sure that any feature that was in tag, branch,
> for-each-ref, etc should be available as part of ref-filter and in all
> of them
>
> Maybe he misunderstood code or maybe you misunderstood the comment here?
>
> Regards,
> Jake
Ah, I didn't quite understand the first time. Thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-28 20:12 ` Karthik Nayak
2015-07-29 0:46 ` Jacob Keller
@ 2015-07-29 10:01 ` Matthieu Moy
2015-07-29 18:52 ` Karthik Nayak
1 sibling, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 10:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> We check if given ref is the current branch in print_ref_list(). Move
>>> this check to print_ref_item() where it is checked right before
>>> printing.
>>
>> This means that the '*' and the different color are coded in C, hence
>> it's not possible to mimick this using "git for-each-ref --format ...".
>>
>> I do not consider this as blocking, but I think the ultimate goal should
>> be to allow this, so that all the goodies of "git branch" can be made
>> available to other ref-listing commands.
>>
>
> Not sure what you mean here.
What you already know, but probably badly explained ;-).
Eventually, the output of "git branch" should correspond to a format
string (so git branch would be almost an alias for "git for-each-ref
refs/heads/ --format '...'"). Internally, this would mean using
show_ref_array_item instead of print_ref_item. This is what you managed
to do for "git tag".
You already identified one difficulty with sha1 alignment in "git branch
-v". I'm pointing out another which is that displaying the "*" in front
of the current branch is currently not possible with a format string.
You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)
(for which you'd need to find a short-and-sweet name ;-) ).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-29 10:01 ` Matthieu Moy
@ 2015-07-29 18:52 ` Karthik Nayak
2015-07-29 21:27 ` Matthieu Moy
0 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 18:52 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Wed, Jul 29, 2015 at 3:31 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> We check if given ref is the current branch in print_ref_list(). Move
>>>> this check to print_ref_item() where it is checked right before
>>>> printing.
>>>
>>> This means that the '*' and the different color are coded in C, hence
>>> it's not possible to mimick this using "git for-each-ref --format ...".
>>>
>>> I do not consider this as blocking, but I think the ultimate goal should
>>> be to allow this, so that all the goodies of "git branch" can be made
>>> available to other ref-listing commands.
>>>
>>
>> Not sure what you mean here.
>
> What you already know, but probably badly explained ;-).
>
> Eventually, the output of "git branch" should correspond to a format
> string (so git branch would be almost an alias for "git for-each-ref
> refs/heads/ --format '...'"). Internally, this would mean using
> show_ref_array_item instead of print_ref_item. This is what you managed
> to do for "git tag".
>
> You already identified one difficulty with sha1 alignment in "git branch
> -v". I'm pointing out another which is that displaying the "*" in front
> of the current branch is currently not possible with a format string.
> You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)
> (for which you'd need to find a short-and-sweet name ;-) ).
>
What I was thinking of was something like this :
struct strbuf format = STRBUF_INIT;
char c = ' ';
if (current)
c = '*';
strbuf_addf(&format, "%c....", c, other format options...);
show_ref_array_item(item, format.buf, quote_style, 0);
strbuf_release(&format);
This would remove the need of making the printing of the "*" to be
needed by ref-filter. As this is only needed in branch.c
If going on what you're saying we could have a "%(starifcurrent)" atom or
something, but I don't see a general use for this.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-29 18:52 ` Karthik Nayak
@ 2015-07-29 21:27 ` Matthieu Moy
2015-08-01 6:48 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 21:27 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> What I was thinking of was something like this :
>
> struct strbuf format = STRBUF_INIT;
> char c = ' ';
> if (current)
> c = '*';
> strbuf_addf(&format, "%c....", c, other format options...);
> show_ref_array_item(item, format.buf, quote_style, 0);
> strbuf_release(&format);
I think that would interact badly with verify_ref_format(). Usually, you
have just one format string and call verify_ref_format() on it, not a
different format string for each ref_array_item. That would probably be
solvable.
> This would remove the need of making the printing of the "*" to be
> needed by ref-filter. As this is only needed in branch.c
>
> If going on what you're saying we could have a "%(starifcurrent)" atom or
> something, but I don't see a general use for this.
To have a really customizeable format, you would want to allow e.g.
git branch --format '%(starifcurrent) %(objectname) %(refname)'
if the user wants to get the sha1 before the refname, and still have the
star in front. It's a bit frustrating to have a hardcoded format that
the user can't reproduce with the --format option, since it means you
can't easily make a small variation on it.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-07-29 21:27 ` Matthieu Moy
@ 2015-08-01 6:48 ` Karthik Nayak
2015-08-01 7:06 ` Jacob Keller
2015-08-01 9:03 ` Eric Sunshine
0 siblings, 2 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-08-01 6:48 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> What I was thinking of was something like this :
>>
>> struct strbuf format = STRBUF_INIT;
>> char c = ' ';
>> if (current)
>> c = '*';
>> strbuf_addf(&format, "%c....", c, other format options...);
>> show_ref_array_item(item, format.buf, quote_style, 0);
>> strbuf_release(&format);
>
> I think that would interact badly with verify_ref_format(). Usually, you
> have just one format string and call verify_ref_format() on it, not a
> different format string for each ref_array_item. That would probably be
> solvable.
>
Good point! I just was wondering if we need another atom just to print a star.
But your points certainly are valid. I'll implement this. Thanks :)
>> This would remove the need of making the printing of the "*" to be
>> needed by ref-filter. As this is only needed in branch.c
>>
>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>> something, but I don't see a general use for this.
>
> To have a really customizeable format, you would want to allow e.g.
>
> git branch --format '%(starifcurrent) %(objectname) %(refname)'
>
> if the user wants to get the sha1 before the refname, and still have the
> star in front. It's a bit frustrating to have a hardcoded format that
> the user can't reproduce with the --format option, since it means you
> can't easily make a small variation on it.
>
Agreed. will have a "starifcurrent" atom :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-08-01 6:48 ` Karthik Nayak
@ 2015-08-01 7:06 ` Jacob Keller
2015-08-01 9:03 ` Eric Sunshine
1 sibling, 0 replies; 88+ messages in thread
From: Jacob Keller @ 2015-08-01 7:06 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder, Junio C Hamano
On Fri, Jul 31, 2015 at 11:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> What I was thinking of was something like this :
>>>
>>> struct strbuf format = STRBUF_INIT;
>>> char c = ' ';
>>> if (current)
>>> c = '*';
>>> strbuf_addf(&format, "%c....", c, other format options...);
>>> show_ref_array_item(item, format.buf, quote_style, 0);
>>> strbuf_release(&format);
>>
>> I think that would interact badly with verify_ref_format(). Usually, you
>> have just one format string and call verify_ref_format() on it, not a
>> different format string for each ref_array_item. That would probably be
>> solvable.
>>
>
> Good point! I just was wondering if we need another atom just to print a star.
> But your points certainly are valid. I'll implement this. Thanks :)
>
>>> This would remove the need of making the printing of the "*" to be
>>> needed by ref-filter. As this is only needed in branch.c
>>>
>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>> something, but I don't see a general use for this.
>>
>> To have a really customizeable format, you would want to allow e.g.
>>
>> git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>
>> if the user wants to get the sha1 before the refname, and still have the
>> star in front. It's a bit frustrating to have a hardcoded format that
>> the user can't reproduce with the --format option, since it means you
>> can't easily make a small variation on it.
>>
>
> Agreed. will have a "starifcurrent" atom :)
>
> --
Wonder if there is some sort of more "generic" atom that would work? I
can't think of anything obvious at all though... it may be worth
having this even though it definitely seems less useful generically,
as the reason above where --format should be at least as expressive as
the builtin formatting.
Regards,
Jake
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-08-01 6:48 ` Karthik Nayak
2015-08-01 7:06 ` Jacob Keller
@ 2015-08-01 9:03 ` Eric Sunshine
2015-08-02 12:59 ` Karthik Nayak
1 sibling, 1 reply; 88+ messages in thread
From: Eric Sunshine @ 2015-08-01 9:03 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder, Junio C Hamano
On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
> Good point! I just was wondering if we need another atom just to print a star.
> But your points certainly are valid. I'll implement this. Thanks :)
>
>>> This would remove the need of making the printing of the "*" to be
>>> needed by ref-filter. As this is only needed in branch.c
>>>
>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>> something, but I don't see a general use for this.
>>
>> To have a really customizeable format, you would want to allow e.g.
>>
>> git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>
>> if the user wants to get the sha1 before the refname, and still have the
>> star in front. It's a bit frustrating to have a hardcoded format that
>> the user can't reproduce with the --format option, since it means you
>> can't easily make a small variation on it.
>
> Agreed. will have a "starifcurrent" atom :)
Please don't. It's better to avoid these highly specialized solutions
and instead seek general purpose ones. For instance, utilizing the
%(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)
which would allow a format like this:
%(if:iscurrentbranch)*%(endif)
Even more generic would be an %(ifeq:x:y) conditional and a
%(currentbranch) atom:
%(ifeq:refname:currentbranch)*%(endif)
Those are just a couple ideas. Other variations are possible and
likely preferable to the specialized %(starifcurrent).
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-08-01 9:03 ` Eric Sunshine
@ 2015-08-02 12:59 ` Karthik Nayak
2015-08-02 17:58 ` Junio C Hamano
0 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-08-02 12:59 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Matthieu Moy, Git, Christian Couder, Junio C Hamano
On Sat, Aug 1, 2015 at 2:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Good point! I just was wondering if we need another atom just to print a star.
>> But your points certainly are valid. I'll implement this. Thanks :)
>>
>>>> This would remove the need of making the printing of the "*" to be
>>>> needed by ref-filter. As this is only needed in branch.c
>>>>
>>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>>> something, but I don't see a general use for this.
>>>
>>> To have a really customizeable format, you would want to allow e.g.
>>>
>>> git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>>
>>> if the user wants to get the sha1 before the refname, and still have the
>>> star in front. It's a bit frustrating to have a hardcoded format that
>>> the user can't reproduce with the --format option, since it means you
>>> can't easily make a small variation on it.
>>
>> Agreed. will have a "starifcurrent" atom :)
>
> Please don't. It's better to avoid these highly specialized solutions
> and instead seek general purpose ones. For instance, utilizing the
> %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)
> which would allow a format like this:
>
> %(if:iscurrentbranch)*%(endif)
>
> Even more generic would be an %(ifeq:x:y) conditional and a
> %(currentbranch) atom:
>
> %(ifeq:refname:currentbranch)*%(endif)
>
> Those are just a couple ideas. Other variations are possible and
> likely preferable to the specialized %(starifcurrent).
This makes sense, thanks. But implementing something like
"%(if:<atom>)" seems to not be as easy as I thought it would be.
First we need to parse that inner atom, but the used_atom_cnt is based
on how many atoms there are initially, which doesn't count this inner atom.
Although we could have a way around that, we'd need to again call populate_value
from itself to get that inner atom's value. This causes more problems.
Either ways
I'm looking at ways around this.
A simple solution would be to do :
%(if)%(atom)%(then).....%(endif)
or just
%(if)%(atom).....%(endif)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
2015-08-02 12:59 ` Karthik Nayak
@ 2015-08-02 17:58 ` Junio C Hamano
0 siblings, 0 replies; 88+ messages in thread
From: Junio C Hamano @ 2015-08-02 17:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Eric Sunshine, Matthieu Moy, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
>> Even more generic would be an %(ifeq:x:y) conditional and a
>> %(currentbranch) atom:
>>
>> %(ifeq:refname:currentbranch)*%(endif)
>>
>> Those are just a couple ideas. Other variations are possible and
>> likely preferable to the specialized %(starifcurrent).
>
> This makes sense, thanks. But implementing something like
> "%(if:<atom>)" seems to not be as easy as I thought it would be.
>
> First we need to parse that inner atom, but the used_atom_cnt is based
> on how many atoms there are initially, which doesn't count this inner atom.
>
> Although we could have a way around that, we'd need to again call populate_value
> from itself to get that inner atom's value. This causes more problems.
> Either ways
> I'm looking at ways around this.
> A simple solution would be to do :
>
> %(if)%(atom)%(then).....%(endif)
>
> or just
>
> %(if)%(atom).....%(endif)
My knee-jerk reaction to the former was "Eeww, the users is forced
to keep verbosely typing unnecessarily '%(', ')%(then)' forever,
only because the implementor was too lazy to do the job properly in
parse_atom()". I do not think the latter a good idea at all.
But I think the former is worth considering, as it will later allow
us to extend it to various forms, e.g.
%(if:notempty)%(atom)%(then)...%(end)
%(if)%(atom)%(then)...%(elif)%(atom1)%(atom2)%(then)...%(end)
Two points being
(1) the default "string is not empty" does not have to be the only
test criteria, by leaving the door to add %(if:<condition>)
later;
(2) what is tested does not have to be a single atom (that is why
I do not think the one without %(then) good at all) but can be
a string that can later be interpreted.
And syntactically, something like this even may want to be
introduced later:
%(if:expr)master == %(refname:short)%(then)...%(end)
^ permalink raw reply [flat|nested] 88+ messages in thread
* [RFC/PATCH 08/11] branch: drop non-commit error reporting
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (5 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 6:56 ` [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures Karthik Nayak
` (3 subsequent siblings)
10 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Based-on-patch-by: Jeff King <peff@peff.net>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index ba9cbad..8d0521e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char *prefix)
struct append_ref_cb {
struct ref_list *ref_list;
const char **pattern;
- int ret;
};
static int match_patterns(const char **pattern, const char *refname)
@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
commit = NULL;
if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
commit = lookup_commit_reference_gently(oid->hash, 1);
- if (!commit) {
- cb->ret = error(_("branch '%s' does not point at a commit"), refname);
+ if (!commit)
return 0;
- }
/* Filter with with_commit if specified */
if (!is_descendant_of(commit, ref_list->with_commit))
@@ -609,7 +606,7 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
return max;
}
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
{
int i, index;
struct append_ref_cb cb;
@@ -634,7 +631,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
init_revisions(&ref_list.revs, NULL);
cb.ref_list = &ref_list;
cb.pattern = pattern;
- cb.ret = 0;
for_each_rawref(append_ref, &cb);
if (detached)
head_ref(append_ref, &cb);
@@ -689,11 +685,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
abbrev, detached, remote_prefix);
free_ref_list(&ref_list);
-
- if (cb.ret)
- error(_("some refs could not be read"));
-
- return cb.ret;
}
static void rename_branch(const char *oldname, const char *newname, int force)
@@ -909,14 +900,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die(_("branch name required"));
return delete_branches(argc, argv, delete > 1, kinds, quiet);
} else if (list) {
- int ret;
if (kinds & REF_LOCAL_BRANCH)
kinds |= REF_DETACHED_HEAD;
- ret = print_ref_list(kinds, detached, verbose, abbrev,
+ print_ref_list(kinds, detached, verbose, abbrev,
with_commit, argv);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
- return ret;
+ return 0;
}
else if (edit_description) {
const char *branch_name;
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (6 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 08/11] branch: drop non-commit error reporting Karthik Nayak
@ 2015-07-28 6:56 ` Karthik Nayak
2015-07-28 8:17 ` Christian Couder
2015-07-28 8:22 ` Christian Couder
2015-07-28 8:42 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Matthieu Moy
` (2 subsequent siblings)
10 siblings, 2 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 6:56 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Make 'branch.c' use 'ref-filter' data structures and make changes to
support the new data structures. This is a part of the process of
porting 'branch.c' to use 'ref-filter' APIs.
This is a temporary step before porting 'branch.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code introduced
here will be removed when 'branch.c' is ported over to use
'ref-filter' APIs
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 289 +++++++++++++++++++++----------------------------------
ref-filter.h | 7 +-
2 files changed, 116 insertions(+), 180 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 8d0521e..df74527 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -19,6 +19,7 @@
#include "column.h"
#include "utf8.h"
#include "wt-status.h"
+#include "ref-filter.h"
static const char * const builtin_branch_usage[] = {
N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -28,10 +29,6 @@ static const char * const builtin_branch_usage[] = {
NULL
};
-#define REF_LOCAL_BRANCH 0x01
-#define REF_REMOTE_BRANCH 0x02
-#define REF_DETACHED_HEAD 0x04
-
static const char *head;
static unsigned char head_sha1[20];
@@ -53,13 +50,6 @@ enum color_branch {
BRANCH_COLOR_UPSTREAM = 5
};
-static enum merge_filter {
- NO_FILTER = 0,
- SHOW_NOT_MERGED,
- SHOW_MERGED
-} merge_filter;
-static unsigned char merge_filter_ref[20];
-
static struct string_list output = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -280,22 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
return(ret);
}
-struct ref_item {
- char *name;
- char *dest;
- unsigned int kind;
- struct commit *commit;
- int ignore;
-};
-
-struct ref_list {
- struct rev_info revs;
- int index, alloc, verbose, abbrev;
- struct ref_item *list;
- struct commit_list *with_commit;
- int kinds;
-};
-
static char *resolve_symref(const char *src, const char *prefix)
{
unsigned char sha1[20];
@@ -310,11 +284,6 @@ static char *resolve_symref(const char *src, const char *prefix)
return xstrdup(dst);
}
-struct append_ref_cb {
- struct ref_list *ref_list;
- const char **pattern;
-};
-
static int match_patterns(const char **pattern, const char *refname)
{
if (!*pattern)
@@ -327,11 +296,22 @@ static int match_patterns(const char **pattern, const char *refname)
return 0;
}
+static void ref_array_append(struct ref_array *array, const char *refname)
+{
+ size_t len = strlen(refname);
+ struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
+ memcpy(ref->refname, refname, len);
+ ref->refname[len] = '\0';
+ REALLOC_ARRAY(array->items, array->nr + 1);
+ array->items[array->nr++] = ref;
+}
+
static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)
{
- struct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);
- struct ref_list *ref_list = cb->ref_list;
- struct ref_item *newitem;
+ struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
+ struct ref_filter *filter = cb->filter;
+ struct ref_array *array = cb->array;
+ struct ref_array_item *item;
struct commit *commit;
int kind, i;
const char *prefix, *orig_refname = refname;
@@ -360,59 +340,47 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
}
/* Don't add types the caller doesn't want */
- if ((kind & ref_list->kinds) == 0)
+ if ((kind & filter->branch_kind) == 0)
return 0;
- if (!match_patterns(cb->pattern, refname))
+ if (!match_patterns(filter->name_patterns, refname))
return 0;
commit = NULL;
- if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
+ if (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {
commit = lookup_commit_reference_gently(oid->hash, 1);
if (!commit)
return 0;
/* Filter with with_commit if specified */
- if (!is_descendant_of(commit, ref_list->with_commit))
+ if (!is_descendant_of(commit, filter->with_commit))
return 0;
- if (merge_filter != NO_FILTER)
- add_pending_object(&ref_list->revs,
+ if (filter->merge != REF_FILTER_MERGED_NONE)
+ add_pending_object(array->revs,
(struct object *)commit, refname);
}
- ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
+ ref_array_append(array, refname);
+ item = array->items[array->nr - 1];
/* Record the new item */
- newitem = &(ref_list->list[ref_list->index++]);
- newitem->name = xstrdup(refname);
- newitem->kind = kind;
- newitem->commit = commit;
- newitem->dest = resolve_symref(orig_refname, prefix);
- newitem->ignore = 0;
+ item->kind = kind;
+ item->commit = commit;
+ item->symref = resolve_symref(orig_refname, prefix);
+ item->ignore = 0;
return 0;
}
-static void free_ref_list(struct ref_list *ref_list)
-{
- int i;
-
- for (i = 0; i < ref_list->index; i++) {
- free(ref_list->list[i].name);
- free(ref_list->list[i].dest);
- }
- free(ref_list->list);
-}
-
static int ref_cmp(const void *r1, const void *r2)
{
- struct ref_item *c1 = (struct ref_item *)(r1);
- struct ref_item *c2 = (struct ref_item *)(r2);
+ struct ref_array_item *c1 = *((struct ref_array_item **)r1);
+ struct ref_array_item *c2 = *((struct ref_array_item **)r2);
if (c1->kind != c2->kind)
return c1->kind - c2->kind;
- return strcmp(c1->name, c2->name);
+ return strcmp(c1->refname, c2->refname);
}
static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
@@ -477,8 +445,8 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
free(ref);
}
-static void add_verbose_info(struct strbuf *out, struct ref_item *item,
- int verbose, int abbrev)
+static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
+ struct ref_filter *filter)
{
struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
const char *sub = _(" **** invalid ref ****");
@@ -490,10 +458,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
}
if (item->kind == REF_LOCAL_BRANCH)
- fill_tracking_info(&stat, item->name, verbose > 1);
+ fill_tracking_info(&stat, item->refname, filter->verbose > 1);
strbuf_addf(out, " %s %s%s",
- find_unique_abbrev(item->commit->object.sha1, abbrev),
+ find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
stat.buf, sub);
strbuf_release(&stat);
strbuf_release(&subject);
@@ -530,21 +498,21 @@ static char *get_head_description(void)
return strbuf_detach(&desc, NULL);
}
-static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
- int abbrev, int detached, const char *remote_prefix)
+static void print_ref_item(struct ref_array_item *item, int maxwidth,
+ struct ref_filter *filter, const char *remote_prefix)
{
char c = ' ';
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = "";
- const char *desc = item->name;
+ const char *desc = item->refname;
if (item->ignore)
return;
switch (item->kind) {
case REF_LOCAL_BRANCH:
- if (!detached && !strcmp(item->name, head)) {
+ if (!filter->detached && !strcmp(item->refname, head)) {
color = BRANCH_COLOR_CURRENT;
c = '*';
} else
@@ -565,7 +533,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
}
strbuf_addf(&name, "%s%s", prefix, desc);
- if (verbose) {
+ if (filter->verbose) {
int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
maxwidth + utf8_compensation, name.buf,
@@ -574,13 +542,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
name.buf, branch_get_color(BRANCH_COLOR_RESET));
- if (item->dest)
- strbuf_addf(&out, " -> %s", item->dest);
- else if (verbose)
+ if (item->symref)
+ strbuf_addf(&out, " -> %s", item->symref);
+ else if (filter->verbose)
/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
- add_verbose_info(&out, item, verbose, abbrev);
+ add_verbose_info(&out, item, filter);
if (column_active(colopts)) {
- assert(!verbose && "--column and --verbose are incompatible");
+ assert(!filter->verbose && "--column and --verbose are incompatible");
string_list_append(&output, out.buf);
} else {
printf("%s\n", out.buf);
@@ -589,14 +557,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
strbuf_release(&out);
}
-static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
{
int i, max = 0;
- for (i = 0; i < refs->index; i++) {
- struct ref_item *it = &refs->list[i];
- int w = utf8_strwidth(it->name);
+ for (i = 0; i < refs->nr; i++) {
+ struct ref_array_item *it = refs->items[i];
+ int w = utf8_strwidth(it->refname);
- if (refs->list[i].ignore)
+ if (it->ignore)
continue;
if (it->kind == REF_REMOTE_BRANCH)
w += remote_bonus;
@@ -606,85 +574,76 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
return max;
}
-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(struct ref_filter *filter)
{
int i, index;
- struct append_ref_cb cb;
- struct ref_list ref_list;
+ struct ref_array array;
+ struct ref_filter_cbdata data;
int maxwidth = 0;
const char *remote_prefix = "";
+ struct rev_info revs;
/*
* If we are listing more than just remote branches,
* then remote branches will have a "remotes/" prefix.
* We need to account for this in the width.
*/
- if (kinds != REF_REMOTE_BRANCH)
+ if (filter->branch_kind != REF_REMOTE_BRANCH)
remote_prefix = "remotes/";
- memset(&ref_list, 0, sizeof(ref_list));
- ref_list.kinds = kinds;
- ref_list.verbose = verbose;
- ref_list.abbrev = abbrev;
- ref_list.with_commit = with_commit;
- if (merge_filter != NO_FILTER)
- init_revisions(&ref_list.revs, NULL);
- cb.ref_list = &ref_list;
- cb.pattern = pattern;
- for_each_rawref(append_ref, &cb);
- if (detached)
- head_ref(append_ref, &cb);
+ memset(&array, 0, sizeof(array));
+ if (filter->merge != REF_FILTER_MERGED_NONE)
+ init_revisions(&revs, NULL);
+
+ data.array = &array;
+ data.filter = filter;
+ array.revs = &revs;
+
+ for_each_rawref(append_ref, &data);
+ if (filter->detached)
+ head_ref(append_ref, &data);
/*
* The following implementation is currently duplicated in ref-filter. It
* will eventually be removed when we port branch.c to use ref-filter APIs.
*/
- if (merge_filter != NO_FILTER) {
- struct commit *filter;
- filter = lookup_commit_reference_gently(merge_filter_ref, 0);
- if (!filter)
- die(_("object '%s' does not point to a commit"),
- sha1_to_hex(merge_filter_ref));
-
- filter->object.flags |= UNINTERESTING;
- add_pending_object(&ref_list.revs,
- (struct object *) filter, "");
- ref_list.revs.limited = 1;
-
- if (prepare_revision_walk(&ref_list.revs))
+ if (filter->merge != REF_FILTER_MERGED_NONE) {
+ filter->merge_commit->object.flags |= UNINTERESTING;
+ add_pending_object(&revs, &filter->merge_commit->object, "");
+ revs.limited = 1;
+
+ if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
- for (i = 0; i < ref_list.index; i++) {
- struct ref_item *item = &ref_list.list[i];
+ for (i = 0; i < array.nr; i++) {
+ struct ref_array_item *item = array.items[i];
struct commit *commit = item->commit;
int is_merged = !!(commit->object.flags & UNINTERESTING);
- item->ignore = is_merged != (merge_filter == SHOW_MERGED);
+ item->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);
}
- for (i = 0; i < ref_list.index; i++) {
- struct ref_item *item = &ref_list.list[i];
+ for (i = 0; i < array.nr; i++) {
+ struct ref_array_item *item = array.items[i];
clear_commit_marks(item->commit, ALL_REV_FLAGS);
}
- clear_commit_marks(filter, ALL_REV_FLAGS);
+ clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
}
- if (verbose)
- maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
+ if (filter->verbose)
+ maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
- index = ref_list.index;
+ index = array.nr;
/* Print detached heads before sorting and printing the rest */
- if (detached) {
- print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
- detached, remote_prefix);
+ if (filter->detached) {
+ print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
index -= 1;
}
- qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
+ qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
for (i = 0; i < index; i++)
- print_ref_item(&ref_list.list[i], maxwidth, verbose,
- abbrev, detached, remote_prefix);
+ print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
- free_ref_list(&ref_list);
+ ref_array_clear(&array);
}
static void rename_branch(const char *oldname, const char *newname, int force)
@@ -740,24 +699,6 @@ static void rename_branch(const char *oldname, const char *newname, int force)
strbuf_release(&newsection);
}
-/*
- * This function is duplicated in ref-filter. It will eventually be removed
- * when we port branch.c to use ref-filter APIs.
- */
-static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)
-{
- merge_filter = ((opt->long_name[0] == 'n')
- ? SHOW_NOT_MERGED
- : SHOW_MERGED);
- if (unset)
- merge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */
- if (!arg)
- arg = "HEAD";
- if (get_sha1(arg, merge_filter_ref))
- die(_("malformed object name %s"), arg);
- return 0;
-}
-
static const char edit_description[] = "BRANCH_DESCRIPTION";
static int edit_branch_description(const char *branch_name)
@@ -797,17 +738,15 @@ static int edit_branch_description(const char *branch_name)
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, force = 0, list = 0;
- int verbose = 0, abbrev = -1, detached = 0;
int reflog = 0, edit_description = 0;
int quiet = 0, unset_upstream = 0;
const char *new_upstream = NULL;
enum branch_track track;
- int kinds = REF_LOCAL_BRANCH;
- struct commit_list *with_commit = NULL;
+ struct ref_filter filter;
struct option options[] = {
OPT_GROUP(N_("Generic options")),
- OPT__VERBOSE(&verbose,
+ OPT__VERBOSE(&filter.verbose,
N_("show hash and subject, give twice for upstream branch")),
OPT__QUIET(&quiet, N_("suppress informational messages")),
OPT_SET_INT('t', "track", &track, N_("set up tracking mode (see git-pull(1))"),
@@ -817,14 +756,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
OPT_BOOL(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
OPT__COLOR(&branch_use_color, N_("use colored output")),
- OPT_SET_INT('r', "remotes", &kinds, N_("act on remote-tracking branches"),
+ OPT_SET_INT('r', "remotes", &filter.branch_kind, N_("act on remote-tracking branches"),
REF_REMOTE_BRANCH),
- OPT_CONTAINS(&with_commit, N_("print only branches that contain the commit")),
- OPT_WITH(&with_commit, N_("print only branches that contain the commit")),
- OPT__ABBREV(&abbrev),
+ OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
+ OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
+ OPT__ABBREV(&filter.abbrev),
OPT_GROUP(N_("Specific git-branch actions:")),
- OPT_SET_INT('a', "all", &kinds, N_("list both remote-tracking and local branches"),
+ OPT_SET_INT('a', "all", &filter.branch_kind, N_("list both remote-tracking and local branches"),
REF_REMOTE_BRANCH | REF_LOCAL_BRANCH),
OPT_BIT('d', "delete", &delete, N_("delete fully merged branch"), 1),
OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2),
@@ -835,22 +774,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "edit-description", &edit_description,
N_("edit the description for the branch")),
OPT__FORCE(&force, N_("force creation, move/rename, deletion")),
- {
- OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
- N_("commit"), N_("print only not merged branches"),
- PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
- opt_parse_merge_filter, (intptr_t) "HEAD",
- },
- {
- OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
- N_("commit"), N_("print only merged branches"),
- PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
- opt_parse_merge_filter, (intptr_t) "HEAD",
- },
+ OPT_MERGED(&filter, N_("print only branches that are merged")),
+ OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
OPT_END(),
};
+ memset(&filter, 0, sizeof(filter));
+ filter.branch_kind = REF_LOCAL_BRANCH;
+ filter.abbrev = -1;
+
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_branch_usage, options);
@@ -862,11 +795,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!head)
die(_("Failed to resolve HEAD as a valid ref."));
if (!strcmp(head, "HEAD"))
- detached = 1;
+ filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
die(_("HEAD not found below refs/heads!"));
- hashcpy(merge_filter_ref, head_sha1);
-
argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
0);
@@ -874,17 +805,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
list = 1;
- if (with_commit || merge_filter != NO_FILTER)
+ if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
list = 1;
if (!!delete + !!rename + !!new_upstream +
list + unset_upstream > 1)
usage_with_options(builtin_branch_usage, options);
- if (abbrev == -1)
- abbrev = DEFAULT_ABBREV;
+ if (filter.abbrev == -1)
+ filter.abbrev = DEFAULT_ABBREV;
finalize_colopts(&colopts, -1);
- if (verbose) {
+ if (filter.verbose) {
if (explicitly_enable_column(colopts))
die(_("--column and --verbose are incompatible"));
colopts = 0;
@@ -898,12 +829,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (delete) {
if (!argc)
die(_("branch name required"));
- return delete_branches(argc, argv, delete > 1, kinds, quiet);
+ return delete_branches(argc, argv, delete > 1, filter.branch_kind, quiet);
} else if (list) {
- if (kinds & REF_LOCAL_BRANCH)
- kinds |= REF_DETACHED_HEAD;
- print_ref_list(kinds, detached, verbose, abbrev,
- with_commit, argv);
+ if (filter.branch_kind & REF_LOCAL_BRANCH)
+ filter.branch_kind |= REF_DETACHED_HEAD;
+ filter.name_patterns = argv;
+ print_ref_list(&filter);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
return 0;
@@ -913,7 +844,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
struct strbuf branch_ref = STRBUF_INIT;
if (!argc) {
- if (detached)
+ if (filter.detached)
die(_("Cannot give description to detached HEAD"));
branch_name = head;
} else if (argc == 1)
@@ -1001,7 +932,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!branch)
die(_("no such branch '%s'"), argv[0]);
- if (kinds != REF_LOCAL_BRANCH)
+ if (filter.branch_kind != REF_LOCAL_BRANCH)
die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
if (track == BRANCH_TRACK_OVERRIDE)
diff --git a/ref-filter.h b/ref-filter.h
index 7d1871d..3458595 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@
#include "refs.h"
#include "commit.h"
#include "parse-options.h"
+#include "revision.h"
/* Quoting styles */
#define QUOTE_NONE 0
@@ -48,6 +49,7 @@ struct ref_sorting {
struct ref_array_item {
unsigned char objectname[20];
int flag, kind;
+ int ignore : 1;
const char *symref;
struct commit *commit;
struct atom_value *value;
@@ -57,6 +59,7 @@ struct ref_array_item {
struct ref_array {
int nr, alloc;
struct ref_array_item **items;
+ struct rev_info *revs;
};
struct ref_filter {
@@ -72,8 +75,10 @@ struct ref_filter {
struct commit *merge_commit;
unsigned int with_commit_tag_algo : 1,
- match_as_path : 1;
+ match_as_path : 1,
+ detached : 1;
unsigned int lines, branch_kind;
+ int abbrev, verbose;
};
struct ref_filter_cbdata {
--
2.4.6
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 6:56 ` [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures Karthik Nayak
@ 2015-07-28 8:17 ` Christian Couder
2015-07-28 13:48 ` Matthieu Moy
2015-07-28 20:38 ` Karthik Nayak
2015-07-28 8:22 ` Christian Couder
1 sibling, 2 replies; 88+ messages in thread
From: Christian Couder @ 2015-07-28 8:17 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> +static void ref_array_append(struct ref_array *array, const char *refname)
> +{
> + size_t len = strlen(refname);
> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
> + memcpy(ref->refname, refname, len);
> + ref->refname[len] = '\0';
> + REALLOC_ARRAY(array->items, array->nr + 1);
> + array->items[array->nr++] = ref;
> +}
This function belongs more to ref-filter.{c,h}...
[...]
> - ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
> + ref_array_append(array, refname);
> + item = array->items[array->nr - 1];
...and the above is a bit ugly.
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 8:17 ` Christian Couder
@ 2015-07-28 13:48 ` Matthieu Moy
2015-07-28 20:41 ` Karthik Nayak
2015-07-28 20:38 ` Karthik Nayak
1 sibling, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 13:48 UTC (permalink / raw)
To: Christian Couder; +Cc: Karthik Nayak, git, Junio C Hamano
Christian Couder <christian.couder@gmail.com> writes:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> +static void ref_array_append(struct ref_array *array, const char *refname)
>> +{
>> + size_t len = strlen(refname);
>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>> + memcpy(ref->refname, refname, len);
>> + ref->refname[len] = '\0';
This looks very much like new_ref_array_item, except that the later also
takes an objectname parameter. I find it suspicious that you leave the
objectname field uninitialized.
Why is this code not calling new_ref_array_item?
A detail: you could return a pointer to the newly allocated object to
write
item = ref_array_append(array, refname);
instead of
ref_array_append(array, refname);
item = array->items[array->nr - 1];
>> + REALLOC_ARRAY(array->items, array->nr + 1);
>> + array->items[array->nr++] = ref;
>> +}
>
> This function belongs more to ref-filter.{c,h}...
The function disapears in the next commit, but I also think that this
function deserves to exist in ref-filter.{c,h} and remain after the end
of the series.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 13:48 ` Matthieu Moy
@ 2015-07-28 20:41 ` Karthik Nayak
2015-07-29 10:08 ` Matthieu Moy
0 siblings, 1 reply; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 20:41 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Christian Couder, git, Junio C Hamano
On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>> +{
>>> + size_t len = strlen(refname);
>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>> + memcpy(ref->refname, refname, len);
>>> + ref->refname[len] = '\0';
>
> This looks very much like new_ref_array_item, except that the later also
> takes an objectname parameter. I find it suspicious that you leave the
> objectname field uninitialized.
>
Well the objectname is not required here, the idea is to retain the way branch.c
works.
> Why is this code not calling new_ref_array_item?
>
Because no objectname is there.
> A detail: you could return a pointer to the newly allocated object to
> write
>
> item = ref_array_append(array, refname);
>
> instead of
>
> ref_array_append(array, refname);
> item = array->items[array->nr - 1];
>
Yeah doing that :)
>>> + REALLOC_ARRAY(array->items, array->nr + 1);
>>> + array->items[array->nr++] = ref;
>>> +}
>>
>> This function belongs more to ref-filter.{c,h}...
>
> The function disapears in the next commit, but I also think that this
> function deserves to exist in ref-filter.{c,h} and remain after the end
> of the series.
>
Why though? I don't see the need when new_ref_array_item() is present.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 20:41 ` Karthik Nayak
@ 2015-07-29 10:08 ` Matthieu Moy
2015-07-29 19:38 ` Karthik Nayak
0 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-29 10:08 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Christian Couder, git, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>
>>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>>> +{
>>>> + size_t len = strlen(refname);
>>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>>> + memcpy(ref->refname, refname, len);
>>>> + ref->refname[len] = '\0';
>>
>> This looks very much like new_ref_array_item, except that the later also
>> takes an objectname parameter. I find it suspicious that you leave the
>> objectname field uninitialized.
>>
>
> Well the objectname is not required here, the idea is to retain the way branch.c
> works.
>
>> Why is this code not calling new_ref_array_item?
>>
>
> Because no objectname is there.
You do have the object_id in the scope of the call-site, so why not use
it?
(Well, in any case, do as you think is best, it's temporary throw-away
code, we shouldn't loose too much time polishing it)
>> The function disapears in the next commit, but I also think that this
>> function deserves to exist in ref-filter.{c,h} and remain after the end
>> of the series.
>>
>
> Why though? I don't see the need when new_ref_array_item() is present.
OK, if the function is specificly for "append an item but leave the
objectname uninitialized", it's too specific to be useful somewhere
else. But then, make it more explicit in the name of the function and/or
in a comment.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-29 10:08 ` Matthieu Moy
@ 2015-07-29 19:38 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 19:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Christian Couder, git, Junio C Hamano
On Wed, Jul 29, 2015 at 3:38 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>
>>>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>>>> +{
>>>>> + size_t len = strlen(refname);
>>>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>>>> + memcpy(ref->refname, refname, len);
>>>>> + ref->refname[len] = '\0';
>>>
>>> This looks very much like new_ref_array_item, except that the later also
>>> takes an objectname parameter. I find it suspicious that you leave the
>>> objectname field uninitialized.
>>>
>>
>> Well the objectname is not required here, the idea is to retain the way branch.c
>> works.
>>
>>> Why is this code not calling new_ref_array_item?
>>>
>>
>> Because no objectname is there.
>
> You do have the object_id in the scope of the call-site, so why not use
> it?
>
> (Well, in any case, do as you think is best, it's temporary throw-away
> code, we shouldn't loose too much time polishing it)
>
Also the fact the new_ref_array_item() is static and not shared. Wouldn't make
sense to make it an API only for a single commit, when the code itself
is temporary.
>>> The function disapears in the next commit, but I also think that this
>>> function deserves to exist in ref-filter.{c,h} and remain after the end
>>> of the series.
>>>
>>
>> Why though? I don't see the need when new_ref_array_item() is present.
>
> OK, if the function is specificly for "append an item but leave the
> objectname uninitialized", it's too specific to be useful somewhere
> else. But then, make it more explicit in the name of the function and/or
> in a comment.
>
Will add a comment :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 8:17 ` Christian Couder
2015-07-28 13:48 ` Matthieu Moy
@ 2015-07-28 20:38 ` Karthik Nayak
1 sibling, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 20:38 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 1:47 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> +static void ref_array_append(struct ref_array *array, const char *refname)
>> +{
>> + size_t len = strlen(refname);
>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>> + memcpy(ref->refname, refname, len);
>> + ref->refname[len] = '\0';
>> + REALLOC_ARRAY(array->items, array->nr + 1);
>> + array->items[array->nr++] = ref;
>> +}
>
> This function belongs more to ref-filter.{c,h}...
>
Its a temporary function which is removed in the next patch.
>
>> - ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
>> + ref_array_append(array, refname);
>> + item = array->items[array->nr - 1];
>
> ...and the above is a bit ugly.
Will fix that :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 6:56 ` [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures Karthik Nayak
2015-07-28 8:17 ` Christian Couder
@ 2015-07-28 8:22 ` Christian Couder
2015-07-28 20:31 ` Karthik Nayak
1 sibling, 1 reply; 88+ messages in thread
From: Christian Couder @ 2015-07-28 8:22 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> diff --git a/ref-filter.h b/ref-filter.h
> index 7d1871d..3458595 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -5,6 +5,7 @@
> #include "refs.h"
> #include "commit.h"
> #include "parse-options.h"
> +#include "revision.h"
>
> /* Quoting styles */
> #define QUOTE_NONE 0
> @@ -48,6 +49,7 @@ struct ref_sorting {
> struct ref_array_item {
> unsigned char objectname[20];
> int flag, kind;
> + int ignore : 1;
Maybe you could add a comment to say that this will be removed in the
next patch.
> const char *symref;
> struct commit *commit;
> struct atom_value *value;
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures
2015-07-28 8:22 ` Christian Couder
@ 2015-07-28 20:31 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 20:31 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Matthieu Moy, Junio C Hamano
On Tue, Jul 28, 2015 at 1:52 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 7d1871d..3458595 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -5,6 +5,7 @@
>> #include "refs.h"
>> #include "commit.h"
>> #include "parse-options.h"
>> +#include "revision.h"
>>
>> /* Quoting styles */
>> #define QUOTE_NONE 0
>> @@ -48,6 +49,7 @@ struct ref_sorting {
>> struct ref_array_item {
>> unsigned char objectname[20];
>> int flag, kind;
>> + int ignore : 1;
>
> Maybe you could add a comment to say that this will be removed in the
> next patch.
>
Yes, Will do. Thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (7 preceding siblings ...)
2015-07-28 6:56 ` [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures Karthik Nayak
@ 2015-07-28 8:42 ` Matthieu Moy
2015-07-28 15:54 ` Karthik Nayak
2015-07-28 15:43 ` Junio C Hamano
2015-07-28 16:19 ` Junio C Hamano
10 siblings, 1 reply; 88+ messages in thread
From: Matthieu Moy @ 2015-07-28 8:42 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> + if (skip_prefix(name, "objectname:size=", &p)) {
> + unsigned int size = atoi(p);
You have the same problem as for tag.c: don't use atoi, and make
accurate error checking (absence of value, negative value, non-integer
value).
If you have other higher-priorities task, leave a temporary comment
/* FIXME: ... */ or /* TODO: ... */ and make sure you have no such
comment when you drop the "RFC" in the subject of your emails.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 8:42 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Matthieu Moy
@ 2015-07-28 15:54 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 15:54 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Tue, Jul 28, 2015 at 2:12 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> + if (skip_prefix(name, "objectname:size=", &p)) {
>> + unsigned int size = atoi(p);
>
> You have the same problem as for tag.c: don't use atoi, and make
> accurate error checking (absence of value, negative value, non-integer
> value).
>
> If you have other higher-priorities task, leave a temporary comment
> /* FIXME: ... */ or /* TODO: ... */ and make sure you have no such
> comment when you drop the "RFC" in the subject of your emails.
>
It's a small change, will fix it with the drop in RFC :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (8 preceding siblings ...)
2015-07-28 8:42 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Matthieu Moy
@ 2015-07-28 15:43 ` Junio C Hamano
2015-07-28 15:55 ` Karthik Nayak
2015-07-28 16:19 ` Junio C Hamano
10 siblings, 1 reply; 88+ messages in thread
From: Junio C Hamano @ 2015-07-28 15:43 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, Matthieu.Moy
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:size=X) where X is a number.
> This will print the first X characters of an objectname.
> The minimum value for X is 5. Hence any value lesser than
> 5 will default to 5 characters.
Is the reason why you do not call this "abbrev" because you are
allowing it to truncate to a non-unique prefix?
If so, wouldn't it make more sense to treat this kind of string
function in a way very similar to your earlier "padright"? I.e.
"%(maxwidth:5)%(objectname)" or something?
If not, and if this is really an abbrev, then it makes sense to make
it specific to the objectname, e.g. "%(objectname:abbrev=7)".
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 15:43 ` Junio C Hamano
@ 2015-07-28 15:55 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-28 15:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Christian Couder, Matthieu Moy
On Tue, Jul 28, 2015 at 9:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(objectname:size=X) where X is a number.
>> This will print the first X characters of an objectname.
>> The minimum value for X is 5. Hence any value lesser than
>> 5 will default to 5 characters.
>
> Is the reason why you do not call this "abbrev" because you are
> allowing it to truncate to a non-unique prefix?
>
> If so, wouldn't it make more sense to treat this kind of string
> function in a way very similar to your earlier "padright"? I.e.
> "%(maxwidth:5)%(objectname)" or something?
>
> If not, and if this is really an abbrev, then it makes sense to make
> it specific to the objectname, e.g. "%(objectname:abbrev=7)".
It is finding unique abbrev, I just named size as it was smaller. But
abbrev seems better, will rename, thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 6:56 ` [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option Karthik Nayak
` (9 preceding siblings ...)
2015-07-28 15:43 ` Junio C Hamano
@ 2015-07-28 16:19 ` Junio C Hamano
2015-07-29 16:02 ` Karthik Nayak
10 siblings, 1 reply; 88+ messages in thread
From: Junio C Hamano @ 2015-07-28 16:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, Matthieu.Moy
Karthik Nayak <karthik.188@gmail.com> writes:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:size=X) where X is a number.
> This will print the first X characters of an objectname.
> The minimum value for X is 5. Hence any value lesser than
> 5 will default to 5 characters.
Where does this hardcoded 5 come from?
I'd agree that we would want some minimum for sanity (not safety),
but I do not think we want random callers of find-unique-abbrev
arbitrarily imposing their own minimum, making different codepaths
behave inconsistently without a good reason.
It seems that the minimum we use for sanity at the core level is
MINIMUM_ABBREV. Is there a reason why that value is inappropriate
for this codepath?
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> ref-filter.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 0a34924..4c5e3f8 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -196,6 +196,8 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
> static int grab_objectname(const char *name, const unsigned char *sha1,
> struct atom_value *v)
> {
> + const char *p;
> +
> if (!strcmp(name, "objectname")) {
> char *s = xmalloc(41);
> strcpy(s, sha1_to_hex(sha1));
> @@ -206,6 +208,13 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> return 1;
> }
> + if (skip_prefix(name, "objectname:size=", &p)) {
> + unsigned int size = atoi(p);
> + if (size < 5)
> + size = 5;
> + v->s = xstrdup(find_unique_abbrev(sha1, size));
> + return 1;
> + }
> return 0;
> }
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option
2015-07-28 16:19 ` Junio C Hamano
@ 2015-07-29 16:02 ` Karthik Nayak
0 siblings, 0 replies; 88+ messages in thread
From: Karthik Nayak @ 2015-07-29 16:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Christian Couder, Matthieu Moy
On Tue, Jul 28, 2015 at 9:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(objectname:size=X) where X is a number.
>> This will print the first X characters of an objectname.
>> The minimum value for X is 5. Hence any value lesser than
>> 5 will default to 5 characters.
>
> Where does this hardcoded 5 come from?
>
> I'd agree that we would want some minimum for sanity (not safety),
> but I do not think we want random callers of find-unique-abbrev
> arbitrarily imposing their own minimum, making different codepaths
> behave inconsistently without a good reason.
>
> It seems that the minimum we use for sanity at the core level is
> MINIMUM_ABBREV. Is there a reason why that value is inappropriate
> for this codepath?
>
I don't quite remember, This is definitely wrong. Thanks
Will use MINIMUM_ABBREV.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 88+ messages in thread