* [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
@ 2015-10-08 9:17 ` Karthik Nayak
2015-10-08 18:48 ` Matthieu Moy
2015-10-08 19:19 ` Matthieu Moy
2015-10-08 9:17 ` [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>) Karthik Nayak
` (9 subsequent siblings)
10 siblings, 2 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:17 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Implement %(if), %(then) and %(else) atoms. Used as
%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
format string between %(if) and %(then) expands to an empty string, or
to only whitespaces, then the whole %(if)...%(end) expands to the
string following %(then). Otherwise, it expands to the string
following %(else), if any. Nesting of this construct is possible.
This is in preperation for porting over `git branch -l` to use
ref-filter APIs for printing.
Add Documentation and tests regarding 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 | 47 +++++++++++++-
ref-filter.c | 127 +++++++++++++++++++++++++++++++++++--
t/t6302-for-each-ref-filter.sh | 67 +++++++++++++++++++
3 files changed, 231 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 16b4ac5..7345acb 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -134,9 +134,17 @@ align::
`<position>` is either left, right or middle, default being
left and `<width>` is the total length of the content with
alignment. If the contents length is more than the width then
- no alignment is performed. If used with '--quote' everything
- in between %(align:...) and %(end) is quoted, but if nested
- then only the topmost level performs quoting.
+ no alignment is performed.
+
+if::
+ Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
+ If there is an atom with value or string literal after the
+ %(if) then everything after the %(then) is printed, else if
+ the %(else) atom is used, then everything after %(else) is
+ printed. We ignore space when evaluating the string before
+ %(then), this is useful when we use the %(HEAD) atom which
+ prints either "*" or " " and we want to apply the 'if'
+ condition only on the 'HEAD' ref.
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -169,6 +177,19 @@ the date by adding one of `:default`, `:relative`, `:short`, `:local`,
`:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.
`%(taggerdate:relative)`.
+Some atoms like %(align) and %(if) always require a matching %(end).
+We call them "opening atoms" and sometimes denote them as %($open).
+
+When a scripting language specific quoting is in effect, except for
+opening atoms, replacement from every %(atom) is quoted when and only
+when it appears at the top-level (that is, when it appears outside
+%($open)...%(end)).
+
+When a scripting language specific quoting is in effect (i.e. one of
+`--shell`, `--perl`, `--python`, `--tcl` is used), everything between
+a top-level opening atom and its matching %(end) is evaluated
+according to the semantics of the opening atom and its result is
+quoted.
EXAMPLES
--------
@@ -256,6 +277,26 @@ eval=`git for-each-ref --shell --format="$fmt" \
eval "$eval"
------------
+
+An example to show the usage of %(if)...%(then)...%(else)...%(end).
+This prefixes the current branch with a star.
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(if)%(HEAD)%(then)* %(else) %(end)%(refname:short)" refs/heads/
+------------
+
+
+An example to show the usage of %(if)...%(then)...%(end).
+This adds a red color to authorname, if present
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(refname)%(if)%(authorname)%(then)%(color:red)%(end) %(authorname)"
+------------
+
SEE ALSO
--------
linkgit:git-show-ref[1]
diff --git a/ref-filter.c b/ref-filter.c
index dbd8fce..95f007c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,9 @@ static struct {
{ "color" },
{ "align" },
{ "end" },
+ { "if" },
+ { "then" },
+ { "else" },
};
#define REF_FORMATTING_STATE_INIT { 0, NULL }
@@ -69,10 +72,16 @@ struct contents {
struct object_id oid;
};
+struct if_then_else {
+ unsigned int then_atom : 1,
+ else_atom : 1,
+ condition_satisfied : 1;
+};
+
struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
- void (*at_end)(struct ref_formatting_stack *stack);
+ void (*at_end)(struct ref_formatting_stack **stack);
void *at_end_data;
};
@@ -216,13 +225,14 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
*stack = prev;
}
-static void end_align_handler(struct ref_formatting_stack *stack)
+static void end_align_handler(struct ref_formatting_stack **stack)
{
- struct align *align = (struct align *)stack->at_end_data;
+ struct ref_formatting_stack *cur = *stack;
+ struct align *align = (struct align *)cur->at_end_data;
struct strbuf s = STRBUF_INIT;
- strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
- strbuf_swap(&stack->output, &s);
+ strbuf_utf8_align(&s, align->position, align->width, cur->output.buf);
+ strbuf_swap(&cur->output, &s);
strbuf_release(&s);
}
@@ -236,6 +246,97 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new->at_end_data = &atomv->u.align;
}
+static void if_then_else_handler(struct ref_formatting_stack **stack)
+{
+ struct ref_formatting_stack *cur = *stack;
+ struct ref_formatting_stack *prev = cur->prev;
+ struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
+
+ if (if_then_else->else_atom) {
+ /*
+ * There is an %(else) atom: we need to drop one state from the
+ * stack, either the %(else) branch if the condition is satisfied, or
+ * the %(then) branch if it isn't.
+ */
+ if (if_then_else->condition_satisfied) {
+ strbuf_reset(&cur->output);
+ pop_stack_element(&cur);
+ } else {
+ strbuf_swap(&cur->output, &prev->output);
+ strbuf_reset(&cur->output);
+ pop_stack_element(&cur);
+ }
+ } else if (!if_then_else->condition_satisfied)
+ /*
+ * No %(else) atom: just drop the %(then) branch if the
+ * condition is not satisfied.
+ */
+ strbuf_reset(&cur->output);
+
+ *stack = cur;
+ free(if_then_else);
+}
+
+static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+ struct ref_formatting_stack *new;
+ struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
+
+ push_stack_element(&state->stack);
+ new = state->stack;
+ new->at_end = if_then_else_handler;
+ new->at_end_data = if_then_else;
+}
+
+static int is_empty(const char *s){
+ while (*s != '\0') {
+ if (!isspace(*s))
+ return 0;
+ s++;
+ }
+ return 1;
+}
+
+static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+ struct ref_formatting_stack *cur = state->stack;
+ struct if_then_else *if_then_else = NULL;
+
+ if (cur->at_end == if_then_else_handler)
+ if_then_else = (struct if_then_else *)cur->at_end_data;
+ if (!if_then_else)
+ die(_("format: %%(then) atom used without an %%(if) atom"));
+ if (if_then_else->then_atom)
+ die(_("format: %%(then) atom used more than once"));
+ if_then_else->then_atom = 1;
+ /*
+ * If there exists non-empty string between the 'if' and
+ * 'then' atom then the 'if' condition is satisfied.
+ */
+ if (cur->output.len && !is_empty(cur->output.buf))
+ if_then_else->condition_satisfied = 1;
+ strbuf_reset(&cur->output);
+}
+
+static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+ struct ref_formatting_stack *prev = state->stack;
+ struct if_then_else *if_then_else = NULL;
+
+ if (prev->at_end == if_then_else_handler)
+ if_then_else = (struct if_then_else *)prev->at_end_data;
+ if (!if_then_else)
+ die(_("format: %%(else) atom used without an %%(if) atom"));
+ if (!if_then_else->then_atom)
+ die(_("format: %%(else) atom used without a %%(then) atom"));
+ if (if_then_else->else_atom)
+ die(_("format: %%(else) atom used more than once"));
+ if_then_else->else_atom = 1;
+ push_stack_element(&state->stack);
+ state->stack->at_end_data = prev->at_end_data;
+ state->stack->at_end = prev->at_end;
+}
+
static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
{
struct ref_formatting_stack *current = state->stack;
@@ -243,14 +344,17 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
if (!current->at_end)
die(_("format: %%(end) atom used without corresponding atom"));
- current->at_end(current);
+ current->at_end(&state->stack);
+
+ /* Stack may have been popped within at_end(), hence reset the current pointer */
+ current = state->stack;
/*
* Perform quote formatting when the stack element is that of
* a supporting atom. If nested then perform quote formatting
* only on the topmost supporting atom.
*/
- if (!state->stack->prev->prev) {
+ if (!current->prev->prev) {
quote_formatting(&s, current->output.buf, state->quote_style);
strbuf_swap(¤t->output, &s);
}
@@ -920,6 +1024,15 @@ static void populate_value(struct ref_array_item *ref)
} else if (!strcmp(name, "end")) {
v->handler = end_atom_handler;
continue;
+ } else if (!strcmp(name, "if")) {
+ v->handler = if_atom_handler;
+ continue;
+ } else if (!strcmp(name, "then")) {
+ v->handler = then_atom_handler;
+ continue;
+ } else if (!strcmp(name, "else")) {
+ v->handler = else_atom_handler;
+ continue;
} else
continue;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..617fa1f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -255,4 +255,71 @@ test_expect_success 'reverse version sort' '
test_cmp expect actual
'
+test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms' '
+ test_must_fail git for-each-ref --format="%(if)" &&
+ test_must_fail git for-each-ref --format="%(then)" &&
+ test_must_fail git for-each-ref --format="%(else)" &&
+ test_must_fail git for-each-ref --format="%(if) %(else)" &&
+ test_must_fail git for-each-ref --format="%(then) %(else)" &&
+ test_must_fail git for-each-ref --format="%(if) %(else)" &&
+ test_must_fail git for-each-ref --format="%(if) %(then) %(else)"
+'
+
+test_expect_success 'check %(if)...%(then)...%(end) atoms' '
+ git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): %(refname)%(end)" >actual &&
+ cat >expect <<-\EOF &&
+ A U Thor: refs/heads/master
+ A U Thor: refs/heads/side
+ A U Thor: refs/odd/spot
+
+ A U Thor: refs/tags/foo1.10
+ A U Thor: refs/tags/foo1.3
+ A U Thor: refs/tags/foo1.6
+ A U Thor: refs/tags/four
+ A U Thor: refs/tags/one
+
+ A U Thor: refs/tags/three
+ A U Thor: refs/tags/two
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
+ git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)" >actual &&
+ cat >expect <<-\EOF &&
+ A U Thor: refs/heads/master
+ A U Thor: refs/heads/side
+ A U Thor: refs/odd/spot
+ No author: refs/tags/double-tag
+ A U Thor: refs/tags/foo1.10
+ A U Thor: refs/tags/foo1.3
+ A U Thor: refs/tags/foo1.6
+ A U Thor: refs/tags/four
+ A U Thor: refs/tags/one
+ No author: refs/tags/signed-tag
+ A U Thor: refs/tags/three
+ A U Thor: refs/tags/two
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'ignore spaces in %(if) atom usage' '
+ git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
+ cat >expect <<-\EOF &&
+ master: Head ref
+ side: Not Head ref
+ odd/spot: Not Head ref
+ double-tag: Not Head ref
+ foo1.10: Not Head ref
+ foo1.3: Not Head ref
+ foo1.6: Not Head ref
+ four: Not Head ref
+ one: Not Head ref
+ signed-tag: Not Head ref
+ three: Not Head ref
+ two: Not Head ref
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms
2015-10-08 9:17 ` [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms Karthik Nayak
@ 2015-10-08 18:48 ` Matthieu Moy
2015-10-10 16:22 ` Karthik Nayak
2015-10-08 19:19 ` Matthieu Moy
1 sibling, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 18:48 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> +static void if_then_else_handler(struct ref_formatting_stack **stack)
> +{
> + struct ref_formatting_stack *cur = *stack;
> + struct ref_formatting_stack *prev = cur->prev;
> + struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> +
You should add
if (!if_then_else->then_atom)
die(_("format: %%(if) atom used without a %%(then) atom"));
here ...
> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *cur = state->stack;
> + struct if_then_else *if_then_else = NULL;
> +
> + if (cur->at_end == if_then_else_handler)
> + if_then_else = (struct if_then_else *)cur->at_end_data;
> + if (!if_then_else)
> + die(_("format: %%(then) atom used without an %%(if) atom"));
> + if (if_then_else->then_atom)
> + die(_("format: %%(then) atom used more than once"));
> + if_then_else->then_atom = 1;
... and
if (if_then_else->else_atom)
die(_("format: %%(then) atom used after %%(else)"));
here, just in case (adding the two corresponding test_must_fail wouldn't
harm of course).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms
2015-10-08 18:48 ` Matthieu Moy
@ 2015-10-10 16:22 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 16:22 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:18 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void if_then_else_handler(struct ref_formatting_stack **stack)
>> +{
>> + struct ref_formatting_stack *cur = *stack;
>> + struct ref_formatting_stack *prev = cur->prev;
>> + struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> +
>
> You should add
>
> if (!if_then_else->then_atom)
> die(_("format: %%(if) atom used without a %%(then) atom"));
>
> here ...
>
Will do.
>> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> + struct ref_formatting_stack *cur = state->stack;
>> + struct if_then_else *if_then_else = NULL;
>> +
>> + if (cur->at_end == if_then_else_handler)
>> + if_then_else = (struct if_then_else *)cur->at_end_data;
>> + if (!if_then_else)
>> + die(_("format: %%(then) atom used without an %%(if) atom"));
>> + if (if_then_else->then_atom)
>> + die(_("format: %%(then) atom used more than once"));
>> + if_then_else->then_atom = 1;
>
> ... and
>
> if (if_then_else->else_atom)
> die(_("format: %%(then) atom used after %%(else)"));
>
> here, just in case (adding the two corresponding test_must_fail wouldn't
> harm of course).
>
Will do, thanks!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms
2015-10-08 9:17 ` [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms Karthik Nayak
2015-10-08 18:48 ` Matthieu Moy
@ 2015-10-08 19:19 ` Matthieu Moy
2015-10-10 17:12 ` Karthik Nayak
1 sibling, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 19:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -134,9 +134,17 @@ align::
> `<position>` is either left, right or middle, default being
> left and `<width>` is the total length of the content with
> alignment. If the contents length is more than the width then
> - no alignment is performed. If used with '--quote' everything
> - in between %(align:...) and %(end) is quoted, but if nested
> - then only the topmost level performs quoting.
> + no alignment is performed.
> +
> +if::
> + Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
I guess you forgot to replace .. with ... (I think you agreed with me
that it was better).
> @@ -69,10 +72,16 @@ struct contents {
> struct object_id oid;
> };
>
> +struct if_then_else {
> + unsigned int then_atom : 1,
> + else_atom : 1,
Maybe "then_atom_seen" and "else_atom_seen" would be better names. Or
maybe they'd be too long, I leave it up to you.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms
2015-10-08 19:19 ` Matthieu Moy
@ 2015-10-10 17:12 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 17:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:49 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -134,9 +134,17 @@ align::
>> `<position>` is either left, right or middle, default being
>> left and `<width>` is the total length of the content with
>> alignment. If the contents length is more than the width then
>> - no alignment is performed. If used with '--quote' everything
>> - in between %(align:...) and %(end) is quoted, but if nested
>> - then only the topmost level performs quoting.
>> + no alignment is performed.
>> +
>> +if::
>> + Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
>
> I guess you forgot to replace .. with ... (I think you agreed with me
> that it was better).
>
Oops! I missed this.
>> @@ -69,10 +72,16 @@ struct contents {
>> struct object_id oid;
>> };
>>
>> +struct if_then_else {
>> + unsigned int then_atom : 1,
>> + else_atom : 1,
>
> Maybe "then_atom_seen" and "else_atom_seen" would be better names. Or
> maybe they'd be too long, I leave it up to you.
>
They do describe the usage in a better way, so why not.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
2015-10-08 9:17 ` [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms Karthik Nayak
@ 2015-10-08 9:17 ` Karthik Nayak
2015-10-08 18:51 ` Matthieu Moy
2015-10-08 9:17 ` [PATCH v2 03/10] ref-filter: add support for %(refname:dir) and %(refname:base) Karthik Nayak
` (8 subsequent siblings)
10 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:17 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Implement %(if:equals=<string>) wherein the if condition is only
satisfied if the value obtained between the %(if:...) and %(then) atom
is the same as the given '<string>'.
Similarly, implement (if:notequals=<string>) wherein the if condition
is only satisfied if the value obtained between the %(if:...) and
%(then) atom is differnt from the given '<string>'.
Add tests and Documentation 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 | 4 +++-
ref-filter.c | 28 ++++++++++++++++++++++++----
t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++
3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7345acb..206d646 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -144,7 +144,9 @@ if::
printed. We ignore space when evaluating the string before
%(then), this is useful when we use the %(HEAD) atom which
prints either "*" or " " and we want to apply the 'if'
- condition only on the 'HEAD' ref.
+ condition only on the 'HEAD' ref. Append ":equals=<string>" or
+ ":notequals=<string>" to compare the value between the
+ %(if:...) and %(then) atoms with the given string.
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 95f007c..359c76d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -73,6 +73,8 @@ struct contents {
};
struct if_then_else {
+ const char *if_equals,
+ *not_equals;
unsigned int then_atom : 1,
else_atom : 1,
condition_satisfied : 1;
@@ -281,6 +283,14 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
{
struct ref_formatting_stack *new;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
+ const char *valp;
+
+ if (skip_prefix(atomv->s, "equals=", &valp))
+ if_then_else->if_equals = valp;
+ else if (skip_prefix(atomv->s, "notequals=", &valp))
+ if_then_else->not_equals = valp;
+ else if (atomv->s[0])
+ die(_("format: unknown format if:%s"), atomv->s);
push_stack_element(&state->stack);
new = state->stack;
@@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
if (if_then_else->then_atom)
die(_("format: %%(then) atom used more than once"));
if_then_else->then_atom = 1;
+
/*
- * If there exists non-empty string between the 'if' and
- * 'then' atom then the 'if' condition is satisfied.
+ * If the 'equals' or 'notequals' attribute is used then
+ * perform the required comparison. If not, only non-empty
+ * strings satisfy the 'if' condition.
*/
- if (cur->output.len && !is_empty(cur->output.buf))
+ if (if_then_else->if_equals) {
+ if (!strcmp(if_then_else->if_equals, cur->output.buf))
+ if_then_else->condition_satisfied = 1;
+ } else if (if_then_else->not_equals) {
+ if (strcmp(if_then_else->not_equals, cur->output.buf))
+ if_then_else->condition_satisfied = 1;
+ } else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
}
@@ -1024,8 +1042,10 @@ static void populate_value(struct ref_array_item *ref)
} else if (!strcmp(name, "end")) {
v->handler = end_atom_handler;
continue;
- } else if (!strcmp(name, "if")) {
+ } else if (match_atom_name(name, "if", &valp)) {
v->handler = if_atom_handler;
+ if (valp)
+ v->s = xstrdup(valp);
continue;
} else if (!strcmp(name, "then")) {
v->handler = then_atom_handler;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 617fa1f..f45ac1f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -322,4 +322,22 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
test_cmp expect actual
'
+test_expect_success 'check %(if:equals=<string>)' '
+ git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
+ cat >expect <<-\EOF &&
+ Found master
+ Not master
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check %(if:notequals=<string>)' '
+ git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
+ cat >expect <<-\EOF &&
+ Found master
+ Not master
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
2015-10-08 9:17 ` [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>) Karthik Nayak
@ 2015-10-08 18:51 ` Matthieu Moy
2015-10-10 17:22 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 18:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> @@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
> if (if_then_else->then_atom)
> die(_("format: %%(then) atom used more than once"));
> if_then_else->then_atom = 1;
> +
> /*
Useless new blank line.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
2015-10-08 18:51 ` Matthieu Moy
@ 2015-10-10 17:22 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 17:22 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:21 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
>> if (if_then_else->then_atom)
>> die(_("format: %%(then) atom used more than once"));
>> if_then_else->then_atom = 1;
>> +
>> /*
>
> Useless new blank line.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
Will remove it, thanks!
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 03/10] ref-filter: add support for %(refname:dir) and %(refname:base)
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
2015-10-08 9:17 ` [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms Karthik Nayak
2015-10-08 9:17 ` [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>) Karthik Nayak
@ 2015-10-08 9:17 ` Karthik Nayak
2015-10-08 9:17 ` [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length Karthik Nayak
` (7 subsequent siblings)
10 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:17 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak
Add the options `:dir` and `:base` to the %(refname) atom. The `:dir`
option gives the directory (the part after $GIT_DIR/) of the ref
without the refname. The `:base` option gives the base directory of
the given ref (i.e. the directory following $GIT_DIR/refs/).
Add tests and documentation for the same.
---
Documentation/git-for-each-ref.txt | 4 +++-
ref-filter.c | 23 +++++++++++++++++++
t/t6302-for-each-ref-filter.sh | 47 ++++++++++++++++++++++++++++++++++++++
3 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 206d646..b8d33a1 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,9 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
- abbreviation mode.
+ abbreviation mode. For the base directory of the ref (i.e. foo
+ in refs/foo/bar/boz) append `:base`. For the entire directory
+ path append `:dir`.
objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/ref-filter.c b/ref-filter.c
index 359c76d..94d8754 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1105,6 +1105,29 @@ static void populate_value(struct ref_array_item *ref)
else
v->s = "<>";
continue;
+ } else if (!strcmp(formatp, "dir") &&
+ (starts_with(name, "refname"))) {
+ const char *sp, *ep, *tmp;
+
+ sp = tmp = ref->refname;
+ /* Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
+ do {
+ ep = tmp;
+ tmp = strchr(ep + 1, '/');
+ } while (tmp);
+ v->s = xstrndup(sp, ep - sp);
+ continue;
+ } else if (!strcmp(formatp, "base") &&
+ (starts_with(name, "refname"))) {
+ const char *sp, *ep;
+
+ if (skip_prefix(ref->refname, "refs/", &sp)) {
+ ep = strchr(sp, '/');
+ if (!ep)
+ continue;
+ v->s = xstrndup(sp, ep - sp);
+ }
+ continue;
} else
die("unknown %.*s format %s",
(int)(formatp - name), name, formatp);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index f45ac1f..19a5075 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -340,4 +340,51 @@ test_expect_success 'check %(if:notequals=<string>)' '
test_cmp expect actual
'
+test_expect_success 'update refs for %(refname:dir) and %(refname:base)' '
+ git update-ref refs/foo HEAD &&
+ git update-ref refs/foodir/bar/boz HEAD
+'
+
+test_expect_success 'check %(refname:dir)' '
+ git for-each-ref --format="%(refname:dir)" >actual &&
+ cat >expect <<-\EOF &&
+ refs
+ refs/foodir/bar
+ refs/heads
+ refs/heads
+ refs/odd
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ refs/tags
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'check %(refname:base)' '
+ git for-each-ref --format="%(refname:base)" >actual &&
+ cat >expect <<-\EOF &&
+
+ foodir
+ heads
+ heads
+ odd
+ tags
+ tags
+ tags
+ tags
+ tags
+ tags
+ tags
+ tags
+ tags
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (2 preceding siblings ...)
2015-10-08 9:17 ` [PATCH v2 03/10] ref-filter: add support for %(refname:dir) and %(refname:base) Karthik Nayak
@ 2015-10-08 9:17 ` Karthik Nayak
2015-10-08 18:58 ` Matthieu Moy
2015-10-08 9:18 ` [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c Karthik Nayak
` (6 subsequent siblings)
10 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:17 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Add support for %(objectname:short=<length>) which would print the
abbreviated unique objectname of given length. When no length is
specified 7 is used. The minimum length is 4.
Add tests and documentation 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 | 4 +++-
ref-filter.c | 30 ++++++++++++++++++++++--------
t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++
3 files changed, 47 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b8d33a1..c713ec0 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -104,7 +104,9 @@ objectsize::
objectname::
The object name (aka SHA-1).
- For a non-ambiguous abbreviation of the object name append `:short`.
+ For a non-ambiguous abbreviation of the object name append
+ `:short`. The length can be explicitly specified by appending
+ `:short=<length>`. Minimum length being 4.
upstream::
The name of a local ref which can be considered ``upstream''
diff --git a/ref-filter.c b/ref-filter.c
index 94d8754..de4d451 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -464,14 +464,28 @@ 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)
{
- if (!strcmp(name, "objectname")) {
- char *s = xmalloc(41);
- strcpy(s, sha1_to_hex(sha1));
- v->s = s;
- return 1;
- }
- if (!strcmp(name, "objectname:short")) {
- v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
+ const char *p;
+
+ if (match_atom_name(name, "objectname", &p)) {
+ /* No arguments given, copy the entire objectname */
+ if (!p) {
+ char *s = xmalloc(41);
+ strcpy(s, sha1_to_hex(sha1));
+ v->s = s;
+ } else {
+ unsigned int length = DEFAULT_ABBREV;
+
+ if (skip_prefix(p, "short", &p)) {
+ if (p[0] == '=')
+ if (strtoul_ui(p + 1, 10, &length))
+ die(_("positive length expected with objectname:%s"), p + 1);
+ } else
+ die(_("format: unknown option entered with objectname:%s"), p);
+
+ if (length < MINIMUM_ABBREV)
+ length = MINIMUM_ABBREV;
+ v->s = xstrdup(find_unique_abbrev(sha1, length));
+ }
return 1;
}
return 0;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..6fc569e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
test_cmp expected actual
'
+cat >expected <<EOF
+$(git rev-parse --short=1 HEAD)
+EOF
+
+test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
+ git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<EOF
+$(git rev-parse --short=10 HEAD)
+EOF
+
+test_expect_success 'Check objectname:short format' '
+ git for-each-ref --format="%(objectname:short=10)" refs/heads >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success 'Check objectname:short format for invalid input' '
+ test_must_fail git for-each-ref --format="%(objectname:short=-1)" refs/heads
+'
+
test_expect_success 'Check for invalid refname format' '
test_must_fail git for-each-ref --format="%(refname:INVALID)"
'
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length
2015-10-08 9:17 ` [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length Karthik Nayak
@ 2015-10-08 18:58 ` Matthieu Moy
2015-10-10 18:15 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 18:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -464,14 +464,28 @@ 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)
> {
> - if (!strcmp(name, "objectname")) {
> - char *s = xmalloc(41);
> - strcpy(s, sha1_to_hex(sha1));
> - v->s = s;
> - return 1;
> - }
> - if (!strcmp(name, "objectname:short")) {
> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> + const char *p;
> +
> + if (match_atom_name(name, "objectname", &p)) {
> + /* No arguments given, copy the entire objectname */
> + if (!p) {
> + char *s = xmalloc(41);
While you're there, you may want to get rid of this hardcoded 41 and
write GIT_SHA1_HEXSZ + 1 instead.
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7c9bec7..6fc569e 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
> test_cmp expected actual
> '
>
> +cat >expected <<EOF
> +$(git rev-parse --short=1 HEAD)
> +EOF
Please write all code within test_expect_success including this
(t/README:
- Put all code inside test_expect_success and other assertions.
Even code that isn't a test per se, but merely some setup code
should be inside a test assertion.
).
> +test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
> + git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +$(git rev-parse --short=10 HEAD)
> +EOF
Likewise.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length
2015-10-08 18:58 ` Matthieu Moy
@ 2015-10-10 18:15 ` Karthik Nayak
2015-10-11 16:05 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 18:15 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:28 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -464,14 +464,28 @@ 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)
>> {
>> - if (!strcmp(name, "objectname")) {
>> - char *s = xmalloc(41);
>> - strcpy(s, sha1_to_hex(sha1));
>> - v->s = s;
>> - return 1;
>> - }
>> - if (!strcmp(name, "objectname:short")) {
>> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> + const char *p;
>> +
>> + if (match_atom_name(name, "objectname", &p)) {
>> + /* No arguments given, copy the entire objectname */
>> + if (!p) {
>> + char *s = xmalloc(41);
>
> While you're there, you may want to get rid of this hardcoded 41 and
> write GIT_SHA1_HEXSZ + 1 instead.
>
Sure, will change that.
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 7c9bec7..6fc569e 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>> test_cmp expected actual
>> '
>>
>> +cat >expected <<EOF
>> +$(git rev-parse --short=1 HEAD)
>> +EOF
>
> Please write all code within test_expect_success including this
> (t/README:
>
> - Put all code inside test_expect_success and other assertions.
>
> Even code that isn't a test per se, but merely some setup code
> should be inside a test assertion.
> ).
>
Was just following the previous syntax, should have read that. fixed it
>> +test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
>> + git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
>> + test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> +$(git rev-parse --short=10 HEAD)
>> +EOF
>
> Likewise.
>
fixed it here too. Thanks
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length
2015-10-10 18:15 ` Karthik Nayak
@ 2015-10-11 16:05 ` Matthieu Moy
2015-10-11 17:44 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:05 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
>>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>>> index 7c9bec7..6fc569e 100755
>>> --- a/t/t6300-for-each-ref.sh
>>> +++ b/t/t6300-for-each-ref.sh
>>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>>> test_cmp expected actual
>>> '
>>>
>>> +cat >expected <<EOF
>>> +$(git rev-parse --short=1 HEAD)
>>> +EOF
>>
>> Please write all code within test_expect_success including this
>> (t/README:
>>
>> - Put all code inside test_expect_success and other assertions.
>>
>> Even code that isn't a test per se, but merely some setup code
>> should be inside a test assertion.
>> ).
>>
>
> Was just following the previous syntax, should have read that. fixed it
The common practice (not necessarily a rule, though) when you write code
next to other code that does not follow the style is:
* If it's not too disturbing, adopt the new style and keep the old code
as-is. I think we are in this case.
* If the new and the old style do not mix well, prepend a "modernize
style" patch to the series, and adopt the new style in the patch
itself.
* If you're too lazy to do a "modernize style", adopt the old style for
consistency.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length
2015-10-11 16:05 ` Matthieu Moy
@ 2015-10-11 17:44 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 17:44 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Sun, Oct 11, 2015 at 9:35 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>>>> index 7c9bec7..6fc569e 100755
>>>> --- a/t/t6300-for-each-ref.sh
>>>> +++ b/t/t6300-for-each-ref.sh
>>>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>>>> test_cmp expected actual
>>>> '
>>>>
>>>> +cat >expected <<EOF
>>>> +$(git rev-parse --short=1 HEAD)
>>>> +EOF
>>>
>>> Please write all code within test_expect_success including this
>>> (t/README:
>>>
>>> - Put all code inside test_expect_success and other assertions.
>>>
>>> Even code that isn't a test per se, but merely some setup code
>>> should be inside a test assertion.
>>> ).
>>>
>>
>> Was just following the previous syntax, should have read that. fixed it
>
> The common practice (not necessarily a rule, though) when you write code
> next to other code that does not follow the style is:
>
> * If it's not too disturbing, adopt the new style and keep the old code
> as-is. I think we are in this case.
>
> * If the new and the old style do not mix well, prepend a "modernize
> style" patch to the series, and adopt the new style in the patch
> itself.
>
> * If you're too lazy to do a "modernize style", adopt the old style for
> consistency.
>
Ah thanks, Makes sense :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (3 preceding siblings ...)
2015-10-08 9:17 ` [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-08 19:01 ` Matthieu Moy
2015-10-08 9:18 ` [PATCH v2 06/10] ref-filter: introduce format_ref_array_item() Karthik Nayak
` (5 subsequent siblings)
10 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Copy the implementation of get_head_description() from branch.c. This
gives a description of the HEAD ref if called. This is used as the
refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
is used. Make it public because we need it to calculate the length of
the HEAD refs description in branch.c:calc_maxwidth() when we port
branch.c 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 | 4 ++++
ref-filter.c | 38 ++++++++++++++++++++++++++++++++++++--
ref-filter.h | 2 ++
3 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index b7a60f4..67ef9f1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -355,6 +355,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
strbuf_release(&subject);
}
+/*
+ * This is duplicated in ref-filter.c, will be removed when we adopt
+ * ref-filter's printing APIs.
+ */
static char *get_head_description(void)
{
struct strbuf desc = STRBUF_INIT;
diff --git a/ref-filter.c b/ref-filter.c
index de4d451..a3cc3af 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
#include "utf8.h"
#include "git-compat-util.h"
#include "version.h"
+#include "wt-status.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -916,6 +917,37 @@ static inline char *copy_advance(char *dst, const char *src)
return dst;
}
+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);
+}
+
/*
* Parse the object referred by ref, and grab needed value.
*/
@@ -954,9 +986,11 @@ static void populate_value(struct ref_array_item *ref)
name++;
}
- if (starts_with(name, "refname"))
+ if (starts_with(name, "refname")) {
refname = ref->refname;
- else if (starts_with(name, "symref"))
+ if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+ refname = get_head_description();
+ } else if (starts_with(name, "symref"))
refname = ref->symref ? ref->symref : "";
else if (starts_with(name, "upstream")) {
const char *branch_name;
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..4aea594 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -106,5 +106,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
struct ref_sorting *ref_default_sorting(void);
/* Function to parse --merged and --no-merged options */
int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+/* Get the current HEAD's description */
+char *get_head_description(void);
#endif /* REF_FILTER_H */
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c
2015-10-08 9:18 ` [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c Karthik Nayak
@ 2015-10-08 19:01 ` Matthieu Moy
2015-10-10 18:17 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 19:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> Copy the implementation of get_head_description() from branch.c. This
> gives a description of the HEAD ref if called. This is used as the
> refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
> is used. Make it public because we need it to calculate the length of
> the HEAD refs description in branch.c:calc_maxwidth() when we port
> branch.c to use ref-filter APIs.
If it's made public, then it could be simpler to just _move_ the
function instead of copying it. You'd need to add a #include
<ref-filter.c> to branch.c, but you're going to add one anyway.
Code movement is more "git blame" friendly than code copy, and as a
reviewer I'd rather see the code movement here and not hear about it
later in the series.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c
2015-10-08 19:01 ` Matthieu Moy
@ 2015-10-10 18:17 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 18:17 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:31 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Copy the implementation of get_head_description() from branch.c. This
>> gives a description of the HEAD ref if called. This is used as the
>> refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
>> is used. Make it public because we need it to calculate the length of
>> the HEAD refs description in branch.c:calc_maxwidth() when we port
>> branch.c to use ref-filter APIs.
>
> If it's made public, then it could be simpler to just _move_ the
> function instead of copying it. You'd need to add a #include
> <ref-filter.c> to branch.c, but you're going to add one anyway.
>
> Code movement is more "git blame" friendly than code copy, and as a
> reviewer I'd rather see the code movement here and not hear about it
> later in the series.
>
Also this needs to be done, cause now compiling branch.c would give a
multiple declaration error. Will do as you suggested. Thanks
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 06/10] ref-filter: introduce format_ref_array_item()
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (4 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-08 9:18 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
` (4 subsequent siblings)
10 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
To allow column display, we will need to first render the output in a
string list to allow print_columns() to compute the proper size of
each column before starting the actual output. Introduce the function
format_ref_array_item() that does the formatting of a ref_array_item
to an strbuf.
show_ref_array_item() is kept as a convenience wrapper around it which
obtains the strbuf and prints it the standard output.
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 | 16 ++++++++++++----
ref-filter.h | 3 +++
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index a3cc3af..38e4424 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1756,10 +1756,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
}
}
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+ int quote_style, struct strbuf *final_buf)
{
const char *cp, *sp, *ep;
- struct strbuf *final_buf;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
state.quote_style = quote_style;
@@ -1789,9 +1789,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
}
if (state.stack->prev)
die(_("format: %%(end) atom missing"));
- final_buf = &state.stack->output;
- fwrite(final_buf->buf, 1, final_buf->len, stdout);
+ strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+{
+ struct strbuf final_buf = STRBUF_INIT;
+
+ format_ref_array_item(info, format, quote_style, &final_buf);
+ fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ strbuf_release(&final_buf);
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 4aea594..0014b92 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -98,6 +98,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
int verify_ref_format(const char *format);
/* Sort the given ref_array as per the ref_sorting provided */
void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
+/* Based on the given format and quote_style, fill the strbuf */
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+ int quote_style, struct strbuf *final_buf);
/* Print the ref using the given format and quote_style */
void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
/* Callback function for parsing the sort option */
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (5 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 06/10] ref-filter: introduce format_ref_array_item() Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-08 18:40 ` Matthieu Moy
2015-10-08 9:18 ` [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket) Karthik Nayak
` (3 subsequent siblings)
10 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Borrowing from branch.c's implementation print "[gone]" whenever an
unknown upstream ref is encountered instead of just ignoring it.
This makes sure that when branch.c is ported over to using ref-filter
APIs for printing, this feature is not lost.
Make changes to t/t6300-for-each-ref.sh to reflect this change.
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 | 4 +++-
t/t6300-for-each-ref.sh | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 38e4424..6a38089 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
char buf[40];
if (stat_tracking_info(branch, &num_ours,
- &num_theirs, NULL))
+ &num_theirs, NULL)) {
+ v->s = "[gone]";
continue;
+ }
if (!num_ours && !num_theirs)
v->s = "";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6fc569e..4f620bf 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -359,7 +359,7 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
test_expect_success 'Check that :track[short] works when upstream is invalid' '
cat >expected <<-\EOF &&
-
+ [gone]
EOF
test_when_finished "git config branch.master.merge refs/heads/master" &&
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-08 9:18 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
@ 2015-10-08 18:40 ` Matthieu Moy
2015-10-10 18:19 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 18:40 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
> char buf[40];
>
> if (stat_tracking_info(branch, &num_ours,
> - &num_theirs, NULL))
> + &num_theirs, NULL)) {
> + v->s = "[gone]";
My remark about translation still holds. The string was previously
translated in "branch" and you are removing this translation (well, not
here, but when 09/10 starts using this code).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-08 18:40 ` Matthieu Moy
@ 2015-10-10 18:19 ` Karthik Nayak
2015-10-11 16:12 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-10 18:19 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>> char buf[40];
>>
>> if (stat_tracking_info(branch, &num_ours,
>> - &num_theirs, NULL))
>> + &num_theirs, NULL)) {
>> + v->s = "[gone]";
>
> My remark about translation still holds. The string was previously
> translated in "branch" and you are removing this translation (well, not
> here, but when 09/10 starts using this code).
>
I should have mentioned in my cover letter, I didn't really understand
what has to be done about this, couldn't find much reference to go
about this. What do you suggest?
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-10 18:19 ` Karthik Nayak
@ 2015-10-11 16:12 ` Matthieu Moy
2015-10-11 16:16 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Matthieu Moy
2015-10-11 17:46 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
0 siblings, 2 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:12 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>>> char buf[40];
>>>
>>> if (stat_tracking_info(branch, &num_ours,
>>> - &num_theirs, NULL))
>>> + &num_theirs, NULL)) {
>>> + v->s = "[gone]";
>>
>> My remark about translation still holds. The string was previously
>> translated in "branch" and you are removing this translation (well, not
>> here, but when 09/10 starts using this code).
>>
>
> I should have mentioned in my cover letter, I didn't really understand
> what has to be done about this, couldn't find much reference to go
> about this. What do you suggest?
>From the user point of view :
git for-each-ref --format '%(upstream:track)' => Should always be the
same, because this may be parsed by scripts (plumbing). Should not
depend on $LANG, and shouldn't change from a version of Git to another.
git branch --format '%(upstream:track)' => Should show what is most
pleasant to the user (porcelain): translated according to $LANG and
friends, and may be improved in the future.
I already pointed out a fix where a string was translated in a plumbing
command. Another example is setup_unpack_trees_porcelain() in
unpack-trees.c which solves exactly the same problem.
I'll followup with a small series on top of yours to show the way. I did
not try to polish it since I guess you have local changes on the same
part of the code. Feel free to squash patches together or to squash them
with yours. The commit messages are not meant to be final either.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup
2015-10-11 16:12 ` Matthieu Moy
@ 2015-10-11 16:16 ` Matthieu Moy
2015-10-11 16:16 ` [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output Matthieu Moy
` (2 more replies)
2015-10-11 17:46 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
1 sibling, 3 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:16 UTC (permalink / raw)
To: git, karthik.188; +Cc: Matthieu Moy
The char buf[40] is safe (at least while the strings are not
translated), but I'd rather avoid magic numbers like this 40 in the
code, and use a construct that does not have this size limitation.
Especially if it makes the code shorter.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
ref-filter.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 6044eb0..7932c21 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1116,7 +1116,6 @@ static void populate_value(struct ref_array_item *ref)
strcmp(formatp, "trackshort") &&
(starts_with(name, "upstream") ||
starts_with(name, "push"))) {
- char buf[40];
unsigned int nobracket = 0;
if (!strcmp(valp, ",nobracket"))
@@ -1135,24 +1134,21 @@ static void populate_value(struct ref_array_item *ref)
v->s = "";
else if (!num_ours) {
if (nobracket)
- sprintf(buf, "behind %d", num_theirs);
+ v->s = xstrfmt("behind %d", num_theirs);
else
- sprintf(buf, "[behind %d]", num_theirs);
- v->s = xstrdup(buf);
+ v->s = xstrfmt("[behind %d]", num_theirs);
} else if (!num_theirs) {
if (nobracket)
- sprintf(buf, "ahead %d", num_ours);
+ v->s = xstrfmt("ahead %d", num_ours);
else
- sprintf(buf, "[ahead %d]", num_ours);
- v->s = xstrdup(buf);
+ v->s = xstrfmt("[ahead %d]", num_ours);
} else {
if (nobracket)
- sprintf(buf, "ahead %d, behind %d",
- num_ours, num_theirs);
+ v->s = xstrfmt("ahead %d, behind %d",
+ num_ours, num_theirs);
else
- sprintf(buf, "[ahead %d, behind %d]",
- num_ours, num_theirs);
- v->s = xstrdup(buf);
+ v->s = xstrfmt("[ahead %d, behind %d]",
+ num_ours, num_theirs);
}
continue;
} else if (!strcmp(formatp, "trackshort") &&
--
2.6.0.rc2.24.gb06d8e9.dirty
^ permalink raw reply related [flat|nested] 66+ messages in thread* [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output
2015-10-11 16:16 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Matthieu Moy
@ 2015-10-11 16:16 ` Matthieu Moy
2015-10-11 19:25 ` Karthik Nayak
2015-10-11 16:16 ` [PATCH 3/3] branch, tag: use porcelain output Matthieu Moy
2015-10-12 17:45 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Junio C Hamano
2 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:16 UTC (permalink / raw)
To: git, karthik.188; +Cc: Matthieu Moy
This patch shows the way, but is obviously incomplete as it works only
for "nobracket" version. Actually, I think the code should first build
the unbracketed output string and then do something like
if (!nobracket) {
const char *to_free = v->s;
v->s = xstrfmt("[%s]", v->s);
free(to_free);
}
so we don't have to worry about brackets anywhere else in the code.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
ref-filter.c | 28 ++++++++++++++++++++++++----
ref-filter.h | 3 +++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 7932c21..c2ee8c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -15,6 +15,26 @@
#include "version.h"
#include "wt-status.h"
+static struct ref_msg {
+ const char *gone;
+ const char *ahead;
+ const char *behind;
+ const char *ahead_behind;
+} msgs = {
+ "gone",
+ "ahead %d",
+ "behind %d",
+ "ahead %d, behind %d"
+};
+
+void setup_ref_filter_porcelain_msg(void)
+{
+ msgs.gone = _("gone");
+ msgs.ahead = _("ahead %d");
+ msgs.behind = _("behind %d");
+ msgs.ahead_behind = _("ahead %d, behind %d");
+}
+
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
static struct {
@@ -1124,7 +1144,7 @@ static void populate_value(struct ref_array_item *ref)
if (stat_tracking_info(branch, &num_ours,
&num_theirs, NULL)) {
if (nobracket)
- v->s = "gone";
+ v->s = msgs.gone;
else
v->s = "[gone]";
continue;
@@ -1134,17 +1154,17 @@ static void populate_value(struct ref_array_item *ref)
v->s = "";
else if (!num_ours) {
if (nobracket)
- v->s = xstrfmt("behind %d", num_theirs);
+ v->s = xstrfmt(msgs.behind, num_theirs);
else
v->s = xstrfmt("[behind %d]", num_theirs);
} else if (!num_theirs) {
if (nobracket)
- v->s = xstrfmt("ahead %d", num_ours);
+ v->s = xstrfmt(msgs.ahead, num_ours);
else
v->s = xstrfmt("[ahead %d]", num_ours);
} else {
if (nobracket)
- v->s = xstrfmt("ahead %d, behind %d",
+ v->s = xstrfmt(msgs.ahead_behind,
num_ours, num_theirs);
else
v->s = xstrfmt("[ahead %d, behind %d]",
diff --git a/ref-filter.h b/ref-filter.h
index 0014b92..2cce02c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -112,4 +112,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
/* Get the current HEAD's description */
char *get_head_description(void);
+/* Set up translated strings in the output. */
+void setup_ref_filter_porcelain_msg(void);
+
#endif /* REF_FILTER_H */
--
2.6.0.rc2.24.gb06d8e9.dirty
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output
2015-10-11 16:16 ` [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output Matthieu Moy
@ 2015-10-11 19:25 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 19:25 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git
On Sun, Oct 11, 2015 at 9:46 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> This patch shows the way, but is obviously incomplete as it works only
> for "nobracket" version. Actually, I think the code should first build
> the unbracketed output string and then do something like
>
> if (!nobracket) {
> const char *to_free = v->s;
> v->s = xstrfmt("[%s]", v->s);
> free(to_free);
> }
>
Makes sense.
> so we don't have to worry about brackets anywhere else in the code.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> ref-filter.c | 28 ++++++++++++++++++++++++----
> ref-filter.h | 3 +++
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 7932c21..c2ee8c9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
> #include "version.h"
> #include "wt-status.h"
>
> +static struct ref_msg {
> + const char *gone;
> + const char *ahead;
> + const char *behind;
> + const char *ahead_behind;
> +} msgs = {
> + "gone",
> + "ahead %d",
> + "behind %d",
> + "ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> + msgs.gone = _("gone");
> + msgs.ahead = _("ahead %d");
> + msgs.behind = _("behind %d");
> + msgs.ahead_behind = _("ahead %d, behind %d");
> +}
> +
> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> static struct {
> @@ -1124,7 +1144,7 @@ static void populate_value(struct ref_array_item *ref)
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> if (nobracket)
> - v->s = "gone";
> + v->s = msgs.gone;
> else
> v->s = "[gone]";
> continue;
> @@ -1134,17 +1154,17 @@ static void populate_value(struct ref_array_item *ref)
> v->s = "";
> else if (!num_ours) {
> if (nobracket)
> - v->s = xstrfmt("behind %d", num_theirs);
> + v->s = xstrfmt(msgs.behind, num_theirs);
> else
> v->s = xstrfmt("[behind %d]", num_theirs);
> } else if (!num_theirs) {
> if (nobracket)
> - v->s = xstrfmt("ahead %d", num_ours);
> + v->s = xstrfmt(msgs.ahead, num_ours);
> else
> v->s = xstrfmt("[ahead %d]", num_ours);
> } else {
> if (nobracket)
> - v->s = xstrfmt("ahead %d, behind %d",
> + v->s = xstrfmt(msgs.ahead_behind,
> num_ours, num_theirs);
> else
> v->s = xstrfmt("[ahead %d, behind %d]",
> diff --git a/ref-filter.h b/ref-filter.h
> index 0014b92..2cce02c 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -112,4 +112,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
> /* Get the current HEAD's description */
> char *get_head_description(void);
>
> +/* Set up translated strings in the output. */
> +void setup_ref_filter_porcelain_msg(void);
> +
> #endif /* REF_FILTER_H */
> --
> 2.6.0.rc2.24.gb06d8e9.dirty
>
I'm squashing this and the other two patches into my series. Thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH 3/3] branch, tag: use porcelain output
2015-10-11 16:16 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Matthieu Moy
2015-10-11 16:16 ` [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output Matthieu Moy
@ 2015-10-11 16:16 ` Matthieu Moy
2015-10-12 17:45 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Junio C Hamano
2 siblings, 0 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:16 UTC (permalink / raw)
To: git, karthik.188; +Cc: Matthieu Moy
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
builtin/branch.c | 2 ++
builtin/tag.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d6c062..041c649 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -531,6 +531,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_END(),
};
+ setup_ref_filter_porcelain_msg();
+
memset(&filter, 0, sizeof(filter));
filter.kind = FILTER_REFS_BRANCHES;
filter.abbrev = -1;
diff --git a/builtin/tag.c b/builtin/tag.c
index 9e17dca..b6d262f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -372,6 +372,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_END()
};
+ setup_ref_filter_porcelain_msg();
+
git_config(git_tag_config, sorting_tail);
memset(&opt, 0, sizeof(opt));
--
2.6.0.rc2.24.gb06d8e9.dirty
^ permalink raw reply related [flat|nested] 66+ messages in thread
* Re: [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup
2015-10-11 16:16 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Matthieu Moy
2015-10-11 16:16 ` [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output Matthieu Moy
2015-10-11 16:16 ` [PATCH 3/3] branch, tag: use porcelain output Matthieu Moy
@ 2015-10-12 17:45 ` Junio C Hamano
2015-10-12 18:01 ` Karthik Nayak
2 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-12 17:45 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, karthik.188
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> The char buf[40] is safe (at least while the strings are not
> translated), but I'd rather avoid magic numbers like this 40 in the
> code, and use a construct that does not have this size limitation.
> Especially if it makes the code shorter.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
The construct being fixed with this change looks very similar to
Peff's a5e03bf5 (ref-filter: drop sprintf and strcpy calls,
2015-09-24) on jk/war-on-sprintf topic, but the new code since that
commit cleaned up.
I'd expect that this will be rolled into Karthik's series in the
next reroll?
Looking good. Thanks.
> ref-filter.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 6044eb0..7932c21 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1116,7 +1116,6 @@ static void populate_value(struct ref_array_item *ref)
> strcmp(formatp, "trackshort") &&
> (starts_with(name, "upstream") ||
> starts_with(name, "push"))) {
> - char buf[40];
> unsigned int nobracket = 0;
>
> if (!strcmp(valp, ",nobracket"))
> @@ -1135,24 +1134,21 @@ static void populate_value(struct ref_array_item *ref)
> v->s = "";
> else if (!num_ours) {
> if (nobracket)
> - sprintf(buf, "behind %d", num_theirs);
> + v->s = xstrfmt("behind %d", num_theirs);
> else
> - sprintf(buf, "[behind %d]", num_theirs);
> - v->s = xstrdup(buf);
> + v->s = xstrfmt("[behind %d]", num_theirs);
> } else if (!num_theirs) {
> if (nobracket)
> - sprintf(buf, "ahead %d", num_ours);
> + v->s = xstrfmt("ahead %d", num_ours);
> else
> - sprintf(buf, "[ahead %d]", num_ours);
> - v->s = xstrdup(buf);
> + v->s = xstrfmt("[ahead %d]", num_ours);
> } else {
> if (nobracket)
> - sprintf(buf, "ahead %d, behind %d",
> - num_ours, num_theirs);
> + v->s = xstrfmt("ahead %d, behind %d",
> + num_ours, num_theirs);
> else
> - sprintf(buf, "[ahead %d, behind %d]",
> - num_ours, num_theirs);
> - v->s = xstrdup(buf);
> + v->s = xstrfmt("[ahead %d, behind %d]",
> + num_ours, num_theirs);
> }
> continue;
> } else if (!strcmp(formatp, "trackshort") &&
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup
2015-10-12 17:45 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Junio C Hamano
@ 2015-10-12 18:01 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-12 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, Git
On Mon, Oct 12, 2015 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> The char buf[40] is safe (at least while the strings are not
>> translated), but I'd rather avoid magic numbers like this 40 in the
>> code, and use a construct that does not have this size limitation.
>> Especially if it makes the code shorter.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>
> The construct being fixed with this change looks very similar to
> Peff's a5e03bf5 (ref-filter: drop sprintf and strcpy calls,
> 2015-09-24) on jk/war-on-sprintf topic, but the new code since that
> commit cleaned up.
>
Yes, pretty much the same.
> I'd expect that this will be rolled into Karthik's series in the
> next reroll?
>
> Looking good. Thanks.
Yes, I've adding this into my series.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-11 16:12 ` Matthieu Moy
2015-10-11 16:16 ` [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup Matthieu Moy
@ 2015-10-11 17:46 ` Karthik Nayak
2015-10-11 18:10 ` Matthieu Moy
1 sibling, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 17:46 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Sun, Oct 11, 2015 at 9:42 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> --- a/ref-filter.c
>>>> +++ b/ref-filter.c
>>>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>>>> char buf[40];
>>>>
>>>> if (stat_tracking_info(branch, &num_ours,
>>>> - &num_theirs, NULL))
>>>> + &num_theirs, NULL)) {
>>>> + v->s = "[gone]";
>>>
>>> My remark about translation still holds. The string was previously
>>> translated in "branch" and you are removing this translation (well, not
>>> here, but when 09/10 starts using this code).
>>>
>>
>> I should have mentioned in my cover letter, I didn't really understand
>> what has to be done about this, couldn't find much reference to go
>> about this. What do you suggest?
>
> From the user point of view :
>
> git for-each-ref --format '%(upstream:track)' => Should always be the
> same, because this may be parsed by scripts (plumbing). Should not
> depend on $LANG, and shouldn't change from a version of Git to another.
>
A little blurry on how this works, as in how translation takes place,
probably need to look at some code.
> git branch --format '%(upstream:track)' => Should show what is most
> pleasant to the user (porcelain): translated according to $LANG and
> friends, and may be improved in the future.
>
> I already pointed out a fix where a string was translated in a plumbing
> command. Another example is setup_unpack_trees_porcelain() in
> unpack-trees.c which solves exactly the same problem.
>
You did. I was just too clueless.
> I'll followup with a small series on top of yours to show the way. I did
> not try to polish it since I guess you have local changes on the same
> part of the code. Feel free to squash patches together or to squash them
> with yours. The commit messages are not meant to be final either.
>
Thanks a lot for this. This should help me out. I'll probably squash them along.
Thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-11 17:46 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
@ 2015-10-11 18:10 ` Matthieu Moy
2015-10-11 19:38 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 18:10 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> A little blurry on how this works, as in how translation takes place,
> probably need to look at some code.
What you really need to understand is: _("foo") is translated, "foo" is
not and will always be "foo". Technically, _ is a macro, it could be
called get_the_translated_string_for(...) but that would be too long.
In git, _("foo") should be used when talking to a user (porcelain), and
"foo" when talking to a program (plumbing). This way a user running
git <plumbing-command> | grep "some-plumbing-message"
will always get the same result regardless of the current locale.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
2015-10-11 18:10 ` Matthieu Moy
@ 2015-10-11 19:38 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 19:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Sun, Oct 11, 2015 at 11:40 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> A little blurry on how this works, as in how translation takes place,
>> probably need to look at some code.
>
> What you really need to understand is: _("foo") is translated, "foo" is
> not and will always be "foo". Technically, _ is a macro, it could be
> called get_the_translated_string_for(...) but that would be too long.
>
> In git, _("foo") should be used when talking to a user (porcelain), and
> "foo" when talking to a program (plumbing). This way a user running
>
> git <plumbing-command> | grep "some-plumbing-message"
>
> will always get the same result regardless of the current locale.
That helped a lot, thanks a bunch.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (6 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-08 19:17 ` Matthieu Moy
2015-10-13 18:13 ` Karthik Nayak
2015-10-08 9:18 ` [PATCH v2 09/10] branch: use ref-filter printing APIs Karthik Nayak
` (2 subsequent siblings)
10 siblings, 2 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak
Add support for %(upstream:track,nobracket) which will print the
tracking information without the brackets (i.e. "ahead N, behind M").
Add test and documentation for the same.
---
Documentation/git-for-each-ref.txt | 6 ++++--
ref-filter.c | 28 +++++++++++++++++++++++-----
t/t6300-for-each-ref.sh | 9 +++++++++
3 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c713ec0..a38cbf6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -114,8 +114,10 @@ upstream::
`refname` above. Additionally respects `:track` to show
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
- or "=" (in sync). Has no effect if the ref does not have
- tracking information associated with it.
+ or "=" (in sync). Append `:track,nobracket` to show tracking
+ information without brackets (i.e "ahead N, behind M"). Has
+ no effect if the ref does not have tracking information
+ associated with it.
push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index 6a38089..6044eb0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
if (!strcmp(formatp, "short"))
refname = shorten_unambiguous_ref(refname,
warn_ambiguous_refs);
- else if (!strcmp(formatp, "track") &&
+ else if (skip_prefix(formatp, "track", &valp) &&
+ strcmp(formatp, "trackshort") &&
(starts_with(name, "upstream") ||
starts_with(name, "push"))) {
char buf[40];
+ unsigned int nobracket = 0;
+
+ if (!strcmp(valp, ",nobracket"))
+ nobracket = 1;
if (stat_tracking_info(branch, &num_ours,
&num_theirs, NULL)) {
- v->s = "[gone]";
+ if (nobracket)
+ v->s = "gone";
+ else
+ v->s = "[gone]";
continue;
}
if (!num_ours && !num_theirs)
v->s = "";
else if (!num_ours) {
- sprintf(buf, "[behind %d]", num_theirs);
+ if (nobracket)
+ sprintf(buf, "behind %d", num_theirs);
+ else
+ sprintf(buf, "[behind %d]", num_theirs);
v->s = xstrdup(buf);
} else if (!num_theirs) {
- sprintf(buf, "[ahead %d]", num_ours);
+ if (nobracket)
+ sprintf(buf, "ahead %d", num_ours);
+ else
+ sprintf(buf, "[ahead %d]", num_ours);
v->s = xstrdup(buf);
} else {
- sprintf(buf, "[ahead %d, behind %d]",
+ if (nobracket)
+ sprintf(buf, "ahead %d, behind %d",
+ num_ours, num_theirs);
+ else
+ sprintf(buf, "[ahead %d, behind %d]",
num_ours, num_theirs);
v->s = xstrdup(buf);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 4f620bf..7ab8bf8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
'
cat >expected <<EOF
+ahead 1
+EOF
+
+test_expect_success 'Check upstream:track,nobracket format' '
+ git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<EOF
>
EOF
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-08 9:18 ` [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket) Karthik Nayak
@ 2015-10-08 19:17 ` Matthieu Moy
2015-10-08 19:19 ` Matthieu Moy
2015-10-13 18:13 ` Karthik Nayak
1 sibling, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 19:17 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
>
> Add test and documentation for the same.
> ---
> Documentation/git-for-each-ref.txt | 6 ++++--
> ref-filter.c | 28 +++++++++++++++++++++++-----
> t/t6300-for-each-ref.sh | 9 +++++++++
> 3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index c713ec0..a38cbf6 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,8 +114,10 @@ upstream::
> `refname` above. Additionally respects `:track` to show
> "[ahead N, behind M]" and `:trackshort` to show the terse
> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> - or "=" (in sync). Has no effect if the ref does not have
> - tracking information associated with it.
> + or "=" (in sync). Append `:track,nobracket` to show tracking
> + information without brackets (i.e "ahead N, behind M"). Has
> + no effect if the ref does not have tracking information
> + associated with it.
>
> push::
> The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6a38089..6044eb0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
> if (!strcmp(formatp, "short"))
> refname = shorten_unambiguous_ref(refname,
> warn_ambiguous_refs);
> - else if (!strcmp(formatp, "track") &&
> + else if (skip_prefix(formatp, "track", &valp) &&
> + strcmp(formatp, "trackshort") &&
> (starts_with(name, "upstream") ||
> starts_with(name, "push"))) {
> char buf[40];
> + unsigned int nobracket = 0;
> +
> + if (!strcmp(valp, ",nobracket"))
> + nobracket = 1;
>
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> - v->s = "[gone]";
> + if (nobracket)
> + v->s = "gone";
> + else
> + v->s = "[gone]";
> continue;
> }
>
> if (!num_ours && !num_theirs)
> v->s = "";
> else if (!num_ours) {
> - sprintf(buf, "[behind %d]", num_theirs);
> + if (nobracket)
> + sprintf(buf, "behind %d", num_theirs);
> + else
> + sprintf(buf, "[behind %d]", num_theirs);
> v->s = xstrdup(buf);
> } else if (!num_theirs) {
> - sprintf(buf, "[ahead %d]", num_ours);
> + if (nobracket)
> + sprintf(buf, "ahead %d", num_ours);
> + else
> + sprintf(buf, "[ahead %d]", num_ours);
> v->s = xstrdup(buf);
> } else {
> - sprintf(buf, "[ahead %d, behind %d]",
> + if (nobracket)
> + sprintf(buf, "ahead %d, behind %d",
> + num_ours, num_theirs);
> + else
> + sprintf(buf, "[ahead %d, behind %d]",
> num_ours, num_theirs);
> v->s = xstrdup(buf);
> }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 4f620bf..7ab8bf8 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
> '
>
> cat >expected <<EOF
> +ahead 1
> +EOF
> +
> +test_expect_success 'Check upstream:track,nobracket format' '
> + git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> >
> EOF
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-08 19:17 ` Matthieu Moy
@ 2015-10-08 19:19 ` Matthieu Moy
0 siblings, 0 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 19:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Oops, sorry, I sent the wrong message, this one is empty. Please ignore.
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>>
>> Add test and documentation for the same.
>> ---
>> Documentation/git-for-each-ref.txt | 6 ++++--
>> ref-filter.c | 28 +++++++++++++++++++++++-----
>> t/t6300-for-each-ref.sh | 9 +++++++++
>> 3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index c713ec0..a38cbf6 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -114,8 +114,10 @@ upstream::
>> `refname` above. Additionally respects `:track` to show
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> - or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it.
>> + or "=" (in sync). Append `:track,nobracket` to show tracking
>> + information without brackets (i.e "ahead N, behind M"). Has
>> + no effect if the ref does not have tracking information
>> + associated with it.
>>
>> push::
>> The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6a38089..6044eb0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>> if (!strcmp(formatp, "short"))
>> refname = shorten_unambiguous_ref(refname,
>> warn_ambiguous_refs);
>> - else if (!strcmp(formatp, "track") &&
>> + else if (skip_prefix(formatp, "track", &valp) &&
>> + strcmp(formatp, "trackshort") &&
>> (starts_with(name, "upstream") ||
>> starts_with(name, "push"))) {
>> char buf[40];
>> + unsigned int nobracket = 0;
>> +
>> + if (!strcmp(valp, ",nobracket"))
>> + nobracket = 1;
>>
>> if (stat_tracking_info(branch, &num_ours,
>> &num_theirs, NULL)) {
>> - v->s = "[gone]";
>> + if (nobracket)
>> + v->s = "gone";
>> + else
>> + v->s = "[gone]";
>> continue;
>> }
>>
>> if (!num_ours && !num_theirs)
>> v->s = "";
>> else if (!num_ours) {
>> - sprintf(buf, "[behind %d]", num_theirs);
>> + if (nobracket)
>> + sprintf(buf, "behind %d", num_theirs);
>> + else
>> + sprintf(buf, "[behind %d]", num_theirs);
>> v->s = xstrdup(buf);
>> } else if (!num_theirs) {
>> - sprintf(buf, "[ahead %d]", num_ours);
>> + if (nobracket)
>> + sprintf(buf, "ahead %d", num_ours);
>> + else
>> + sprintf(buf, "[ahead %d]", num_ours);
>> v->s = xstrdup(buf);
>> } else {
>> - sprintf(buf, "[ahead %d, behind %d]",
>> + if (nobracket)
>> + sprintf(buf, "ahead %d, behind %d",
>> + num_ours, num_theirs);
>> + else
>> + sprintf(buf, "[ahead %d, behind %d]",
>> num_ours, num_theirs);
>> v->s = xstrdup(buf);
>> }
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 4f620bf..7ab8bf8 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>> '
>>
>> cat >expected <<EOF
>> +ahead 1
>> +EOF
>> +
>> +test_expect_success 'Check upstream:track,nobracket format' '
>> + git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
>> + test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> >
>> EOF
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-08 9:18 ` [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket) Karthik Nayak
2015-10-08 19:17 ` Matthieu Moy
@ 2015-10-13 18:13 ` Karthik Nayak
2015-10-13 18:54 ` Matthieu Moy
1 sibling, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-13 18:13 UTC (permalink / raw)
To: Git; +Cc: Christian Couder, Matthieu Moy, Junio C Hamano, Karthik Nayak
On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
>
> Add test and documentation for the same.
> ---
> Documentation/git-for-each-ref.txt | 6 ++++--
> ref-filter.c | 28 +++++++++++++++++++++++-----
> t/t6300-for-each-ref.sh | 9 +++++++++
> 3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index c713ec0..a38cbf6 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,8 +114,10 @@ upstream::
> `refname` above. Additionally respects `:track` to show
> "[ahead N, behind M]" and `:trackshort` to show the terse
> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> - or "=" (in sync). Has no effect if the ref does not have
> - tracking information associated with it.
> + or "=" (in sync). Append `:track,nobracket` to show tracking
> + information without brackets (i.e "ahead N, behind M"). Has
> + no effect if the ref does not have tracking information
> + associated with it.
>
> push::
> The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6a38089..6044eb0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
> if (!strcmp(formatp, "short"))
> refname = shorten_unambiguous_ref(refname,
> warn_ambiguous_refs);
> - else if (!strcmp(formatp, "track") &&
> + else if (skip_prefix(formatp, "track", &valp) &&
> + strcmp(formatp, "trackshort") &&
> (starts_with(name, "upstream") ||
> starts_with(name, "push"))) {
If you see here, we detect "track" first for
%(upstream:track,nobracket) so although
the idea was to use something similar to %(align:...) I don't see a
good way of going
about this. If we want %(upstream:nobracket,track) to be supported then, I think
we'll have to change this whole layout and have the detection done up where we
locat "upstream" / "push", what would be a nice way to go around this?
What I could think of:
1. Do the cleanup that Junio and Matthieu suggested, where we
basically parse the
atoms and store them into a used_atom struct. I could either work on
those patches
before this and then rebase this on top.
2. Let this be and come back on it when implementing the above series.
After reading Matthieu's and Junio's discussion, I lean towards the latter.
> char buf[40];
> + unsigned int nobracket = 0;
> +
> + if (!strcmp(valp, ",nobracket"))
> + nobracket = 1;
>
> if (stat_tracking_info(branch, &num_ours,
> &num_theirs, NULL)) {
> - v->s = "[gone]";
> + if (nobracket)
> + v->s = "gone";
> + else
> + v->s = "[gone]";
> continue;
> }
>
> if (!num_ours && !num_theirs)
> v->s = "";
> else if (!num_ours) {
> - sprintf(buf, "[behind %d]", num_theirs);
> + if (nobracket)
> + sprintf(buf, "behind %d", num_theirs);
> + else
> + sprintf(buf, "[behind %d]", num_theirs);
> v->s = xstrdup(buf);
> } else if (!num_theirs) {
> - sprintf(buf, "[ahead %d]", num_ours);
> + if (nobracket)
> + sprintf(buf, "ahead %d", num_ours);
> + else
> + sprintf(buf, "[ahead %d]", num_ours);
> v->s = xstrdup(buf);
> } else {
> - sprintf(buf, "[ahead %d, behind %d]",
> + if (nobracket)
> + sprintf(buf, "ahead %d, behind %d",
> + num_ours, num_theirs);
> + else
> + sprintf(buf, "[ahead %d, behind %d]",
> num_ours, num_theirs);
> v->s = xstrdup(buf);
> }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 4f620bf..7ab8bf8 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
> '
>
> cat >expected <<EOF
> +ahead 1
> +EOF
> +
> +test_expect_success 'Check upstream:track,nobracket format' '
> + git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
> + test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> >
> EOF
>
> --
> 2.6.0
>
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-13 18:13 ` Karthik Nayak
@ 2015-10-13 18:54 ` Matthieu Moy
2015-10-13 19:09 ` Junio C Hamano
2015-10-14 6:12 ` Karthik Nayak
0 siblings, 2 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-13 18:54 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>>
>> Add test and documentation for the same.
>> ---
>> Documentation/git-for-each-ref.txt | 6 ++++--
>> ref-filter.c | 28 +++++++++++++++++++++++-----
>> t/t6300-for-each-ref.sh | 9 +++++++++
>> 3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index c713ec0..a38cbf6 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -114,8 +114,10 @@ upstream::
>> `refname` above. Additionally respects `:track` to show
>> "[ahead N, behind M]" and `:trackshort` to show the terse
>> version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> - or "=" (in sync). Has no effect if the ref does not have
>> - tracking information associated with it.
>> + or "=" (in sync). Append `:track,nobracket` to show tracking
>> + information without brackets (i.e "ahead N, behind M"). Has
>> + no effect if the ref does not have tracking information
>> + associated with it.
>>
>> push::
>> The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6a38089..6044eb0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>> if (!strcmp(formatp, "short"))
>> refname = shorten_unambiguous_ref(refname,
>> warn_ambiguous_refs);
>> - else if (!strcmp(formatp, "track") &&
>> + else if (skip_prefix(formatp, "track", &valp) &&
>> + strcmp(formatp, "trackshort") &&
>> (starts_with(name, "upstream") ||
>> starts_with(name, "push"))) {
>
> If you see here, we detect "track" first for
> %(upstream:track,nobracket)
Yes, but I still think that this was a bad idea. If you want nobracket
to apply to "track", then the syntax should be
%(upstream:track=nobracket). I think the "nobracket" should apply to
"upstream" (i.e. be global to the atom), hence
%(upstream:nobracket,track) and %(upstream:track,nobracket) should both
be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
but just ignoring "nobracket" in this case would make sense to me.
Special-casing the implementation of "nobracket" also means you're
special-casing its user-visible behavior. And as a user, I hate it when
the behavior is subtely different for things that look like each other
(in this case, %(align:...) and %(upstream:...) ).
> %(upstream:nobracket,track) to be supported then, I think we'll have
> to change this whole layout and have the detection done up where we
> locat "upstream" / "push", what would be a nice way to go around this?
You mean, below
else if (starts_with(name, "upstream")) {
within populate_value()?
I think it would, yes.
> What I could think of:
> 1. Do the cleanup that Junio and Matthieu suggested, where we
> basically parse the
> atoms and store them into a used_atom struct. I could either work on
> those patches
> before this and then rebase this on top.
> 2. Let this be and come back on it when implementing the above series.
> After reading Matthieu's and Junio's discussion, I lean towards the latter.
Leaving it as-is does not fit in my arguments to do the refactoring
later. It's not introducing "another instance of an existing pattern",
but actually a new pattern.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-13 18:54 ` Matthieu Moy
@ 2015-10-13 19:09 ` Junio C Hamano
2015-10-14 6:12 ` Karthik Nayak
1 sibling, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2015-10-13 19:09 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Karthik Nayak, Git, Christian Couder
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> If you see here, we detect "track" first for
>> %(upstream:track,nobracket)
>
> Yes, but I still think that this was a bad idea. If you want
> nobracket to apply to "track", then the syntax should be
> %(upstream:track=nobracket). I think the "nobracket" should apply
> to "upstream" (i.e. be global to the atom), hence
> %(upstream:nobracket,track) and %(upstream:track,nobracket) should
> both be accepted.
That makes sense to me, as I think what is being discussed would be
%(upstream:track=yes,bracket=no) when it is fully spelled out.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
2015-10-13 18:54 ` Matthieu Moy
2015-10-13 19:09 ` Junio C Hamano
@ 2015-10-14 6:12 ` Karthik Nayak
1 sibling, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-14 6:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
Wed, Oct 14, 2015 at 12:24 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Yes, but I still think that this was a bad idea. If you want nobracket
> to apply to "track", then the syntax should be
> %(upstream:track=nobracket). I think the "nobracket" should apply to
> "upstream" (i.e. be global to the atom), hence
> %(upstream:nobracket,track) and %(upstream:track,nobracket) should both
> be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
> but just ignoring "nobracket" in this case would make sense to me.
>
Oh okay, was thinking only WRT the "track" option.
> Special-casing the implementation of "nobracket" also means you're
> special-casing its user-visible behavior. And as a user, I hate it when
> the behavior is subtely different for things that look like each other
> (in this case, %(align:...) and %(upstream:...) ).
>
Makes sense, was just looking for opinions.
>> %(upstream:nobracket,track) to be supported then, I think we'll have
>> to change this whole layout and have the detection done up where we
>> locat "upstream" / "push", what would be a nice way to go around this?
>
> You mean, below
>
> else if (starts_with(name, "upstream")) {
>
> within populate_value()?
>
> I think it would, yes.
>
Yes, that's what I meant.
>> What I could think of:
>> 1. Do the cleanup that Junio and Matthieu suggested, where we
>> basically parse the
>> atoms and store them into a used_atom struct. I could either work on
>> those patches
>> before this and then rebase this on top.
>> 2. Let this be and come back on it when implementing the above series.
>> After reading Matthieu's and Junio's discussion, I lean towards the latter.
>
> Leaving it as-is does not fit in my arguments to do the refactoring
> later. It's not introducing "another instance of an existing pattern",
> but actually a new pattern.
>
I meant after changing whatever we discussed above.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 09/10] branch: use ref-filter printing APIs
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (7 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket) Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-13 16:31 ` Junio C Hamano
2015-10-08 9:18 ` [PATCH v2 10/10] branch: implement '--format' option Karthik Nayak
2015-10-08 12:27 ` [PATCH v2 00/10] port branch.c to use ref-filter's printing options Matthieu Moy
10 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.
Introduce get_format() which gets the format required for printing of
refs. Make amendments to print_ref_list() to reflect these changes.
Change calc_maxwidth() to also account for the length of HEAD ref, by
calling ref-filter:get_head_discription().
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 | 261 ++++++++++++-----------------------------------
t/t6040-tracking-info.sh | 2 +-
2 files changed, 66 insertions(+), 197 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 67ef9f1..4d8b570 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -35,12 +35,12 @@ static unsigned char head_sha1[20];
static int branch_use_color = -1;
static char branch_colors[][COLOR_MAXLEN] = {
- GIT_COLOR_RESET,
- GIT_COLOR_NORMAL, /* PLAIN */
- GIT_COLOR_RED, /* REMOTE */
- GIT_COLOR_NORMAL, /* LOCAL */
- GIT_COLOR_GREEN, /* CURRENT */
- GIT_COLOR_BLUE, /* UPSTREAM */
+ "%(color:reset)",
+ "%(color:reset)", /* PLAIN */
+ "%(color:red)", /* REMOTE */
+ "%(color:reset)", /* LOCAL */
+ "%(color:green)", /* CURRENT */
+ "%(color:blue)", /* UPSTREAM */
};
enum color_branch {
BRANCH_COLOR_RESET = 0,
@@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
return(ret);
}
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
- int show_upstream_ref)
-{
- int ours, theirs;
- char *ref = NULL;
- struct branch *branch = branch_get(branch_name);
- const char *upstream;
- struct strbuf fancy = STRBUF_INIT;
- int upstream_is_gone = 0;
- int added_decoration = 1;
-
- if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
- if (!upstream)
- return;
- upstream_is_gone = 1;
- }
-
- if (show_upstream_ref) {
- ref = shorten_unambiguous_ref(upstream, 0);
- if (want_color(branch_use_color))
- strbuf_addf(&fancy, "%s%s%s",
- branch_get_color(BRANCH_COLOR_UPSTREAM),
- ref, branch_get_color(BRANCH_COLOR_RESET));
- else
- strbuf_addstr(&fancy, ref);
- }
-
- if (upstream_is_gone) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
- else
- added_decoration = 0;
- } else if (!ours && !theirs) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s]"), fancy.buf);
- else
- added_decoration = 0;
- } else if (!ours) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
- else
- strbuf_addf(stat, _("[behind %d]"), theirs);
-
- } else if (!theirs) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
- else
- strbuf_addf(stat, _("[ahead %d]"), ours);
- } else {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
- fancy.buf, ours, theirs);
- else
- strbuf_addf(stat, _("[ahead %d, behind %d]"),
- ours, theirs);
- }
- strbuf_release(&fancy);
- if (added_decoration)
- strbuf_addch(stat, ' ');
- free(ref);
-}
-
-static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
- struct ref_filter *filter, const char *refname)
-{
- struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
- const char *sub = _(" **** invalid ref ****");
- struct commit *commit = item->commit;
-
- if (!parse_commit(commit)) {
- pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
- sub = subject.buf;
- }
-
- if (item->kind == FILTER_REFS_BRANCHES)
- fill_tracking_info(&stat, refname, filter->verbose > 1);
-
- strbuf_addf(out, " %s %s%s",
- find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
- stat.buf, sub);
- strbuf_release(&stat);
- strbuf_release(&subject);
-}
-
-/*
- * This is duplicated in ref-filter.c, will be removed when we adopt
- * ref-filter's printing APIs.
- */
-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 format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
- struct ref_filter *filter, const char *remote_prefix)
-{
- char c;
- int current = 0;
- int color;
- struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
- const char *prefix = "";
- const char *desc = item->refname;
- char *to_free = NULL;
-
- switch (item->kind) {
- case FILTER_REFS_BRANCHES:
- skip_prefix(desc, "refs/heads/", &desc);
- if (!filter->detached && !strcmp(desc, head))
- current = 1;
- else
- color = BRANCH_COLOR_LOCAL;
- break;
- case FILTER_REFS_REMOTES:
- skip_prefix(desc, "refs/remotes/", &desc);
- color = BRANCH_COLOR_REMOTE;
- prefix = remote_prefix;
- break;
- case FILTER_REFS_DETACHED_HEAD:
- desc = to_free = get_head_description();
- current = 1;
- break;
- default:
- color = BRANCH_COLOR_PLAIN;
- break;
- }
-
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
-
- strbuf_addf(&name, "%s%s", prefix, desc);
- 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,
- branch_get_color(BRANCH_COLOR_RESET));
- } else
- strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
- name.buf, branch_get_color(BRANCH_COLOR_RESET));
-
- if (item->symref) {
- skip_prefix(item->symref, "refs/remotes/", &desc);
- strbuf_addf(&out, " -> %s", desc);
- }
- else if (filter->verbose)
- /* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
- add_verbose_info(&out, item, filter, desc);
- if (column_active(colopts)) {
- assert(!filter->verbose && "--column and --verbose are incompatible");
- string_list_append(&output, out.buf);
- } else {
- printf("%s\n", out.buf);
- }
- strbuf_release(&name);
- strbuf_release(&out);
- free(to_free);
-}
-
static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
{
int i, max = 0;
@@ -467,7 +281,10 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
skip_prefix(it->refname, "refs/heads/", &desc);
skip_prefix(it->refname, "refs/remotes/", &desc);
- w = utf8_strwidth(desc);
+ if (it->kind == FILTER_REFS_DETACHED_HEAD)
+ w = strlen(get_head_description());
+ else
+ w = utf8_strwidth(desc);
if (it->kind == FILTER_REFS_REMOTES)
w += remote_bonus;
@@ -477,12 +294,51 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
return max;
}
+static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
+{
+ struct strbuf fmt = STRBUF_INIT;
+ struct strbuf local = STRBUF_INIT;
+ struct strbuf remote = STRBUF_INIT;
+
+ strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
+
+ if (filter->verbose) {
+ strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
+ strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
+ strbuf_addf(&local, " %%(objectname:short=7) ");
+
+ if (filter->verbose > 1)
+ strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
+ "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
+ branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
+ else
+ strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
+
+ strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
+ "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
+ branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
+ remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
+ } else {
+ strbuf_addf(&local, "%%(refname:short)%s", branch_get_color(BRANCH_COLOR_RESET));
+ strbuf_addf(&remote, "%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
+ branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
+ }
+
+ strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
+
+ strbuf_release(&local);
+ strbuf_release(&remote);
+ return strbuf_detach(&fmt, NULL);
+}
+
static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
{
int i;
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
+ struct strbuf out = STRBUF_INIT;
+ char *format;
/*
* If we are listing more than just remote branches,
@@ -494,12 +350,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
memset(&array, 0, sizeof(array));
- verify_ref_format("%(refname)%(symref)");
filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
if (filter->verbose)
maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
+ format = build_format(filter, maxwidth, remote_prefix);
+ verify_ref_format(format);
+
/*
* If no sorting parameter is given then we default to sorting
* by 'refname'. This would give us an alphabetically sorted
@@ -511,10 +369,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
sorting = ref_default_sorting();
ref_array_sort(sorting, &array);
- for (i = 0; i < array.nr; i++)
- format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
+ for (i = 0; i < array.nr; i++) {
+ format_ref_array_item(array.items[i], format, 0, &out);
+ if (column_active(colopts)) {
+ assert(!filter->verbose && "--column and --verbose are incompatible");
+ /* format to a string_list to let print_columns() do its job */
+ string_list_append(&output, out.buf);
+ } else {
+ fwrite(out.buf, 1, out.len, stdout);
+ putchar('\n');
+ }
+ strbuf_release(&out);
+ }
ref_array_clear(&array);
+ free(format);
}
static void rename_branch(const char *oldname, const char *newname, int force)
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 3d5c238..97a0765 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
b2 [ahead 1, behind 1] d
b3 [behind 1] b
b4 [ahead 2] f
-b5 g
+b5 [gone] g
b6 c
EOF
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 09/10] branch: use ref-filter printing APIs
2015-10-08 9:18 ` [PATCH v2 09/10] branch: use ref-filter printing APIs Karthik Nayak
@ 2015-10-13 16:31 ` Junio C Hamano
2015-10-13 17:43 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-13 16:31 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, Matthieu.Moy
Karthik Nayak <karthik.188@gmail.com> writes:
> Port branch.c to use ref-filter APIs for printing. This clears out
> most of the code used in branch.c for printing and replaces them with
> calls made to the ref-filter library.
>
> Introduce get_format() which gets the format required for printing of
> refs. Make amendments to print_ref_list() to reflect these changes.
Is it get_format() still?
>
> Change calc_maxwidth() to also account for the length of HEAD ref, by
> calling ref-filter:get_head_discription().
>
> 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 | 261 ++++++++++++-----------------------------------
> t/t6040-tracking-info.sh | 2 +-
> 2 files changed, 66 insertions(+), 197 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 67ef9f1..4d8b570 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -35,12 +35,12 @@ static unsigned char head_sha1[20];
>
> static int branch_use_color = -1;
> static char branch_colors[][COLOR_MAXLEN] = {
> - GIT_COLOR_RESET,
> - GIT_COLOR_NORMAL, /* PLAIN */
> - GIT_COLOR_RED, /* REMOTE */
> - GIT_COLOR_NORMAL, /* LOCAL */
> - GIT_COLOR_GREEN, /* CURRENT */
> - GIT_COLOR_BLUE, /* UPSTREAM */
> + "%(color:reset)",
> + "%(color:reset)", /* PLAIN */
> + "%(color:red)", /* REMOTE */
> + "%(color:reset)", /* LOCAL */
> + "%(color:green)", /* CURRENT */
> + "%(color:blue)", /* UPSTREAM */
> };
> enum color_branch {
> BRANCH_COLOR_RESET = 0,
> @@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> return(ret);
> }
>
> -static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> - int show_upstream_ref)
> -{
> - int ours, theirs;
> - char *ref = NULL;
> - struct branch *branch = branch_get(branch_name);
> - const char *upstream;
> - struct strbuf fancy = STRBUF_INIT;
> - int upstream_is_gone = 0;
> - int added_decoration = 1;
> -
> - if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> - if (!upstream)
> - return;
> - upstream_is_gone = 1;
> - }
> -
> - if (show_upstream_ref) {
> - ref = shorten_unambiguous_ref(upstream, 0);
> - if (want_color(branch_use_color))
> - strbuf_addf(&fancy, "%s%s%s",
> - branch_get_color(BRANCH_COLOR_UPSTREAM),
> - ref, branch_get_color(BRANCH_COLOR_RESET));
> - else
> - strbuf_addstr(&fancy, ref);
> - }
> -
> - if (upstream_is_gone) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
> - else
> - added_decoration = 0;
> - } else if (!ours && !theirs) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s]"), fancy.buf);
> - else
> - added_decoration = 0;
> - } else if (!ours) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
> - else
> - strbuf_addf(stat, _("[behind %d]"), theirs);
> -
> - } else if (!theirs) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
> - else
> - strbuf_addf(stat, _("[ahead %d]"), ours);
> - } else {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
> - fancy.buf, ours, theirs);
> - else
> - strbuf_addf(stat, _("[ahead %d, behind %d]"),
> - ours, theirs);
> - }
> - strbuf_release(&fancy);
> - if (added_decoration)
> - strbuf_addch(stat, ' ');
> - free(ref);
> -}
> -
> -static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
> - struct ref_filter *filter, const char *refname)
> -{
> - struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> - const char *sub = _(" **** invalid ref ****");
> - struct commit *commit = item->commit;
> -
> - if (!parse_commit(commit)) {
> - pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
> - sub = subject.buf;
> - }
> -
> - if (item->kind == FILTER_REFS_BRANCHES)
> - fill_tracking_info(&stat, refname, filter->verbose > 1);
> -
> - strbuf_addf(out, " %s %s%s",
> - find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
> - stat.buf, sub);
> - strbuf_release(&stat);
> - strbuf_release(&subject);
> -}
> -
> -/*
> - * This is duplicated in ref-filter.c, will be removed when we adopt
> - * ref-filter's printing APIs.
> - */
> -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 format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
> - struct ref_filter *filter, const char *remote_prefix)
> -{
> - char c;
> - int current = 0;
> - int color;
> - struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> - const char *prefix = "";
> - const char *desc = item->refname;
> - char *to_free = NULL;
> -
> - switch (item->kind) {
> - case FILTER_REFS_BRANCHES:
> - skip_prefix(desc, "refs/heads/", &desc);
> - if (!filter->detached && !strcmp(desc, head))
> - current = 1;
> - else
> - color = BRANCH_COLOR_LOCAL;
> - break;
> - case FILTER_REFS_REMOTES:
> - skip_prefix(desc, "refs/remotes/", &desc);
> - color = BRANCH_COLOR_REMOTE;
> - prefix = remote_prefix;
> - break;
> - case FILTER_REFS_DETACHED_HEAD:
> - desc = to_free = get_head_description();
> - current = 1;
> - break;
> - default:
> - color = BRANCH_COLOR_PLAIN;
> - break;
> - }
> -
> - c = ' ';
> - if (current) {
> - c = '*';
> - color = BRANCH_COLOR_CURRENT;
> - }
> -
> - strbuf_addf(&name, "%s%s", prefix, desc);
> - 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,
> - branch_get_color(BRANCH_COLOR_RESET));
> - } else
> - strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
> - name.buf, branch_get_color(BRANCH_COLOR_RESET));
> -
> - if (item->symref) {
> - skip_prefix(item->symref, "refs/remotes/", &desc);
> - strbuf_addf(&out, " -> %s", desc);
> - }
> - else if (filter->verbose)
> - /* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
> - add_verbose_info(&out, item, filter, desc);
> - if (column_active(colopts)) {
> - assert(!filter->verbose && "--column and --verbose are incompatible");
> - string_list_append(&output, out.buf);
> - } else {
> - printf("%s\n", out.buf);
> - }
> - strbuf_release(&name);
> - strbuf_release(&out);
> - free(to_free);
> -}
> -
> static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
> {
> int i, max = 0;
> @@ -467,7 +281,10 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>
> skip_prefix(it->refname, "refs/heads/", &desc);
> skip_prefix(it->refname, "refs/remotes/", &desc);
> - w = utf8_strwidth(desc);
> + if (it->kind == FILTER_REFS_DETACHED_HEAD)
> + w = strlen(get_head_description());
> + else
> + w = utf8_strwidth(desc);
>
> if (it->kind == FILTER_REFS_REMOTES)
> w += remote_bonus;
> @@ -477,12 +294,51 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
> return max;
> }
>
> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{
> + struct strbuf fmt = STRBUF_INIT;
> + struct strbuf local = STRBUF_INIT;
> + struct strbuf remote = STRBUF_INIT;
> +
> + strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
> +
> + if (filter->verbose) {
> + strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
> + strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&local, " %%(objectname:short=7) ");
> +
> + if (filter->verbose > 1)
> + strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> + "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
> + else
> + strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> + strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> + "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> + remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> + } else {
> + strbuf_addf(&local, "%%(refname:short)%s", branch_get_color(BRANCH_COLOR_RESET));
> + strbuf_addf(&remote, "%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> + branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> + }
> +
> + strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
> +
> + strbuf_release(&local);
> + strbuf_release(&remote);
> + return strbuf_detach(&fmt, NULL);
> +}
> +
> static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
> {
> int i;
> struct ref_array array;
> int maxwidth = 0;
> const char *remote_prefix = "";
> + struct strbuf out = STRBUF_INIT;
> + char *format;
>
> /*
> * If we are listing more than just remote branches,
> @@ -494,12 +350,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>
> memset(&array, 0, sizeof(array));
>
> - verify_ref_format("%(refname)%(symref)");
> filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
>
> if (filter->verbose)
> maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
>
> + format = build_format(filter, maxwidth, remote_prefix);
> + verify_ref_format(format);
> +
> /*
> * If no sorting parameter is given then we default to sorting
> * by 'refname'. This would give us an alphabetically sorted
> @@ -511,10 +369,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
> sorting = ref_default_sorting();
> ref_array_sort(sorting, &array);
>
> - for (i = 0; i < array.nr; i++)
> - format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
> + for (i = 0; i < array.nr; i++) {
> + format_ref_array_item(array.items[i], format, 0, &out);
> + if (column_active(colopts)) {
> + assert(!filter->verbose && "--column and --verbose are incompatible");
> + /* format to a string_list to let print_columns() do its job */
> + string_list_append(&output, out.buf);
> + } else {
> + fwrite(out.buf, 1, out.len, stdout);
> + putchar('\n');
> + }
> + strbuf_release(&out);
> + }
>
> ref_array_clear(&array);
> + free(format);
> }
>
> static void rename_branch(const char *oldname, const char *newname, int force)
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 3d5c238..97a0765 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
> b2 [ahead 1, behind 1] d
> b3 [behind 1] b
> b4 [ahead 2] f
> -b5 g
> +b5 [gone] g
> b6 c
> EOF
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 09/10] branch: use ref-filter printing APIs
2015-10-13 16:31 ` Junio C Hamano
@ 2015-10-13 17:43 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-13 17:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git, Christian Couder, Matthieu Moy
On Tue, Oct 13, 2015 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Port branch.c to use ref-filter APIs for printing. This clears out
>> most of the code used in branch.c for printing and replaces them with
>> calls made to the ref-filter library.
>>
>> Introduce get_format() which gets the format required for printing of
>> refs. Make amendments to print_ref_list() to reflect these changes.
>
> Is it get_format() still?
Will change that, thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* [PATCH v2 10/10] branch: implement '--format' option
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (8 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 09/10] branch: use ref-filter printing APIs Karthik Nayak
@ 2015-10-08 9:18 ` Karthik Nayak
2015-10-08 12:27 ` [PATCH v2 00/10] port branch.c to use ref-filter's printing options Matthieu Moy
10 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 9:18 UTC (permalink / raw)
To: git; +Cc: christian.couder, Matthieu.Moy, gitster, Karthik Nayak, Karthik Nayak
Implement the '--format' option provided by 'ref-filter'.
This lets the user list tags as per desired format similar
to the implementation in 'git for-each-ref'.
Add tests and documentation 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-branch.txt | 7 ++++++-
builtin/branch.c | 14 +++++++++-----
t/t3203-branch-output.sh | 11 +++++++++++
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c7af1..5227206 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -12,7 +12,7 @@ SYNOPSIS
[--list] [-v [--abbrev=<length> | --no-abbrev]]
[--column[=<options>] | --no-column]
[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
- [--points-at <object>] [<pattern>...]
+ [--points-at <object>] [--format=<format>] [<pattern>...]
'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
'git branch' --unset-upstream [<branchname>]
@@ -244,6 +244,11 @@ start-point is either a local or remote-tracking branch.
--points-at <object>::
Only list branches of the given object.
+--format <format>::
+ A string that interpolates `%(fieldname)` from the object
+ pointed at by a ref being shown. The format is the same as
+ that of linkgit:git-for-each-ref[1].
+
Examples
--------
diff --git a/builtin/branch.c b/builtin/branch.c
index 4d8b570..2356e72 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -27,6 +27,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
N_("git branch [<options>] [-r | -a] [--points-at]"),
+ N_("git branch [<options>] [-r | -a] [--format]"),
NULL
};
@@ -331,14 +332,14 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
return strbuf_detach(&fmt, NULL);
}
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
{
int i;
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
struct strbuf out = STRBUF_INIT;
- char *format;
+ char *to_free = NULL;
/*
* If we are listing more than just remote branches,
@@ -355,7 +356,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
if (filter->verbose)
maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
- format = build_format(filter, maxwidth, remote_prefix);
+ if (!format)
+ format = to_free = build_format(filter, maxwidth, remote_prefix);
verify_ref_format(format);
/*
@@ -383,7 +385,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
}
ref_array_clear(&array);
- free(format);
+ free(to_free);
}
static void rename_branch(const char *oldname, const char *newname, int force)
@@ -484,6 +486,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
enum branch_track track;
struct ref_filter filter;
static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+ const char *format = NULL;
struct option options[] = {
OPT_GROUP(N_("Generic options")),
@@ -524,6 +527,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
N_("print only branches of the object"), 0, parse_opt_object_name
},
+ OPT_STRING( 0 , "format", &format, N_("format"), N_("format to use for the output")),
OPT_END(),
};
@@ -582,7 +586,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
filter.kind |= FILTER_REFS_DETACHED_HEAD;
filter.name_patterns = argv;
- print_ref_list(&filter, sorting);
+ print_ref_list(&filter, sorting, format);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
return 0;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f1ae5ff..a475ff1 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -163,4 +163,15 @@ test_expect_success 'git branch --points-at option' '
test_cmp expect actual
'
+test_expect_success 'git branch --format option' '
+ cat >expect <<-\EOF &&
+ Refname is (HEAD detached from fromtag)
+ Refname is refs/heads/branch-one
+ Refname is refs/heads/branch-two
+ Refname is refs/heads/master
+ EOF
+ git branch --format="Refname is %(refname)" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.6.0
^ permalink raw reply related [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 9:17 [PATCH v2 00/10] port branch.c to use ref-filter's printing options Karthik Nayak
` (9 preceding siblings ...)
2015-10-08 9:18 ` [PATCH v2 10/10] branch: implement '--format' option Karthik Nayak
@ 2015-10-08 12:27 ` Matthieu Moy
2015-10-08 13:17 ` Karthik Nayak
2015-10-08 16:09 ` Karthik Nayak
10 siblings, 2 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 12:27 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, christian.couder, gitster
Karthik Nayak <karthik.188@gmail.com> writes:
> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
> +This prefixes the current branch with a star.
> +
> +------------
> +#!/bin/sh
> +
> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else) %(end)%(refname:short)" refs/heads/
I don't think the #!/bin/sh adds any value here. Just the 'git
for-each-ref' line is sufficient IMHO.
> +An example to show the usage of %(if)...%(then)...%(end).
> +This adds a red color to authorname, if present
I don't think this is such a good example.
%(color:red)%(authorname)%(color:reset) just works even if %(authorname)
is empty.
A better example would be
git for-each-ref --format='%(if)%(authorname)%(then)Authored by %(authorname)%(end)'
which avoids writting "Authored by " with no author.
> -static int is_empty(const char * s){
> +static int is_empty(const char *s){
You still have the { on the declaration line, it should be on the next
line.
> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
> static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> {
> struct ref_formatting_stack *cur = state->stack;
> - struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> + struct if_then_else *if_then_else = NULL;
>
> + if (cur->at_end == if_then_else_handler)
> + if_then_else = (struct if_then_else *)cur->at_end_data;
OK, now the cast is safe since at_end_data has to be of type struct
if_then_else * if at_end is if_then_else_handler.
> + unsigned int nobracket = 0;
> +
> + if (!strcmp(valp, ",nobracket"))
> + nobracket = 1;
The code to parse comma-separated values is different here and
elsewhere. I'd rather have the same code (possibly factored into a
helper function), both to get consistent behavior (you're not allowing
%(upstream:nobracket,track) for example, right?) and consistent code.
> if (!num_ours && !num_theirs)
> v->s = "";
> else if (!num_ours) {
> - sprintf(buf, "[behind %d]", num_theirs);
> + if (nobracket)
> + sprintf(buf, "behind %d", num_theirs);
> + else
> + sprintf(buf, "[behind %d]", num_theirs);
Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
unconditionnally, and set obracket = "" or obracket = "[" once and for
all when you test for "nobracket" above. This avoids these "if
(nobracket)" spread accross the code, but at the price of extra %s in
the format strings.
> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = "<>";
> continue;
> + } else if (!strcmp(formatp, "dir") &&
> + (starts_with(name, "refname"))) {
> + const char *sp, *ep, *tmp;
> +
> + sp = tmp = ref->refname;
> + /* Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
> + do {
> + ep = tmp;
> + tmp = strchr(ep + 1, '/');
> + } while (tmp);
To look for the last occurence of '/' you can also use strrchr().
Doesn't it do what you want here?
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
> '
>
> cat >expect <<\EOF
> -b1 [origin/master] [ahead 1, behind 1] d
> -b2 [origin/master] [ahead 1, behind 1] d
> -b3 [origin/master] [behind 1] b
> -b4 [origin/master] [ahead 2] f
> -b5 [brokenbase] [gone] g
> +b1 [origin/master: ahead 1, behind 1] d
> +b2 [origin/master: ahead 1, behind 1] d
> +b3 [origin/master: behind 1] b
> +b4 [origin/master: ahead 2] f
> +b5 [brokenbase: gone] g
> b6 [origin/master] c
> EOF
Cool!
I didn't go through the patches themselves, but modulo my remarks above
the interdiff looks good. Thanks.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 12:27 ` [PATCH v2 00/10] port branch.c to use ref-filter's printing options Matthieu Moy
@ 2015-10-08 13:17 ` Karthik Nayak
2015-10-08 16:09 ` Karthik Nayak
1 sibling, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 13:17 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
>> +This prefixes the current branch with a star.
>> +
>> +------------
>> +#!/bin/sh
>> +
>> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else) %(end)%(refname:short)" refs/heads/
>
> I don't think the #!/bin/sh adds any value here. Just the 'git
> for-each-ref' line is sufficient IMHO.
>
Sure, we can remove that.
>> +An example to show the usage of %(if)...%(then)...%(end).
>> +This adds a red color to authorname, if present
>
> I don't think this is such a good example.
> %(color:red)%(authorname)%(color:reset) just works even if %(authorname)
> is empty.
>
> A better example would be
>
> git for-each-ref --format='%(if)%(authorname)%(then)Authored by %(authorname)%(end)'
>
> which avoids writting "Authored by " with no author.
>
Yeah, will include this.
>> -static int is_empty(const char * s){
>> +static int is_empty(const char *s){
>
> You still have the { on the declaration line, it should be on the next
> line.
>
Oops, will change that.
>> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
>> static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> {
>> struct ref_formatting_stack *cur = state->stack;
>> - struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> + struct if_then_else *if_then_else = NULL;
>>
>> + if (cur->at_end == if_then_else_handler)
>> + if_then_else = (struct if_then_else *)cur->at_end_data;
>
> OK, now the cast is safe since at_end_data has to be of type struct
> if_then_else * if at_end is if_then_else_handler.
>
>> + unsigned int nobracket = 0;
>> +
>> + if (!strcmp(valp, ",nobracket"))
>> + nobracket = 1;
>
> The code to parse comma-separated values is different here and
> elsewhere. I'd rather have the same code (possibly factored into a
> helper function), both to get consistent behavior (you're not allowing
> %(upstream:nobracket,track) for example, right?) and consistent code.
>
Eh, okay, will do that!
>> if (!num_ours && !num_theirs)
>> v->s = "";
>> else if (!num_ours) {
>> - sprintf(buf, "[behind %d]", num_theirs);
>> + if (nobracket)
>> + sprintf(buf, "behind %d", num_theirs);
>> + else
>> + sprintf(buf, "[behind %d]", num_theirs);
>
> Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
> unconditionnally, and set obracket = "" or obracket = "[" once and for
> all when you test for "nobracket" above. This avoids these "if
> (nobracket)" spread accross the code, but at the price of extra %s in
> the format strings.
>
Okay! will do that.
>> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
>> else
>> v->s = "<>";
>> continue;
>> + } else if (!strcmp(formatp, "dir") &&
>> + (starts_with(name, "refname"))) {
>> + const char *sp, *ep, *tmp;
>> +
>> + sp = tmp = ref->refname;
>> + /* Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
>> + do {
>> + ep = tmp;
>> + tmp = strchr(ep + 1, '/');
>> + } while (tmp);
>
> To look for the last occurence of '/' you can also use strrchr().
> Doesn't it do what you want here?
>
Didn't know that, thanks will do that.
>> --- a/t/t6040-tracking-info.sh
>> +++ b/t/t6040-tracking-info.sh
>> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
>> '
>>
>> cat >expect <<\EOF
>> -b1 [origin/master] [ahead 1, behind 1] d
>> -b2 [origin/master] [ahead 1, behind 1] d
>> -b3 [origin/master] [behind 1] b
>> -b4 [origin/master] [ahead 2] f
>> -b5 [brokenbase] [gone] g
>> +b1 [origin/master: ahead 1, behind 1] d
>> +b2 [origin/master: ahead 1, behind 1] d
>> +b3 [origin/master: behind 1] b
>> +b4 [origin/master: ahead 2] f
>> +b5 [brokenbase: gone] g
>> b6 [origin/master] c
>> EOF
>
> Cool!
>
> I didn't go through the patches themselves, but modulo my remarks above
> the interdiff looks good. Thanks.
>
Thanks for the review.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 12:27 ` [PATCH v2 00/10] port branch.c to use ref-filter's printing options Matthieu Moy
2015-10-08 13:17 ` Karthik Nayak
@ 2015-10-08 16:09 ` Karthik Nayak
2015-10-08 17:10 ` Matthieu Moy
1 sibling, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 16:09 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>> + unsigned int nobracket = 0;
>> +
>> + if (!strcmp(valp, ",nobracket"))
>> + nobracket = 1;
>
> The code to parse comma-separated values is different here and
> elsewhere. I'd rather have the same code (possibly factored into a
> helper function), both to get consistent behavior (you're not allowing
> %(upstream:nobracket,track) for example, right?) and consistent code.
>
Speaking of comma-separated values, the only other place we use that is
in the align atom. Also I find this very specific to get a function out of.
Somehow I think this is the simplest way to go about this.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 16:09 ` Karthik Nayak
@ 2015-10-08 17:10 ` Matthieu Moy
2015-10-08 17:28 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 17:10 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> + unsigned int nobracket = 0;
>>> +
>>> + if (!strcmp(valp, ",nobracket"))
>>> + nobracket = 1;
>>
>> The code to parse comma-separated values is different here and
>> elsewhere. I'd rather have the same code (possibly factored into a
>> helper function), both to get consistent behavior (you're not allowing
>> %(upstream:nobracket,track) for example, right?) and consistent code.
>>
>
> Speaking of comma-separated values, the only other place we use that is
> in the align atom. Also I find this very specific to get a function out of.
> Somehow I think this is the simplest way to go about this.
Well, most pieces of code start with one instance, then two, then
more ;-). When the second instance starts being different from the
first, it doesn't give a good example for the future third instance.
This particular piece of code is so important and I won't fight for a
better factored one, but in general "there are only two instances" is a
dubious argument to avoid refactoring.
Still, I find it weird to force the nobracket to be always at the same
position.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 17:10 ` Matthieu Moy
@ 2015-10-08 17:28 ` Karthik Nayak
2015-10-08 18:26 ` Matthieu Moy
2015-10-08 18:35 ` Junio C Hamano
0 siblings, 2 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-08 17:28 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Git, Christian Couder, Junio C Hamano
On Thu, Oct 8, 2015 at 10:40 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>> + unsigned int nobracket = 0;
>>>> +
>>>> + if (!strcmp(valp, ",nobracket"))
>>>> + nobracket = 1;
>>>
>>> The code to parse comma-separated values is different here and
>>> elsewhere. I'd rather have the same code (possibly factored into a
>>> helper function), both to get consistent behavior (you're not allowing
>>> %(upstream:nobracket,track) for example, right?) and consistent code.
>>>
>>
>> Speaking of comma-separated values, the only other place we use that is
>> in the align atom. Also I find this very specific to get a function out of.
>> Somehow I think this is the simplest way to go about this.
>
> Well, most pieces of code start with one instance, then two, then
> more ;-). When the second instance starts being different from the
> first, it doesn't give a good example for the future third instance.
>
Totally agree with you here.
> This particular piece of code is so important and I won't fight for a
> better factored one, but in general "there are only two instances" is a
> dubious argument to avoid refactoring.
>
> Still, I find it weird to force the nobracket to be always at the same
> position.
>
No i mean I could follow up with the way we use it in align, but I don't see
how I can make a function out of this.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 17:28 ` Karthik Nayak
@ 2015-10-08 18:26 ` Matthieu Moy
2015-10-08 18:35 ` Junio C Hamano
1 sibling, 0 replies; 66+ messages in thread
From: Matthieu Moy @ 2015-10-08 18:26 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git, Christian Couder, Junio C Hamano
Karthik Nayak <karthik.188@gmail.com> writes:
> On Thu, Oct 8, 2015 at 10:40 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> This particular piece of code is so important and I won't fight for a
>> better factored one, but in general "there are only two instances" is a
>> dubious argument to avoid refactoring.
>>
>> Still, I find it weird to force the nobracket to be always at the same
>> position.
>>
>
> No i mean I could follow up with the way we use it in align, but I don't see
> how I can make a function out of this.
Ah, OK. Well, there are already two instances of
/* Strip trailing comma */
if (s[1])
strbuf_setlen(s[0], s[0]->len - 1);
(for objectname and align), one of which says
/*
* TODO: Implement a function similar to strbuf_split_str()
* which would omit the separator from the end of each value.
*/
I guess it's time to replace this TODO with actual code. A
straightforward implementation would be to call strbuf_split_str and
then use the same "if (s[1])" as you already have. There's probably a
more efficient way.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 17:28 ` Karthik Nayak
2015-10-08 18:26 ` Matthieu Moy
@ 2015-10-08 18:35 ` Junio C Hamano
2015-10-09 8:22 ` Matthieu Moy
1 sibling, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-08 18:35 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> No i mean I could follow up with the way we use it in align, but I don't see
> how I can make a function out of this.
At least you should be able to pre-parse the %(<atom>:<modifier>)
construct, instead of doing strcmp() every time populate_value() is
called, no? Then your parser would not be doing
>>>> + if (!strcmp(valp, ",nobracket"))
>>>> + nobracket = 1;
with 'valp' that scans over 'name' (which is an element of
used_atom[], i.e. a name from valid_atom[] followed by ":<modifier>"
for each customization). Instead your used_atom[] would be an array
of a data structure that is richer than just a string.
The <modifier> would be split at comma boundary and made into an
array of two field structure, as the most general form of it is
<modifier> ::= <attrval> | <attrval> ',' <modifier>
<attrval> ::= <attr> '=' <value> | <attr> | <value>
if we recall the discussion we had while designing %(align), i.e.
* The most general form is "%(align:position=left,width=32)"
* The value domain of attributes may be distinct, in which case the
value itself could imply what attr it is talking about,
e.g. "%(align:32,left)" is sufficiently clear as '32' cannot be
position and 'left' cannot be width.
And clearly there can be an attr whose presense alone can imply its
value (iow, a boolean), even though your %(align) does not yet have
such a modifier. It is easy to imagine that you would later want to
add %(align:position=left,width=32,truncate) that truncates the value
when its display width exceeds '32'. The 'nobracket' above would be
an another example of a boolean (whose name is negative, which is
not a very good design in general, though).
Then used_atom[] could become something like
struct {
const char *str; /* e.g. "align:position=left,32" */
struct {
const char *part0; /* everything before '=' */
const char *part1; /* optional */
} *modifier;
int modifier_nr;
} *used_atom;
and "align:position=left,32" would be parsed into
.str = "align:position=left,32";
.modifier = { { "position", "left" }, { "32", NULL } };
.modifier_nr = 2;
when the format string is read, which is done only once.
The looping over all the refs done in populate_value() could just
use the pre-parsed representation.
Hmm?
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-08 18:35 ` Junio C Hamano
@ 2015-10-09 8:22 ` Matthieu Moy
2015-10-09 18:29 ` Junio C Hamano
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-09 8:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git, Christian Couder
Junio C Hamano <gitster@pobox.com> writes:
> Then used_atom[] could become something like
>
> struct {
> const char *str; /* e.g. "align:position=left,32" */
> struct {
> const char *part0; /* everything before '=' */
> const char *part1; /* optional */
> } *modifier;
> int modifier_nr;
> } *used_atom;
If the goal is to prepare as much as possible when parsing the format
string, I'd even push it one step further and have stg like
struct {
const char *str; /* e.g. "align:position=left,32" */
union {
struct {
int position;
enum { left, right, center } kind;
} align;
struct {
....;
} objectname;
int modifier_nr;
} *used_atom;
Just a thought, I'm not sure how useful this would be, and this may be
too much change for this series (so it may deserve a separate topic).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-09 8:22 ` Matthieu Moy
@ 2015-10-09 18:29 ` Junio C Hamano
2015-10-11 12:48 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-09 18:29 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:
>
>> Then used_atom[] could become something like
>>
>> struct {
>> const char *str; /* e.g. "align:position=left,32" */
>> struct {
>> const char *part0; /* everything before '=' */
>> const char *part1; /* optional */
>> } *modifier;
>> int modifier_nr;
>> } *used_atom;
>
> If the goal is to prepare as much as possible when parsing the format
> string, I'd even push it one step further and have stg like
>
> struct {
> const char *str; /* e.g. "align:position=left,32" */
> union {
> struct {
> int position;
> enum { left, right, center } kind;
> } align;
> struct {
> ....;
> } objectname;
> int modifier_nr;
> } *used_atom;
>
> Just a thought, I'm not sure how useful this would be, and this may be
> too much change for this series (so it may deserve a separate topic).
Yes, if we are willing to enrich the element of valid_atom[] array
with a type-specific parsing functions, we could even do that. Then
populate_value() would not have to do any parsing and just do the
filling.
I was shooting for a middle ground, but certainly with an eye
towards such an endgame state in the future.
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-09 18:29 ` Junio C Hamano
@ 2015-10-11 12:48 ` Karthik Nayak
2015-10-11 16:21 ` Matthieu Moy
2015-10-12 0:36 ` Junio C Hamano
0 siblings, 2 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 12:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, Git, Christian Couder
On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Then used_atom[] could become something like
>>>
>>> struct {
>>> const char *str; /* e.g. "align:position=left,32" */
>>> struct {
>>> const char *part0; /* everything before '=' */
>>> const char *part1; /* optional */
>>> } *modifier;
>>> int modifier_nr;
>>> } *used_atom;
>>
>> If the goal is to prepare as much as possible when parsing the format
>> string, I'd even push it one step further and have stg like
>>
>> struct {
>> const char *str; /* e.g. "align:position=left,32" */
>> union {
>> struct {
>> int position;
>> enum { left, right, center } kind;
>> } align;
>> struct {
>> ....;
>> } objectname;
>> int modifier_nr;
>> } *used_atom;
>>
>> Just a thought, I'm not sure how useful this would be, and this may be
>> too much change for this series (so it may deserve a separate topic).
>
> Yes, if we are willing to enrich the element of valid_atom[] array
> with a type-specific parsing functions, we could even do that. Then
> populate_value() would not have to do any parsing and just do the
> filling.
>
> I was shooting for a middle ground, but certainly with an eye
> towards such an endgame state in the future.
I like the idea's here, I was trying out what you suggested before
Matthieu suggestion.
We could have:
static void parse_atom_modifiers(struct used_atom *atom)
{
const char *sp = NULL;
atom->modifier = NULL;
atom->modifier_nr = 0;
if ((sp = strchr(atom->str, ':'))) {
while (sp[1]) {
const char *equal, *comma, *ep;
int no = atom->modifier_nr;
atom->modifier_nr++;
sp++;
REALLOC_ARRAY(atom->modifier, atom->modifier_nr);
equal = strchr(sp, '=');
comma = strchr(sp, ',');
if (comma)
ep = comma;
else
ep = sp + strlen(sp);
if (!equal) {
atom->modifier[no].part0 = xstrndup(sp, ep - sp);
atom->modifier[no].part1 = NULL;
} else {
atom->modifier[no].part0 = xstrndup(sp, equal - sp);
atom->modifier[no].part1 = xstrndup(equal + 1, ep - equal - 1);
}
sp = ep;
}
}
}
or something on these lines for what you suggested. We could improve by
having a special parsing function for selected atoms and leave this to
be default.
Also does it make sense to integrate these changes here? Or would you like to
have another series on this?
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-11 12:48 ` Karthik Nayak
@ 2015-10-11 16:21 ` Matthieu Moy
2015-10-11 18:38 ` Karthik Nayak
2015-10-12 0:36 ` Junio C Hamano
1 sibling, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 16:21 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> Also does it make sense to integrate these changes here? Or would you like to
> have another series on this?
To me, the important in this series is to avoid introducing duplicated
and inconsistent code, because it would make further refactoring harder.
But this series starts getting close to finished, so you should not try
to add to much to it (at least for me: I spend some time reviewing v2
and I have an idea of what the interdiff should look like, I'd rather
avoid having new distraction in the v2->v3 interdiff).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-11 16:21 ` Matthieu Moy
@ 2015-10-11 18:38 ` Karthik Nayak
2015-10-11 19:32 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 18:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Git, Christian Couder
On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Also does it make sense to integrate these changes here? Or would you like to
>> have another series on this?
>
> To me, the important in this series is to avoid introducing duplicated
> and inconsistent code, because it would make further refactoring harder.
>
Would you suggest duplicating whats done with %(align) here?
> But this series starts getting close to finished, so you should not try
> to add to much to it (at least for me: I spend some time reviewing v2
> and I have an idea of what the interdiff should look like, I'd rather
> avoid having new distraction in the v2->v3 interdiff).
>
Yes! makes sense, thanks :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-11 18:38 ` Karthik Nayak
@ 2015-10-11 19:32 ` Matthieu Moy
2015-10-11 19:38 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-11 19:32 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Also does it make sense to integrate these changes here? Or would you like to
>>> have another series on this?
>>
>> To me, the important in this series is to avoid introducing duplicated
>> and inconsistent code, because it would make further refactoring harder.
>>
>
> Would you suggest duplicating whats done with %(align) here?
I think introducing a function to split according to commas and remove
commas would make sense, but I won't insist on that. Mimicking what's
done with %(align) is acceptable to me: we'll have several instances of
the same pattern, not ideal but easy enough to refactor later.
Especially if you actually plan to work on that :-).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-11 19:32 ` Matthieu Moy
@ 2015-10-11 19:38 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-11 19:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Git, Christian Couder
On Mon, Oct 12, 2015 at 1:02 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Also does it make sense to integrate these changes here? Or would you like to
>>>> have another series on this?
>>>
>>> To me, the important in this series is to avoid introducing duplicated
>>> and inconsistent code, because it would make further refactoring harder.
>>>
>>
>> Would you suggest duplicating whats done with %(align) here?
>
> I think introducing a function to split according to commas and remove
> commas would make sense, but I won't insist on that. Mimicking what's
> done with %(align) is acceptable to me: we'll have several instances of
> the same pattern, not ideal but easy enough to refactor later.
> Especially if you actually plan to work on that :-).
>
I was planning on working on what Junio and you suggested after this ;-).
So I didn't see the need of " introducing a function to split
according to commas"
if we plan to rewrite the whole parsing part of ref-filter.
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-11 12:48 ` Karthik Nayak
2015-10-11 16:21 ` Matthieu Moy
@ 2015-10-12 0:36 ` Junio C Hamano
2015-10-12 17:48 ` Karthik Nayak
1 sibling, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-12 0:36 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
> Also does it make sense to integrate these changes here? Or would you like to
> have another series on this?
I do not think you would want to ask that question, as my answer
would most likely be "The most preferable would be a series to clean
up the existing codepath that deals with %(align) first, on top of
which everything in flight that is not yet in 'next' is rebased."
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-12 0:36 ` Junio C Hamano
@ 2015-10-12 17:48 ` Karthik Nayak
2015-10-12 18:17 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-12 17:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matthieu Moy, Git, Christian Couder
On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> Also does it make sense to integrate these changes here? Or would you like to
>> have another series on this?
>
> I do not think you would want to ask that question, as my answer
> would most likely be "The most preferable would be a series to clean
> up the existing codepath that deals with %(align) first, on top of
> which everything in flight that is not yet in 'next' is rebased."
>
Ah, but I might take a while to get there, So I'd rather push code which
is almost ready and work on that slowly, if that's ok?
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-12 17:48 ` Karthik Nayak
@ 2015-10-12 18:17 ` Matthieu Moy
2015-10-12 18:59 ` Junio C Hamano
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-12 18:17 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> ...
>>> Also does it make sense to integrate these changes here? Or would you like to
>>> have another series on this?
>>
>> I do not think you would want to ask that question, as my answer
>> would most likely be "The most preferable would be a series to clean
>> up the existing codepath that deals with %(align) first, on top of
>> which everything in flight that is not yet in 'next' is rebased."
>
> Ah, but I might take a while to get there, So I'd rather push code which
> is almost ready and work on that slowly, if that's ok?
That's OK to me. The "most preferable way" above would lead to a cleaner
history, but also more work for you and for me as a reviewer.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-12 18:17 ` Matthieu Moy
@ 2015-10-12 18:59 ` Junio C Hamano
2015-10-12 19:07 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-12 18:59 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Karthik Nayak, Git, Christian Couder
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> ...
>>>> Also does it make sense to integrate these changes here? Or would you like to
>>>> have another series on this?
>>>
>>> I do not think you would want to ask that question, as my answer
>>> would most likely be "The most preferable would be a series to clean
>>> up the existing codepath that deals with %(align) first, on top of
>>> which everything in flight that is not yet in 'next' is rebased."
>>
>> Ah, but I might take a while to get there, So I'd rather push code which
>> is almost ready and work on that slowly, if that's ok?
>
> That's OK to me. The "most preferable way" above would lead to a cleaner
> history, but also more work for you and for me as a reviewer.
I do not think the cleanliness of the resulting history is of prime
concern. At least, that was not where my preference came from.
If you design a new infrastructure to help refactoring early
(i.e. before adding many copies of code that need to be cleaned up
later), it would make the work of reviewing of the design of the
helper and refactoring using that helper smaller, not larger. And
the new codepaths that use the helper would become easier to follow
(otherwise we wouldn't be doing such a refactoring in the first
place), making the reviews easier. That is where my preference
comes from.
There _is_ a sunk-cost that has already been invested in reviewing
the older round that added code that needs to be cleaned up; there
are some parts other than the "need to be cleaned-up" parts that we
would feel confortable having in the reroll without having to
re-review them. I do not know if that is a very high cost, though,
especially for those who have already seen the previous rounds.
Anyway, I wouldn't insist and we can go either way.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-12 18:59 ` Junio C Hamano
@ 2015-10-12 19:07 ` Matthieu Moy
2015-10-18 19:07 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-12 19:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git, Christian Couder
Junio C Hamano <gitster@pobox.com> writes:
> If you design a new infrastructure to help refactoring early
> (i.e. before adding many copies of code that need to be cleaned up
> later), it would make the work of reviewing of the design of the
> helper and refactoring using that helper smaller, not larger.
But most of the code concerned is already reviewed. The first instances
of the pattern to refactor is already in next. With a real time machine,
we could go back in past, refactor and then have cleaner series, but
with Git as our only tool we can't ;-).
The current series will just add one more instance of sub-optimal code,
it isn't hard to review. Inserting new code before them would make the
interdiff far bigger.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-12 19:07 ` Matthieu Moy
@ 2015-10-18 19:07 ` Karthik Nayak
2015-10-19 4:44 ` Junio C Hamano
0 siblings, 1 reply; 66+ messages in thread
From: Karthik Nayak @ 2015-10-18 19:07 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Git, Christian Couder
On Tue, Oct 13, 2015 at 12:37 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you design a new infrastructure to help refactoring early
>> (i.e. before adding many copies of code that need to be cleaned up
>> later), it would make the work of reviewing of the design of the
>> helper and refactoring using that helper smaller, not larger.
>
> But most of the code concerned is already reviewed. The first instances
> of the pattern to refactor is already in next. With a real time machine,
> we could go back in past, refactor and then have cleaner series, but
> with Git as our only tool we can't ;-).
>
> The current series will just add one more instance of sub-optimal code,
> it isn't hard to review. Inserting new code before them would make the
> interdiff far bigger.
>
Sorry for the delay, was a bit busy with college work. For the most
part I've been
trying to integrate the %(upstream) and its options 'track',
'trachshort' and 'short'
so we could implement %(upstream:track,nobracket) or
%(upstream:nobracket,track).
While doing so I realized I was moving a lot of code around and this
had me thinking it's
perhaps easier to do the cleaning up first as Junio suggested.
Maybe then it'd be simpler to do implement this rather than move code around now
and then move code around when we implement the parsing methods we
spoke about earlier?
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-18 19:07 ` Karthik Nayak
@ 2015-10-19 4:44 ` Junio C Hamano
2015-10-19 8:12 ` Matthieu Moy
0 siblings, 1 reply; 66+ messages in thread
From: Junio C Hamano @ 2015-10-19 4:44 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Matthieu Moy, Git, Christian Couder
Karthik Nayak <karthik.188@gmail.com> writes:
> ... While doing so I realized I was moving a lot of code around
> and this had me thinking it's perhaps easier to do the cleaning up
> first as Junio suggested.
>
> Maybe then it'd be simpler to do implement this rather than move
> code around now and then move code around when we implement the
> parsing methods we spoke about earlier?
When I suggested the approach, I hadn't have done any of what you
actually did, so I was primarily talking out of "hunch" (perhaps
that is what some people call "taste") but I expected something like
that may happen ;-).
Right now, you practically own the topic, in the sense that nobody
seems to be in a very urgent need to compete with you in parallel to
implement what we have discussed before you do, so I personally
would suggest whichever order you feel more comfortable and less
error-prone.
Thanks.
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-19 4:44 ` Junio C Hamano
@ 2015-10-19 8:12 ` Matthieu Moy
2015-10-20 20:53 ` Karthik Nayak
0 siblings, 1 reply; 66+ messages in thread
From: Matthieu Moy @ 2015-10-19 8:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git, Christian Couder
Junio C Hamano <gitster@pobox.com> writes:
> I personally would suggest whichever order you feel more comfortable
> and less error-prone.
This is a good summary, and I fully agree with it.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 66+ messages in thread
* Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options
2015-10-19 8:12 ` Matthieu Moy
@ 2015-10-20 20:53 ` Karthik Nayak
0 siblings, 0 replies; 66+ messages in thread
From: Karthik Nayak @ 2015-10-20 20:53 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, Git, Christian Couder
On Mon, Oct 19, 2015 at 1:42 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I personally would suggest whichever order you feel more comfortable
>> and less error-prone.
>
> This is a good summary, and I fully agree with it.
Well then, I'm working on the parsing part of it before this series :)
--
Regards,
Karthik Nayak
^ permalink raw reply [flat|nested] 66+ messages in thread