Re: [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'
Karthik Nayak karthik@gmail.com writes:
+static void ref_formatting(struct ref_formatting_state *state,
+struct atom_value *v, struct strbuf *value)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ strbuf_addf(value, %s, v-s);
+}
You're taking 'state' as argument, but you're not using it in the
function for now. Perhaps add a temporary comment like:
static void ref_formatting(...)
{
/* Formatting according to 'state' will be applied here */
strbuf_addf(...)
}
Or perhaps it's OK like this.
-static void print_value(struct atom_value *v, int quote_style)
+static void print_value(struct ref_formatting_state *state, struct
atom_value *v)
Changing the position of the v parameter makes the patch a bit harder to
read. I would have written in this order:
static void print_value(struct atom_value *v, struct ref_formatting_state
*state)
So the patch reads as encapsulate quote_style in a struct more
straightforwardly.
@@ -1257,6 +1269,10 @@ static void emit(const char *cp, const char *ep)
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
+ struct ref_formatting_state state;
I still found it a bit hard to read, and I would have appreciated a
comment here, like
/*
* Some (pseudo) atom have no immediate side effect, but only
* affect the next atom. Store the relevant information from
* these atoms in the 'state' variable for use when displaying
* the next atom.
*/
With this in mind, it becomes more obvious that you also need to reset
the state after using it, which you forgot to do. See:
$ ./git for-each-ref --format '%(padright:30)|%(refname)|%(refname)|'
refs/tags/v2.4.\*
|refs/tags/v2.4.0 |refs/tags/v2.4.0 |
|refs/tags/v2.4.0-rc0 |refs/tags/v2.4.0-rc0 |
|refs/tags/v2.4.0-rc1 |refs/tags/v2.4.0-rc1 |
|refs/tags/v2.4.0-rc2 |refs/tags/v2.4.0-rc2 |
|refs/tags/v2.4.0-rc3 |refs/tags/v2.4.0-rc3 |
|refs/tags/v2.4.1 |refs/tags/v2.4.1 |
|refs/tags/v2.4.2 |refs/tags/v2.4.2 |
|refs/tags/v2.4.3 |refs/tags/v2.4.3 |
|refs/tags/v2.4.4 |refs/tags/v2.4.4 |
|refs/tags/v2.4.5 |refs/tags/v2.4.5 |
|refs/tags/v2.4.6 |refs/tags/v2.4.6 |
I think only the first column should have padding, not the second. You
can fix this with a patch like this:
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1431,6 +1431,14 @@ static void apply_pseudo_state(struct
ref_formatting_state *state,
state-ifexists = v-s;
}
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
+}
+
+
/*
* If 'lines' is greater than 0, print that many lines from the given
* object_id 'oid'.
@@ -1492,8 +1500,11 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format,
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-pseudo_atom)
apply_pseudo_state(state, atomv);
- else
+ else {
print_value(state, atomv);
+ reset_formatting_state(state);
+ }
+
}
if (*cp) {
sp = cp + strlen(cp);
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'
On Mon, Jul 27, 2015 at 6:12 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Karthik Nayak karthik@gmail.com writes:
+static void ref_formatting(struct ref_formatting_state *state,
+struct atom_value *v, struct strbuf *value)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ strbuf_addf(value, %s, v-s);
+}
You're taking 'state' as argument, but you're not using it in the
function for now. Perhaps add a temporary comment like:
static void ref_formatting(...)
{
/* Formatting according to 'state' will be applied here */
strbuf_addf(...)
}
Or perhaps it's OK like this.
I thought it'd be OK since it doesn't have any adverse effect, but I added
the comment you suggested nonetheless.
-static void print_value(struct atom_value *v, int quote_style)
+static void print_value(struct ref_formatting_state *state, struct
atom_value *v)
Changing the position of the v parameter makes the patch a bit harder to
read. I would have written in this order:
static void print_value(struct atom_value *v, struct ref_formatting_state
*state)
So the patch reads as encapsulate quote_style in a struct more
straightforwardly.
I need to be more careful about things like this, thanks.
@@ -1257,6 +1269,10 @@ static void emit(const char *cp, const char *ep)
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
+ struct ref_formatting_state state;
I still found it a bit hard to read, and I would have appreciated a
comment here, like
/*
* Some (pseudo) atom have no immediate side effect, but only
* affect the next atom. Store the relevant information from
* these atoms in the 'state' variable for use when displaying
* the next atom.
*/
This seems good, will add this.
With this in mind, it becomes more obvious that you also need to reset
the state after using it, which you forgot to do. See:
$ ./git for-each-ref --format '%(padright:30)|%(refname)|%(refname)|'
refs/tags/v2.4.\*
|refs/tags/v2.4.0 |refs/tags/v2.4.0 |
|refs/tags/v2.4.0-rc0 |refs/tags/v2.4.0-rc0 |
|refs/tags/v2.4.0-rc1 |refs/tags/v2.4.0-rc1 |
|refs/tags/v2.4.0-rc2 |refs/tags/v2.4.0-rc2 |
|refs/tags/v2.4.0-rc3 |refs/tags/v2.4.0-rc3 |
|refs/tags/v2.4.1 |refs/tags/v2.4.1 |
|refs/tags/v2.4.2 |refs/tags/v2.4.2 |
|refs/tags/v2.4.3 |refs/tags/v2.4.3 |
|refs/tags/v2.4.4 |refs/tags/v2.4.4 |
|refs/tags/v2.4.5 |refs/tags/v2.4.5 |
|refs/tags/v2.4.6 |refs/tags/v2.4.6 |
I think only the first column should have padding, not the second. You
can fix this with a patch like this:
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1431,6 +1431,14 @@ static void apply_pseudo_state(struct
ref_formatting_state *state,
state-ifexists = v-s;
}
+static void reset_formatting_state(struct ref_formatting_state *state)
+{
+ int quote_style = state-quote_style;
+ memset(state, 0, sizeof(*state));
+ state-quote_style = quote_style;
+}
+
+
/*
* If 'lines' is greater than 0, print that many lines from the given
* object_id 'oid'.
@@ -1492,8 +1500,11 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format,
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
if (atomv-pseudo_atom)
apply_pseudo_state(state, atomv);
- else
+ else {
print_value(state, atomv);
+ reset_formatting_state(state);
+ }
+
}
if (*cp) {
sp = cp + strlen(cp);
If you saw the last patch series, i had them reset individually, I
missed that by mistake.
But what you're doing seems good, will integrate this. thanks :D
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'
Introduce 'ref_formatting' structure to hold values of pseudo atoms
which help only in formatting. This will eventually be used by atoms
like `color` and the `padright` atom which will be introduced in a
later patch.
Helped-by: Junio C Hamano gits...@pobox.com
Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
ref-filter.c | 46 +++---
ref-filter.h | 5 +
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 7561727..a3625e8 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,8 @@
#include quote.h
#include ref-filter.h
#include revision.h
+#include utf8.h
+#include git-compat-util.h
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -620,7 +622,7 @@ static void populate_value(struct ref_array_item *ref)
const char *name = used_atom[i];
struct atom_value *v = ref-value[i];
int deref = 0;
- const char *refname;
+ const char *refname = NULL;
const char *formatp;
struct branch *branch = NULL;
@@ -1190,30 +1192,40 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
qsort(array-items, array-nr, sizeof(struct ref_array_item *),
compare_refs);
}
-static void print_value(struct atom_value *v, int quote_style)
+static void ref_formatting(struct ref_formatting_state *state,
+ struct atom_value *v, struct strbuf *value)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ strbuf_addf(value, %s, v-s);
+}
+
+static void print_value(struct ref_formatting_state *state, struct atom_value
*v)
+{
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf formatted = STRBUF_INIT;
+
+ ref_formatting(state, v, value);
+
+ switch (state-quote_style) {
case QUOTE_NONE:
- fputs(v-s, stdout);
+ fputs(value.buf, stdout);
break;
case QUOTE_SHELL:
- sq_quote_buf(sb, v-s);
+ sq_quote_buf(formatted, value.buf);
break;
case QUOTE_PERL:
- perl_quote_buf(sb, v-s);
+ perl_quote_buf(formatted, value.buf);
break;
case QUOTE_PYTHON:
- python_quote_buf(sb, v-s);
+ python_quote_buf(formatted, value.buf);
break;
case QUOTE_TCL:
- tcl_quote_buf(sb, v-s);
+ tcl_quote_buf(formatted, value.buf);
break;
}
- if (quote_style != QUOTE_NONE) {
- fputs(sb.buf, stdout);
- strbuf_release(sb);
- }
+ if (state-quote_style != QUOTE_NONE)
+ fputs(formatted.buf, stdout);
+ strbuf_release(formatted);
+ strbuf_release(value);
}
static int hex1(char ch)
@@ -1257,6 +1269,10 @@ static void emit(const char *cp, const char *ep)
void show_ref_array_item(struct ref_array_item *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
+ struct ref_formatting_state state;
+
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
@@ -1265,7 +1281,7 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (cp sp)
emit(cp, sp);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style);
+ print_value(state, atomv);
}
if (*cp) {
sp = cp + strlen(cp);
@@ -1278,7 +1294,7 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (color_parse(reset, color) 0)
die(BUG: couldn't parse 'reset' as a color);
resetv.s = color;
- print_value(resetv, quote_style);
+ print_value(state, resetv);
}
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..f24e3ef 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -19,6 +19,11 @@
struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
+ unsigned int pseudo_atom : 1; /* atoms which aren't placeholders for
ref attributes */
+};
+
+struct ref_formatting_state {
+ int quote_style;
};
struct ref_sorting {
--
2.4.6
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
