* [PATCH v2 4/5] ref-filter: add return value to parsers
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
@ 2018-03-14 19:04 ` Olga Telezhnaya
2018-03-14 19:04 ` [PATCH v2 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
` (3 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-14 19:04 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of parsers by adding return value and
strbuf parameter for error message.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 118 insertions(+), 59 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index dd83ef326511d..62ea4adcd0ff1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,28 @@ static struct used_atom {
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *color_value, struct strbuf *err)
{
- if (!color_value)
- die(_("expected format: %%(color:<color>)"));
- if (color_parse(color_value, atom->u.color) < 0)
- die(_("unrecognized color: %%(color:%s)"), color_value);
+ if (!color_value) {
+ strbuf_addstr(err, _("expected format: %(color:<color>)"));
+ return -1;
+ }
+ if (color_parse(color_value, atom->u.color) < 0) {
+ strbuf_addf(err, _("unrecognized color: %%(color:%s)"), color_value);
+ return -1;
+ }
/*
* We check this after we've parsed the color, which lets us complain
* about syntactically bogus color names even if they won't be used.
*/
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+ return 0;
}
-static void refname_atom_parser_internal(struct refname_atom *atom,
- const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,
+ const char *name, struct strbuf *err)
{
if (!arg)
atom->option = R_NORMAL;
@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
else if (skip_prefix(arg, "lstrip=", &arg) ||
skip_prefix(arg, "strip=", &arg)) {
atom->option = R_LSTRIP;
- if (strtol_i(arg, 10, &atom->lstrip))
- die(_("Integer value expected refname:lstrip=%s"), arg);
+ if (strtol_i(arg, 10, &atom->lstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:lstrip=%s"), arg);
+ return -1;
+ }
} else if (skip_prefix(arg, "rstrip=", &arg)) {
atom->option = R_RSTRIP;
- if (strtol_i(arg, 10, &atom->rstrip))
- die(_("Integer value expected refname:rstrip=%s"), arg);
- } else
- die(_("unrecognized %%(%s) argument: %s"), name, arg);
+ if (strtol_i(arg, 10, &atom->rstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:rstrip=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, arg);
+ return -1;
+ }
+ return 0;
}
-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
if (!arg) {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
- return;
+ return refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err);
}
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
+ if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err))
+ return -1;
}
}
string_list_clear(¶ms, 0);
+ return 0;
}
-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(body) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(body) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_BODY_DEP;
+ return 0;
}
-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(subject) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(subject) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_SUB;
+ return 0;
}
-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.trailer_opts.unfold = 1;
else if (!strcmp(s, "only"))
atom->u.contents.trailer_opts.only_trailers = 1;
- else
- die(_("unknown %%(trailers) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
+ return -1;
+ }
}
}
atom->u.contents.option = C_TRAILERS;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.contents.option = C_BARE;
@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.option = C_SUB;
else if (skip_prefix(arg, "trailers", &arg)) {
skip_prefix(arg, ":", &arg);
- trailers_atom_parser(format, atom, *arg ? arg : NULL);
+ if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+ return -1;
} else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
- if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
- die(_("positive value expected contents:lines=%s"), arg);
- } else
- die(_("unrecognized %%(contents) argument: %s"), arg);
+ if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {
+ strbuf_addf(err, _("positive value expected contents:lines=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(contents) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.objectname.option = O_FULL;
@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
else if (skip_prefix(arg, "short=", &arg)) {
atom->u.objectname.option = O_LENGTH;
if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
- atom->u.objectname.length == 0)
- die(_("positive value expected objectname:short=%s"), arg);
+ atom->u.objectname.length == 0) {
+ strbuf_addf(err, _("positive value expected objectname:short=%s"), arg);
+ return -1;
+ }
if (atom->u.objectname.length < MINIMUM_ABBREV)
atom->u.objectname.length = MINIMUM_ABBREV;
- } else
- die(_("unrecognized %%(objectname) argument: %s"), arg);
+ } else {
+ strbuf_addf(err, _("unrecognized %%(objectname) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
+ return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
}
static align_type parse_align_position(const char *s)
@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)
return -1;
}
-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct align *align = &atom->u.align;
struct string_list params = STRING_LIST_INIT_DUP;
int i;
unsigned int width = ~0U;
- if (!arg)
- die(_("expected format: %%(align:<width>,<position>)"));
+ if (!arg) {
+ strbuf_addstr(err, _("expected format: %(align:<width>,<position>)"));
+ return -1;
+ }
align->position = ALIGN_LEFT;
@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
if (skip_prefix(s, "position=", &s)) {
position = parse_align_position(s);
- if (position < 0)
- die(_("unrecognized position:%s"), s);
+ if (position < 0) {
+ strbuf_addf(err, _("unrecognized position:%s"), s);
+ return -1;
+ }
align->position = position;
} else if (skip_prefix(s, "width=", &s)) {
- if (strtoul_ui(s, 10, &width))
- die(_("unrecognized width:%s"), s);
+ if (strtoul_ui(s, 10, &width)) {
+ strbuf_addf(err, _("unrecognized width:%s"), s);
+ return -1;
+ }
} else if (!strtoul_ui(s, 10, &width))
;
else if ((position = parse_align_position(s)) >= 0)
align->position = position;
- else
- die(_("unrecognized %%(align) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unrecognized %%(align) argument: %s"), s);
+ return -1;
+ }
}
- if (width == ~0U)
- die(_("positive width expected with the %%(align) atom"));
+ if (width == ~0U) {
+ strbuf_addstr(err, _("positive width expected with the %(align) atom"));
+ return -1;
+ }
align->width = width;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg) {
atom->u.if_then_else.cmp_status = COMPARE_NONE;
- return;
+ return 0;
} else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
} else {
- die(_("unrecognized %%(if) argument: %s"), arg);
+ strbuf_addf(err, _("unrecognized %%(if) argument: %s"), arg);
+ return -1;
}
+ return 0;
}
-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
+ return 0;
}
static struct {
const char *name;
cmp_type cmp_type;
- void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
+ int (*parser)(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
@@ -459,8 +518,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
}
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
- if (valid_atom[i].parser)
- valid_atom[i].parser(format, &used_atom[at], arg);
+ if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
+ return -1;
if (*atom == '*')
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 2/5] ref-filter: add return value && strbuf to handlers
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
2018-03-14 19:04 ` [PATCH v2 4/5] ref-filter: add return value to parsers Olga Telezhnaya
@ 2018-03-14 19:04 ` Olga Telezhnaya
2018-03-14 19:04 ` [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
` (2 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-14 19:04 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of handlers by adding return value
and strbuf parameter for errors.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 48 insertions(+), 23 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 54fae00bdd410..d120360104806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
+ int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
};
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
}
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
/*
* Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
quote_formatting(&state->stack->output, v->s, state->quote_style);
else
strbuf_addstr(&state->stack->output, v->s);
+ return 0;
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)
strbuf_release(&s);
}
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = &atomv->atom->u.align;
+ return 0;
}
static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
free(if_then_else);
}
-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+ return 0;
}
static int is_empty(const char *s)
@@ -596,19 +603,24 @@ static int is_empty(const char *s)
return 1;
}
-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(then) atom used more than once"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(then) atom used after %%(else)"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(then) atom used without an %(if) atom"));
+ return -1;
+ } else if (if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used more than once"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used after %(else)"));
+ return -1;
+ }
if_then_else->then_atom_seen = 1;
/*
* If the 'equals' or 'notequals' attribute is used then
@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
+ return 0;
}
-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(else) atom used without a %%(then) atom"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(else) atom used more than once"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(else) atom used without an %(if) atom"));
+ return -1;
+ } else if (!if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used without a %(then) atom"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used more than once"));
+ return -1;
+ }
if_then_else->else_atom_seen = 1;
push_stack_element(&state->stack);
state->stack->at_end_data = prev->at_end_data;
state->stack->at_end = prev->at_end;
+ return 0;
}
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *current = state->stack;
struct strbuf s = STRBUF_INIT;
- if (!current->at_end)
- die(_("format: %%(end) atom used without corresponding atom"));
+ if (!current->at_end) {
+ strbuf_addstr(err, _("format: %(end) atom used without corresponding atom"));
+ return -1;
+ }
current->at_end(&state->stack);
/* Stack may have been popped within at_end(), hence reset the current pointer */
@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
}
strbuf_release(&s);
pop_stack_element(&state->stack);
+ return 0;
}
/*
@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,
get_ref_atom_value(info,
parse_ref_filter_atom(format, sp + 2, ep),
&atomv);
- atomv->handler(atomv, &state);
+ if (atomv->handler(atomv, &state, error_buf))
+ return -1;
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (format->need_color_reset_at_eol) {
struct atom_value resetv;
resetv.s = GIT_COLOR_RESET;
- append_atom(&resetv, &state);
+ if (append_atom(&resetv, &state, error_buf))
+ return -1;
}
if (state.stack->prev) {
strbuf_addstr(error_buf, _("format: %(end) atom missing"));
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
2018-03-14 19:04 ` [PATCH v2 4/5] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-14 19:04 ` [PATCH v2 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-14 19:04 ` Olga Telezhnaya
2018-03-15 20:47 ` Martin Ågren
2018-03-14 19:04 ` [PATCH v2 3/5] ref-filter: change parsing function " Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-14 19:04 UTC (permalink / raw)
To: git
Finish removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
It's important to mention that grab_objectname() returned 1 if
it gets objectname atom and 0 otherwise. Now this logic changed:
we return 0 if we have no error, -1 otherwise. If someone needs to
know whether it's objectname atom or not, he/she could use
starts_with() function. It duplicates this checking but it does not
sound like a really big overhead.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 69 insertions(+), 40 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 62ea4adcd0ff1..3f0c3924273d5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned
}
static int grab_objectname(const char *name, const unsigned char *sha1,
- struct atom_value *v, struct used_atom *atom)
+ struct atom_value *v, struct used_atom *atom,
+ struct strbuf *err)
{
if (starts_with(name, "objectname")) {
if (atom->u.objectname.option == O_SHORT) {
v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
- return 1;
} else if (atom->u.objectname.option == O_FULL) {
v->s = xstrdup(sha1_to_hex(sha1));
- return 1;
} else if (atom->u.objectname.option == O_LENGTH) {
v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
- return 1;
- } else
- die("BUG: unknown %%(objectname) option");
+ } else {
+ strbuf_addstr(err, "BUG: unknown %(objectname) option");
+ return -1;
+ }
}
return 0;
}
/* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static int grab_common_values(struct atom_value *val, int deref, struct object *obj,
+ void *buf, unsigned long sz, struct strbuf *err)
{
int i;
@@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
v->s = xstrfmt("%lu", sz);
}
else if (deref)
- grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
+ if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))
+ return -1;
}
+ return 0;
}
/* See grab_values */
@@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)
* pointed at by the ref itself; otherwise it is the object the
* ref (which is a tag) refers to.
*/
-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static int grab_values(struct atom_value *val, int deref, struct object *obj,
+ void *buf, unsigned long sz, struct strbuf *err)
{
- grab_common_values(val, deref, obj, buf, sz);
+ if (grab_common_values(val, deref, obj, buf, sz, err))
+ return -1;
switch (obj->type) {
case OBJ_TAG:
grab_tag_values(val, deref, obj, buf, sz);
@@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
/* grab_blob_values(val, deref, obj, buf, sz); */
break;
default:
- die("Eh? Object of type %d?", obj->type);
+ strbuf_addf(err, "Eh? Object of type %d?", obj->type);
+ return -1;
}
+ return 0;
}
static inline char *copy_advance(char *dst, const char *src)
@@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
return refname;
}
-static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
- struct branch *branch, const char **s)
+static int fill_remote_ref_details(struct used_atom *atom, const char *refname,
+ struct branch *branch, const char **s,
+ struct strbuf *err)
{
int num_ours, num_theirs;
if (atom->u.remote_ref.option == RR_REF)
@@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
if (stat_tracking_info(branch, &num_ours, &num_theirs,
NULL, AHEAD_BEHIND_FULL) < 0)
- return;
+ return 0;
if (!num_ours && !num_theirs)
*s = "=";
@@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
*s = xstrdup(merge);
else
*s = "";
- } else
- die("BUG: unhandled RR_* enum");
+ } else {
+ strbuf_addstr(err, "BUG: unhandled RR_* enum");
+ return -1;
+ }
+ return 0;
}
char *get_head_description(void)
@@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
- if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
- grab_values(ref->value, deref, *obj, buf, size);
+ if (!buf) {
+ strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+ ref->refname);
+ return -1;
+ }
+ if (!*obj) {
+ strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ return -1;
+ }
+ if (grab_values(ref->value, deref, *obj, buf, size, err))
+ return -1;
if (!eaten)
free(buf);
+ return 0;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1513,8 +1529,9 @@ static void populate_value(struct ref_array_item *ref)
branch = branch_get(branch_name);
refname = branch_get_upstream(branch, NULL);
- if (refname)
- fill_remote_ref_details(atom, refname, branch, &v->s);
+ if (refname &&
+ fill_remote_ref_details(atom, refname, branch, &v->s, err))
+ return -1;
continue;
} else if (atom->u.remote_ref.push) {
const char *branch_name;
@@ -1530,7 +1547,8 @@ static void populate_value(struct ref_array_item *ref)
if (!refname)
continue;
}
- fill_remote_ref_details(atom, refname, branch, &v->s);
+ if (fill_remote_ref_details(atom, refname, branch, &v->s, err))
+ return -1;
continue;
} else if (starts_with(name, "color:")) {
v->s = atom->u.color;
@@ -1548,7 +1566,9 @@ static void populate_value(struct ref_array_item *ref)
v->s = xstrdup(buf + 1);
}
continue;
- } else if (!deref && grab_objectname(name, ref->objectname.hash, v, atom)) {
+ } else if (!deref && starts_with(name, "objectname")) {
+ if (grab_objectname(name, ref->objectname.hash, v, atom, err))
+ return -1;
continue;
} else if (!strcmp(name, "HEAD")) {
if (atom->u.head && !strcmp(ref->refname, atom->u.head))
@@ -1590,16 +1610,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1613,20 +1634,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2150,9 +2174,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2231,7 +2259,8 @@ int format_ref_array_item(struct ref_array_item *info,
append_literal(cp, sp, &state);
if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))
return -1;
- get_ref_atom_value(info, pos, &atomv);
+ if (get_ref_atom_value(info, pos, &atomv, error_buf))
+ return -1;
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-14 19:04 ` [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
@ 2018-03-15 20:47 ` Martin Ågren
2018-03-15 21:01 ` Eric Sunshine
` (2 more replies)
0 siblings, 3 replies; 55+ messages in thread
From: Martin Ågren @ 2018-03-15 20:47 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
I skimmed the first four patches of this v2. It seems that patches 1 and
4 are identical to v2. Patches 2 and 3 have very straightforward changes
based on my earlier comments. Let's see what this patch is about. :-)
On 14 March 2018 at 20:04, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:
> Finish removing any printing from ref-filter formatting logic,
> so that it could be more general.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
>
> It's important to mention that grab_objectname() returned 1 if
> it gets objectname atom and 0 otherwise. Now this logic changed:
> we return 0 if we have no error, -1 otherwise. If someone needs to
> know whether it's objectname atom or not, he/she could use
> starts_with() function. It duplicates this checking but it does not
> sound like a really big overhead.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 69 insertions(+), 40 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 62ea4adcd0ff1..3f0c3924273d5 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned
> }
>
> static int grab_objectname(const char *name, const unsigned char *sha1,
> - struct atom_value *v, struct used_atom *atom)
> + struct atom_value *v, struct used_atom *atom,
> + struct strbuf *err)
> {
> if (starts_with(name, "objectname")) {
> if (atom->u.objectname.option == O_SHORT) {
> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> - return 1;
> } else if (atom->u.objectname.option == O_FULL) {
> v->s = xstrdup(sha1_to_hex(sha1));
> - return 1;
> } else if (atom->u.objectname.option == O_LENGTH) {
> v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
> - return 1;
> - } else
> - die("BUG: unknown %%(objectname) option");
> + } else {
> + strbuf_addstr(err, "BUG: unknown %(objectname) option");
> + return -1;
> + }
> }
> return 0;
> }
This is interesting. This die() is never ever supposed to actually
trigger, except to allow a developer adding some new O_xxx-value to
quickly notice that they have forgotten to add code here.
> /* See grab_values */
> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj,
> + void *buf, unsigned long sz, struct strbuf *err)
> {
> int i;
>
> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
> v->s = xstrfmt("%lu", sz);
> }
> else if (deref)
> - grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
> + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))
> + return -1;
> }
> + return 0;
> }
So if that conversion I commented on above had not happened, this would
not have been necessary. Let's read on...
> /* See grab_values */
> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)
> * pointed at by the ref itself; otherwise it is the object the
> * ref (which is a tag) refers to.
> */
> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
> +static int grab_values(struct atom_value *val, int deref, struct object *obj,
> + void *buf, unsigned long sz, struct strbuf *err)
> {
> - grab_common_values(val, deref, obj, buf, sz);
> + if (grab_common_values(val, deref, obj, buf, sz, err))
> + return -1;
> switch (obj->type) {
> case OBJ_TAG:
> grab_tag_values(val, deref, obj, buf, sz);
> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
> /* grab_blob_values(val, deref, obj, buf, sz); */
> break;
> default:
> - die("Eh? Object of type %d?", obj->type);
> + strbuf_addf(err, "Eh? Object of type %d?", obj->type);
> + return -1;
> }
> + return 0;
> }
This seems similar. The string here is quite sloppy, and I do not
believe that the author intended this to be user-visible. I believe this
is more like a very short way of saying "how could we possibly get
here??". It could also be written as die("BUG: unknown object type %d",
obj->type), or even better: BUG(...).
> static inline char *copy_advance(char *dst, const char *src)
> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
> return refname;
> }
>
> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> - struct branch *branch, const char **s)
> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname,
> + struct branch *branch, const char **s,
> + struct strbuf *err)
> {
> int num_ours, num_theirs;
> if (atom->u.remote_ref.option == RR_REF)
> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
> if (stat_tracking_info(branch, &num_ours, &num_theirs,
> NULL, AHEAD_BEHIND_FULL) < 0)
> - return;
> + return 0;
>
> if (!num_ours && !num_theirs)
> *s = "=";
> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
> *s = xstrdup(merge);
> else
> *s = "";
> - } else
> - die("BUG: unhandled RR_* enum");
> + } else {
> + strbuf_addstr(err, "BUG: unhandled RR_* enum");
> + return -1;
> + }
> + return 0;
> }
This one too.. I start to think it is overkill to wire through these
strbufs just to collect messages that should never ever be produced. It
almost seems to me like if 1) we want to collect errors using strbufs,
and 2) we want to use BUG() for these sorts of assertions, then 3) we
will be wiring error-strbufs through more or less all of our code. I am
exaggerating, but there is something to it: A small change deep down a
callstack where you want to add a BUG() "to be safe", and you might need
to wire a strbuf all the way through.
According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a
good thing about BUG() is that we can get a core dump, a filename and a
line number. That opportunity gets lost if we do these sort of
transformations. Of course, these were only die("BUG: "), not BUG(), but
my point is that they should perhaps have been BUG().
> char *get_head_description(void)
> @@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
> return show_ref(&atom->u.refname, ref->refname);
> }
>
> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,
> - int deref, struct object **obj)
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> + int deref, struct object **obj, struct strbuf *err)
> {
> int eaten;
> unsigned long size;
> void *buf = get_obj(oid, obj, &size, &eaten);
> - if (!buf)
> - die(_("missing object %s for %s"),
> - oid_to_hex(oid), ref->refname);
> - if (!*obj)
> - die(_("parse_object_buffer failed on %s for %s"),
> - oid_to_hex(oid), ref->refname);
> -
> - grab_values(ref->value, deref, *obj, buf, size);
> + if (!buf) {
> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
> + ref->refname);
> + return -1;
> + }
> + if (!*obj) {
> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> + oid_to_hex(oid), ref->refname);
> + return -1;
> + }
> + if (grab_values(ref->value, deref, *obj, buf, size, err))
> + return -1;
> if (!eaten)
> free(buf);
> + return 0;
> }
These are "real" errors and yield several more changes in the remainder.
Ignoring those BUG-type messages at the beginning of this patch would
give a patch like the one below. Maybe that would be a bit less
intrusive for the same gain.
Thanks for working on this. I feel that your patches are really
interesting. They open up many possibilities for philosophical exercises
about how errors should be collected and reported. ;-) I would be
interested in knowing your thoughts, and others'.
Martin
---
ref-filter.c | 51 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 62ea4adcd0..e41505b3c0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1447,28 +1447,32 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
- if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
+ if (!buf) {
+ strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+ ref->refname);
+ return -1;
+ }
+ if (!*obj) {
+ strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ return -1;
+ }
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+ return 0;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1590,16 +1594,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1613,20 +1618,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2150,9 +2158,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info,
append_literal(cp, sp, &state);
if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))
return -1;
- get_ref_atom_value(info, pos, &atomv);
+ if (get_ref_atom_value(info, pos, &atomv, error_buf))
+ return -1;
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
--
2.16.2.246.ga4ee44448f
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-15 20:47 ` Martin Ågren
@ 2018-03-15 21:01 ` Eric Sunshine
2018-03-16 7:20 ` Оля Тележная
2018-03-15 21:05 ` Junio C Hamano
2018-03-16 7:17 ` Оля Тележная
2 siblings, 1 reply; 55+ messages in thread
From: Eric Sunshine @ 2018-03-15 21:01 UTC (permalink / raw)
To: Martin Ågren; +Cc: Olga Telezhnaya, Git List
On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> These are "real" errors and yield several more changes in the remainder.
> Ignoring those BUG-type messages at the beginning of this patch would
> give a patch like the one below.
>
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> + int deref, struct object **obj, struct strbuf *err)
> {
> void *buf = get_obj(oid, obj, &size, &eaten);
> - if (!buf)
> - die(_("missing object %s for %s"),
> - oid_to_hex(oid), ref->refname);
> - if (!*obj)
> - die(_("parse_object_buffer failed on %s for %s"),
> - oid_to_hex(oid), ref->refname);
> -
> + if (!buf) {
> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
> + ref->refname);
> + return -1;
> + }
> + if (!*obj) {
> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> + oid_to_hex(oid), ref->refname);
> + return -1;
Doesn't this leak 'buf'?
> + }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> + return 0;
> }
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-15 21:01 ` Eric Sunshine
@ 2018-03-16 7:20 ` Оля Тележная
0 siblings, 0 replies; 55+ messages in thread
From: Оля Тележная @ 2018-03-16 7:20 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Martin Ågren, Git List
2018-03-16 0:01 GMT+03:00 Eric Sunshine <sunshine@sunshineco.com>:
> On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> These are "real" errors and yield several more changes in the remainder.
>> Ignoring those BUG-type messages at the beginning of this patch would
>> give a patch like the one below.
>>
>> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
>> + int deref, struct object **obj, struct strbuf *err)
>> {
>> void *buf = get_obj(oid, obj, &size, &eaten);
>> - if (!buf)
>> - die(_("missing object %s for %s"),
>> - oid_to_hex(oid), ref->refname);
>> - if (!*obj)
>> - die(_("parse_object_buffer failed on %s for %s"),
>> - oid_to_hex(oid), ref->refname);
>> -
>> + if (!buf) {
>> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
>> + ref->refname);
>> + return -1;
>> + }
>> + if (!*obj) {
>> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
>> + oid_to_hex(oid), ref->refname);
>> + return -1;
>
> Doesn't this leak 'buf'?
Yes. Thanks a lot.
>
>> + }
>> grab_values(ref->value, deref, *obj, buf, size);
>> if (!eaten)
>> free(buf);
>> + return 0;
>> }
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-15 20:47 ` Martin Ågren
2018-03-15 21:01 ` Eric Sunshine
@ 2018-03-15 21:05 ` Junio C Hamano
2018-03-16 7:17 ` Оля Тележная
2 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2018-03-15 21:05 UTC (permalink / raw)
To: Martin Ågren; +Cc: Olga Telezhnaya, git
Martin Ågren <martin.agren@gmail.com> writes:
>> static int grab_objectname(const char *name, const unsigned char *sha1,
>> - struct atom_value *v, struct used_atom *atom)
>> + struct atom_value *v, struct used_atom *atom,
>> + struct strbuf *err)
>> {
>> ...
>> + } else {
>> + strbuf_addstr(err, "BUG: unknown %(objectname) option");
>> + return -1;
>> + }
>> }
>> return 0;
>> }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.
Yup, BUG() is meant for a case like this; the original code predates
the BUG("message") which in turn predates the die("BUG: message")
convention, I think.
>> default:
>> - die("Eh? Object of type %d?", obj->type);
>> + strbuf_addf(err, "Eh? Object of type %d?", obj->type);
>> + return -1;
>> }
>> + return 0;
>> }
>
> This seems similar. The string here is quite sloppy, and I do not
> believe that the author intended this to be user-visible. I believe this
> is more like a very short way of saying "how could we possibly get
> here??". It could also be written as die("BUG: unknown object type %d",
> obj->type), or even better: BUG(...).
Likewise.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-15 20:47 ` Martin Ågren
2018-03-15 21:01 ` Eric Sunshine
2018-03-15 21:05 ` Junio C Hamano
@ 2018-03-16 7:17 ` Оля Тележная
2 siblings, 0 replies; 55+ messages in thread
From: Оля Тележная @ 2018-03-16 7:17 UTC (permalink / raw)
To: Martin Ågren; +Cc: git
2018-03-15 23:47 GMT+03:00 Martin Ågren <martin.agren@gmail.com>:
> I skimmed the first four patches of this v2. It seems that patches 1 and
> 4 are identical to v2. Patches 2 and 3 have very straightforward changes
> based on my earlier comments. Let's see what this patch is about. :-)
Yes, you are right.
>
> On 14 March 2018 at 20:04, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:
>> Finish removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>>
>> Change the signature of get_ref_atom_value() and underlying functions
>> by adding return value and strbuf parameter for error message.
>>
>> It's important to mention that grab_objectname() returned 1 if
>> it gets objectname atom and 0 otherwise. Now this logic changed:
>> we return 0 if we have no error, -1 otherwise. If someone needs to
>> know whether it's objectname atom or not, he/she could use
>> starts_with() function. It duplicates this checking but it does not
>> sound like a really big overhead.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> ---
>> ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 69 insertions(+), 40 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 62ea4adcd0ff1..3f0c3924273d5 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned
>> }
>>
>> static int grab_objectname(const char *name, const unsigned char *sha1,
>> - struct atom_value *v, struct used_atom *atom)
>> + struct atom_value *v, struct used_atom *atom,
>> + struct strbuf *err)
>> {
>> if (starts_with(name, "objectname")) {
>> if (atom->u.objectname.option == O_SHORT) {
>> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> - return 1;
>> } else if (atom->u.objectname.option == O_FULL) {
>> v->s = xstrdup(sha1_to_hex(sha1));
>> - return 1;
>> } else if (atom->u.objectname.option == O_LENGTH) {
>> v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
>> - return 1;
>> - } else
>> - die("BUG: unknown %%(objectname) option");
>> + } else {
>> + strbuf_addstr(err, "BUG: unknown %(objectname) option");
>> + return -1;
>> + }
>> }
>> return 0;
>> }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.
Oh, cool, so I can revert this change, OK.
>
>> /* See grab_values */
>> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
>> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj,
>> + void *buf, unsigned long sz, struct strbuf *err)
>> {
>> int i;
>>
>> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
>> v->s = xstrfmt("%lu", sz);
>> }
>> else if (deref)
>> - grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
>> + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))
>> + return -1;
>> }
>> + return 0;
>> }
>
> So if that conversion I commented on above had not happened, this would
> not have been necessary. Let's read on...
Of course, I will check and also revert functions that were touched
only because of other functions.
>
>> /* See grab_values */
>> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)
>> * pointed at by the ref itself; otherwise it is the object the
>> * ref (which is a tag) refers to.
>> */
>> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
>> +static int grab_values(struct atom_value *val, int deref, struct object *obj,
>> + void *buf, unsigned long sz, struct strbuf *err)
>> {
>> - grab_common_values(val, deref, obj, buf, sz);
>> + if (grab_common_values(val, deref, obj, buf, sz, err))
>> + return -1;
>> switch (obj->type) {
>> case OBJ_TAG:
>> grab_tag_values(val, deref, obj, buf, sz);
>> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
>> /* grab_blob_values(val, deref, obj, buf, sz); */
>> break;
>> default:
>> - die("Eh? Object of type %d?", obj->type);
>> + strbuf_addf(err, "Eh? Object of type %d?", obj->type);
>> + return -1;
>> }
>> + return 0;
>> }
>
> This seems similar. The string here is quite sloppy, and I do not
> believe that the author intended this to be user-visible. I believe this
> is more like a very short way of saying "how could we possibly get
> here??". It could also be written as die("BUG: unknown object type %d",
> obj->type), or even better: BUG(...).
OK, thanks!
>
>> static inline char *copy_advance(char *dst, const char *src)
>> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
>> return refname;
>> }
>>
>> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> - struct branch *branch, const char **s)
>> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> + struct branch *branch, const char **s,
>> + struct strbuf *err)
>> {
>> int num_ours, num_theirs;
>> if (atom->u.remote_ref.option == RR_REF)
>> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
>> if (stat_tracking_info(branch, &num_ours, &num_theirs,
>> NULL, AHEAD_BEHIND_FULL) < 0)
>> - return;
>> + return 0;
>>
>> if (!num_ours && !num_theirs)
>> *s = "=";
>> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> *s = xstrdup(merge);
>> else
>> *s = "";
>> - } else
>> - die("BUG: unhandled RR_* enum");
>> + } else {
>> + strbuf_addstr(err, "BUG: unhandled RR_* enum");
>> + return -1;
>> + }
>> + return 0;
>> }
>
> This one too.. I start to think it is overkill to wire through these
> strbufs just to collect messages that should never ever be produced. It
> almost seems to me like if 1) we want to collect errors using strbufs,
> and 2) we want to use BUG() for these sorts of assertions, then 3) we
> will be wiring error-strbufs through more or less all of our code. I am
> exaggerating, but there is something to it: A small change deep down a
> callstack where you want to add a BUG() "to be safe", and you might need
> to wire a strbuf all the way through.
I absolutely agree that we should not touch functions that produce
errors for Git developers. It still means that sometimes we need to
wire strbufs, but I will revert some of them. It's enough to handle
only errors for users.
>
> According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a
> good thing about BUG() is that we can get a core dump, a filename and a
> line number. That opportunity gets lost if we do these sort of
> transformations. Of course, these were only die("BUG: "), not BUG(), but
> my point is that they should perhaps have been BUG().
>
>> char *get_head_description(void)
>> @@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>> return show_ref(&atom->u.refname, ref->refname);
>> }
>>
>> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,
>> - int deref, struct object **obj)
>> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
>> + int deref, struct object **obj, struct strbuf *err)
>> {
>> int eaten;
>> unsigned long size;
>> void *buf = get_obj(oid, obj, &size, &eaten);
>> - if (!buf)
>> - die(_("missing object %s for %s"),
>> - oid_to_hex(oid), ref->refname);
>> - if (!*obj)
>> - die(_("parse_object_buffer failed on %s for %s"),
>> - oid_to_hex(oid), ref->refname);
>> -
>> - grab_values(ref->value, deref, *obj, buf, size);
>> + if (!buf) {
>> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
>> + ref->refname);
>> + return -1;
>> + }
>> + if (!*obj) {
>> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
>> + oid_to_hex(oid), ref->refname);
>> + return -1;
>> + }
>> + if (grab_values(ref->value, deref, *obj, buf, size, err))
>> + return -1;
>> if (!eaten)
>> free(buf);
>> + return 0;
>> }
>
> These are "real" errors and yield several more changes in the remainder.
> Ignoring those BUG-type messages at the beginning of this patch would
> give a patch like the one below. Maybe that would be a bit less
> intrusive for the same gain.
>
> Thanks for working on this. I feel that your patches are really
> interesting. They open up many possibilities for philosophical exercises
> about how errors should be collected and reported. ;-) I would be
> interested in knowing your thoughts, and others'.
Thank you! I was also thinking about other ways how to handle errors.
I haven't invented anything new, so at least I decided to make these
changes (for now).
>
> Martin
>
> ---
> ref-filter.c | 51 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 62ea4adcd0..e41505b3c0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1447,28 +1447,32 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
> return show_ref(&atom->u.refname, ref->refname);
> }
>
> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,
> - int deref, struct object **obj)
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> + int deref, struct object **obj, struct strbuf *err)
> {
> int eaten;
> unsigned long size;
> void *buf = get_obj(oid, obj, &size, &eaten);
> - if (!buf)
> - die(_("missing object %s for %s"),
> - oid_to_hex(oid), ref->refname);
> - if (!*obj)
> - die(_("parse_object_buffer failed on %s for %s"),
> - oid_to_hex(oid), ref->refname);
> -
> + if (!buf) {
> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
> + ref->refname);
> + return -1;
> + }
> + if (!*obj) {
> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> + oid_to_hex(oid), ref->refname);
> + return -1;
> + }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> + return 0;
> }
>
> /*
> * Parse the object referred by ref, and grab needed value.
> */
> -static void populate_value(struct ref_array_item *ref)
> +static int populate_value(struct ref_array_item *ref, struct strbuf *err)
> {
> struct object *obj;
> int i;
> @@ -1590,16 +1594,17 @@ static void populate_value(struct ref_array_item *ref)
> break;
> }
> if (used_atom_cnt <= i)
> - return;
> + return 0;
>
> - get_object(ref, &ref->objectname, 0, &obj);
> + if (get_object(ref, &ref->objectname, 0, &obj, err))
> + return -1;
>
> /*
> * If there is no atom that wants to know about tagged
> * object, we are done.
> */
> if (!need_tagged || (obj->type != OBJ_TAG))
> - return;
> + return 0;
>
> /*
> * If it is a tag object, see if we use a value that derefs
> @@ -1613,20 +1618,23 @@ static void populate_value(struct ref_array_item *ref)
> * is not consistent with what deref_tag() does
> * which peels the onion to the core.
> */
> - get_object(ref, tagged, 1, &obj);
> + return get_object(ref, tagged, 1, &obj, err);
> }
>
> /*
> * Given a ref, return the value for the atom. This lazily gets value
> * out of the object by calling populate value.
> */
> -static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
> +static int get_ref_atom_value(struct ref_array_item *ref, int atom,
> + struct atom_value **v, struct strbuf *err)
> {
> if (!ref->value) {
> - populate_value(ref);
> + if (populate_value(ref, err))
> + return -1;
> fill_missing_values(ref->value);
> }
> *v = &ref->value[atom];
> + return 0;
> }
>
> /*
> @@ -2150,9 +2158,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
> int cmp;
> cmp_type cmp_type = used_atom[s->atom].type;
> int (*cmp_fn)(const char *, const char *);
> + struct strbuf err = STRBUF_INIT;
>
> - get_ref_atom_value(a, s->atom, &va);
> - get_ref_atom_value(b, s->atom, &vb);
> + if (get_ref_atom_value(a, s->atom, &va, &err))
> + die("%s", err.buf);
> + if (get_ref_atom_value(b, s->atom, &vb, &err))
> + die("%s", err.buf);
> + strbuf_release(&err);
> cmp_fn = s->ignore_case ? strcasecmp : strcmp;
> if (s->version)
> cmp = versioncmp(va->s, vb->s);
> @@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info,
> append_literal(cp, sp, &state);
> if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))
> return -1;
> - get_ref_atom_value(info, pos, &atomv);
> + if (get_ref_atom_value(info, pos, &atomv, error_buf))
> + return -1;
> if (atomv->handler(atomv, &state, error_buf))
> return -1;
> }
> --
> 2.16.2.246.ga4ee44448f
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 3/5] ref-filter: change parsing function error handling
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
` (2 preceding siblings ...)
2018-03-14 19:04 ` [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
@ 2018-03-14 19:04 ` Olga Telezhnaya
2018-03-15 22:48 ` Junio C Hamano
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-14 19:04 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of parse_ref_filter_atom() by changing return value,
adding previous return value to function parameter and also adding
strbuf parameter for error message.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 43 ++++++++++++++++++++++++++++++-------------
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index d120360104806..dd83ef326511d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
- const char *atom, const char *ep)
+ const char *atom, const char *ep, int *res,
+ struct strbuf *err)
{
const char *sp;
const char *arg;
@@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
- if (ep <= sp)
- die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+ if (ep <= sp) {
+ strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
int len = strlen(used_atom[i].name);
- if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
- return i;
+ if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) {
+ *res = i;
+ return 0;
+ }
}
/*
@@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
break;
}
- if (ARRAY_SIZE(valid_atom) <= i)
- die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+ if (ARRAY_SIZE(valid_atom) <= i) {
+ strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
- return at;
+ *res = at;
+ return 0;
}
static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
@@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err))
+ die("%s", err.buf);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
format->need_color_reset_at_eol = !!strcmp(color, "reset");
+ strbuf_release(&err);
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,13 +2165,14 @@ int format_ref_array_item(struct ref_array_item *info,
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+ int pos;
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))
+ return -1;
+ get_ref_atom_value(info, pos, &atomv);
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
@@ -2215,7 +2227,12 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ struct strbuf err = STRBUF_INIT;
+ int res;
+ if (parse_ref_filter_atom(&dummy, atom, end, &res, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+ return res;
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v2 3/5] ref-filter: change parsing function error handling
2018-03-14 19:04 ` [PATCH v2 3/5] ref-filter: change parsing function " Olga Telezhnaya
@ 2018-03-15 22:48 ` Junio C Hamano
2018-03-16 7:22 ` Оля Тележная
0 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2018-03-15 22:48 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.
Hmm.
> Change the signature of parse_ref_filter_atom() by changing return value,
> adding previous return value to function parameter and also adding
> strbuf parameter for error message.
This says what the patch changes, but it does not explain why it is
a good idea to return something with different meaning to the
callers (which of course forces us to update all callers so that
they pass &result pointer and check the value returned in the
variable), or more importantly what meaning the return value has and
how the callers are expected to use it. While at it, it probably is
a good idea to explain what the original return value means.
The return value from parse_ref_filter_atom() used to be the
position the atom is found in the used_atom[] array; that
information is now returned in an integer pointed at by the
*res parameter. The function now returns 0 for success and
-1 for failure.
or something like that.
Having said that, I wonder if a calling convention that does not
force callers to pass in a pointer-to-int may make more sense.
Because the original return value is an index into an array, we know
the normal return values are not negative. An updated caller could
become like this instead:
pos = parse_ref_filter_atom(format, atom, ep, &err);
if (pos < 0)
die("%s", err.buf);
... original code that used 'pos' can stay as before ...
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v2 3/5] ref-filter: change parsing function error handling
2018-03-15 22:48 ` Junio C Hamano
@ 2018-03-16 7:22 ` Оля Тележная
0 siblings, 0 replies; 55+ messages in thread
From: Оля Тележная @ 2018-03-16 7:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
2018-03-16 1:48 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Continue removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>
> Hmm.
>
>> Change the signature of parse_ref_filter_atom() by changing return value,
>> adding previous return value to function parameter and also adding
>> strbuf parameter for error message.
>
> This says what the patch changes, but it does not explain why it is
> a good idea to return something with different meaning to the
> callers (which of course forces us to update all callers so that
> they pass &result pointer and check the value returned in the
> variable), or more importantly what meaning the return value has and
> how the callers are expected to use it. While at it, it probably is
> a good idea to explain what the original return value means.
>
> The return value from parse_ref_filter_atom() used to be the
> position the atom is found in the used_atom[] array; that
> information is now returned in an integer pointed at by the
> *res parameter. The function now returns 0 for success and
> -1 for failure.
>
> or something like that.
>
> Having said that, I wonder if a calling convention that does not
> force callers to pass in a pointer-to-int may make more sense.
> Because the original return value is an index into an array, we know
> the normal return values are not negative. An updated caller could
> become like this instead:
>
> pos = parse_ref_filter_atom(format, atom, ep, &err);
> if (pos < 0)
> die("%s", err.buf);
> ... original code that used 'pos' can stay as before ...
>
Martin also mentioned that, but I was not sure which solution is
better. Great, thanks, I will fix that.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 1/5] ref-filter: start adding strbufs with errors
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
` (3 preceding siblings ...)
2018-03-14 19:04 ` [PATCH v2 3/5] ref-filter: change parsing function " Olga Telezhnaya
@ 2018-03-19 13:01 ` Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 4/5] ref-filter: add return value to parsers Olga Telezhnaya
` (4 more replies)
4 siblings, 5 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-19 13:01 UTC (permalink / raw)
To: git
This is a first step in removing any printing from
ref-filter formatting logic, so that it could be more general.
Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.
This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
builtin/branch.c | 7 +++++--
ref-filter.c | 17 ++++++++++++-----
ref-filter.h | 7 ++++---
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
- struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
ref_array_sort(sorting, &array);
for (i = 0; i < array.nr; i++) {
- format_ref_array_item(array.items[i], format, &out);
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ if (format_ref_array_item(array.items[i], format, &out, &err))
+ die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are incompatible");
/* format to a string_list to let print_columns() do its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+ strbuf_release(&err);
strbuf_release(&out);
}
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..54fae00bdd410 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
}
}
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
const struct ref_format *format,
- struct strbuf *final_buf)
+ struct strbuf *final_buf,
+ struct strbuf *error_buf)
{
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(&resetv, &state);
}
- if (state.stack->prev)
- die(_("format: %%(end) atom missing"));
+ if (state.stack->prev) {
+ strbuf_addstr(error_buf, _("format: %(end) atom missing"));
+ return -1;
+ }
strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+ return 0;
}
void show_ref_array_item(struct ref_array_item *info,
const struct ref_format *format)
{
struct strbuf final_buf = STRBUF_INIT;
+ struct strbuf error_buf = STRBUF_INIT;
- format_ref_array_item(info, format, &final_buf);
+ if (format_ref_array_item(info, format, &final_buf, &error_buf))
+ die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ strbuf_release(&error_buf);
strbuf_release(&final_buf);
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *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 struct ref_format *format,
- struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
/* Print the ref using the given format and quote_style */
void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
/* Parse a single sort specifier and add it to the list */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 4/5] ref-filter: add return value to parsers
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
@ 2018-03-19 13:01 ` Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
` (3 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-19 13:01 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of parsers by adding return value and
strbuf parameter for error message.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 118 insertions(+), 59 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 2313a33f0baa4..b90cec1056954 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,28 @@ static struct used_atom {
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *color_value, struct strbuf *err)
{
- if (!color_value)
- die(_("expected format: %%(color:<color>)"));
- if (color_parse(color_value, atom->u.color) < 0)
- die(_("unrecognized color: %%(color:%s)"), color_value);
+ if (!color_value) {
+ strbuf_addstr(err, _("expected format: %(color:<color>)"));
+ return -1;
+ }
+ if (color_parse(color_value, atom->u.color) < 0) {
+ strbuf_addf(err, _("unrecognized color: %%(color:%s)"), color_value);
+ return -1;
+ }
/*
* We check this after we've parsed the color, which lets us complain
* about syntactically bogus color names even if they won't be used.
*/
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+ return 0;
}
-static void refname_atom_parser_internal(struct refname_atom *atom,
- const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,
+ const char *name, struct strbuf *err)
{
if (!arg)
atom->option = R_NORMAL;
@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
else if (skip_prefix(arg, "lstrip=", &arg) ||
skip_prefix(arg, "strip=", &arg)) {
atom->option = R_LSTRIP;
- if (strtol_i(arg, 10, &atom->lstrip))
- die(_("Integer value expected refname:lstrip=%s"), arg);
+ if (strtol_i(arg, 10, &atom->lstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:lstrip=%s"), arg);
+ return -1;
+ }
} else if (skip_prefix(arg, "rstrip=", &arg)) {
atom->option = R_RSTRIP;
- if (strtol_i(arg, 10, &atom->rstrip))
- die(_("Integer value expected refname:rstrip=%s"), arg);
- } else
- die(_("unrecognized %%(%s) argument: %s"), name, arg);
+ if (strtol_i(arg, 10, &atom->rstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:rstrip=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, arg);
+ return -1;
+ }
+ return 0;
}
-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
if (!arg) {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
- return;
+ return refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err);
}
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
+ if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err))
+ return -1;
}
}
string_list_clear(¶ms, 0);
+ return 0;
}
-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(body) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(body) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_BODY_DEP;
+ return 0;
}
-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(subject) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(subject) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_SUB;
+ return 0;
}
-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.trailer_opts.unfold = 1;
else if (!strcmp(s, "only"))
atom->u.contents.trailer_opts.only_trailers = 1;
- else
- die(_("unknown %%(trailers) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
+ return -1;
+ }
}
}
atom->u.contents.option = C_TRAILERS;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.contents.option = C_BARE;
@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.option = C_SUB;
else if (skip_prefix(arg, "trailers", &arg)) {
skip_prefix(arg, ":", &arg);
- trailers_atom_parser(format, atom, *arg ? arg : NULL);
+ if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+ return -1;
} else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
- if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
- die(_("positive value expected contents:lines=%s"), arg);
- } else
- die(_("unrecognized %%(contents) argument: %s"), arg);
+ if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {
+ strbuf_addf(err, _("positive value expected contents:lines=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(contents) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.objectname.option = O_FULL;
@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
else if (skip_prefix(arg, "short=", &arg)) {
atom->u.objectname.option = O_LENGTH;
if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
- atom->u.objectname.length == 0)
- die(_("positive value expected objectname:short=%s"), arg);
+ atom->u.objectname.length == 0) {
+ strbuf_addf(err, _("positive value expected objectname:short=%s"), arg);
+ return -1;
+ }
if (atom->u.objectname.length < MINIMUM_ABBREV)
atom->u.objectname.length = MINIMUM_ABBREV;
- } else
- die(_("unrecognized %%(objectname) argument: %s"), arg);
+ } else {
+ strbuf_addf(err, _("unrecognized %%(objectname) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
+ return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
}
static align_type parse_align_position(const char *s)
@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)
return -1;
}
-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct align *align = &atom->u.align;
struct string_list params = STRING_LIST_INIT_DUP;
int i;
unsigned int width = ~0U;
- if (!arg)
- die(_("expected format: %%(align:<width>,<position>)"));
+ if (!arg) {
+ strbuf_addstr(err, _("expected format: %(align:<width>,<position>)"));
+ return -1;
+ }
align->position = ALIGN_LEFT;
@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
if (skip_prefix(s, "position=", &s)) {
position = parse_align_position(s);
- if (position < 0)
- die(_("unrecognized position:%s"), s);
+ if (position < 0) {
+ strbuf_addf(err, _("unrecognized position:%s"), s);
+ return -1;
+ }
align->position = position;
} else if (skip_prefix(s, "width=", &s)) {
- if (strtoul_ui(s, 10, &width))
- die(_("unrecognized width:%s"), s);
+ if (strtoul_ui(s, 10, &width)) {
+ strbuf_addf(err, _("unrecognized width:%s"), s);
+ return -1;
+ }
} else if (!strtoul_ui(s, 10, &width))
;
else if ((position = parse_align_position(s)) >= 0)
align->position = position;
- else
- die(_("unrecognized %%(align) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unrecognized %%(align) argument: %s"), s);
+ return -1;
+ }
}
- if (width == ~0U)
- die(_("positive width expected with the %%(align) atom"));
+ if (width == ~0U) {
+ strbuf_addstr(err, _("positive width expected with the %(align) atom"));
+ return -1;
+ }
align->width = width;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg) {
atom->u.if_then_else.cmp_status = COMPARE_NONE;
- return;
+ return 0;
} else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
} else {
- die(_("unrecognized %%(if) argument: %s"), arg);
+ strbuf_addf(err, _("unrecognized %%(if) argument: %s"), arg);
+ return -1;
}
+ return 0;
}
-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
+ return 0;
}
static struct {
const char *name;
cmp_type cmp_type;
- void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
+ int (*parser)(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
@@ -457,8 +516,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
}
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
- if (valid_atom[i].parser)
- valid_atom[i].parser(format, &used_atom[at], arg);
+ if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
+ return -1;
if (*atom == '*')
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 4/5] ref-filter: add return value to parsers Olga Telezhnaya
@ 2018-03-19 13:01 ` Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
` (2 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-19 13:01 UTC (permalink / raw)
To: git
Finish removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Some die() calls are left; all of them are not for users, but for
Git developers.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index b90cec1056954..85f971663a4aa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
- if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
+ if (!buf) {
+ strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+ ref->refname);
+ if (!eaten)
+ free(buf);
+ return -1;
+ }
+ if (!*obj) {
+ strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ if (!eaten)
+ free(buf);
+ return -1;
+ }
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+ return 0;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
if (pos < 0)
return -1;
- get_ref_atom_value(info, pos, &atomv);
+ if (get_ref_atom_value(info, pos, &atomv, error_buf))
+ return -1;
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 4/5] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-19 13:01 ` [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
@ 2018-03-19 13:01 ` Olga Telezhnaya
2018-03-19 19:24 ` Junio C Hamano
2018-03-19 13:01 ` [PATCH v3 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-19 13:01 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of handlers by adding return value
and strbuf parameter for errors.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 48 insertions(+), 23 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 54fae00bdd410..d120360104806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
+ int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
};
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
}
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
/*
* Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
quote_formatting(&state->stack->output, v->s, state->quote_style);
else
strbuf_addstr(&state->stack->output, v->s);
+ return 0;
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)
strbuf_release(&s);
}
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = &atomv->atom->u.align;
+ return 0;
}
static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
free(if_then_else);
}
-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+ return 0;
}
static int is_empty(const char *s)
@@ -596,19 +603,24 @@ static int is_empty(const char *s)
return 1;
}
-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(then) atom used more than once"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(then) atom used after %%(else)"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(then) atom used without an %(if) atom"));
+ return -1;
+ } else if (if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used more than once"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used after %(else)"));
+ return -1;
+ }
if_then_else->then_atom_seen = 1;
/*
* If the 'equals' or 'notequals' attribute is used then
@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
+ return 0;
}
-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(else) atom used without a %%(then) atom"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(else) atom used more than once"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(else) atom used without an %(if) atom"));
+ return -1;
+ } else if (!if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used without a %(then) atom"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used more than once"));
+ return -1;
+ }
if_then_else->else_atom_seen = 1;
push_stack_element(&state->stack);
state->stack->at_end_data = prev->at_end_data;
state->stack->at_end = prev->at_end;
+ return 0;
}
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *current = state->stack;
struct strbuf s = STRBUF_INIT;
- if (!current->at_end)
- die(_("format: %%(end) atom used without corresponding atom"));
+ if (!current->at_end) {
+ strbuf_addstr(err, _("format: %(end) atom used without corresponding atom"));
+ return -1;
+ }
current->at_end(&state->stack);
/* Stack may have been popped within at_end(), hence reset the current pointer */
@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
}
strbuf_release(&s);
pop_stack_element(&state->stack);
+ return 0;
}
/*
@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,
get_ref_atom_value(info,
parse_ref_filter_atom(format, sp + 2, ep),
&atomv);
- atomv->handler(atomv, &state);
+ if (atomv->handler(atomv, &state, error_buf))
+ return -1;
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (format->need_color_reset_at_eol) {
struct atom_value resetv;
resetv.s = GIT_COLOR_RESET;
- append_atom(&resetv, &state);
+ if (append_atom(&resetv, &state, error_buf))
+ return -1;
}
if (state.stack->prev) {
strbuf_addstr(error_buf, _("format: %(end) atom missing"));
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers
2018-03-19 13:01 ` [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-19 19:24 ` Junio C Hamano
0 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2018-03-19 19:24 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.
"more general" sounds overly broad. Is this to avoid calling die()
so that the caller can decide what error messages it wants to give,
abort operation or not, etc.? From a quick scan of the patch, I
have a feeling that "any printing" is not the problem you are
solving (calling die() is).
> Change the signature of handlers by adding return value
> and strbuf parameter for errors.
Could you explain what the "return value" means? We can see from
the patch that the function signature is being changed by the patch.
What needs human explanation is why and what the values mean,
e.g. "to allow the caller to notice an error, the function returns 0
upon success and non-0 upon failure, and an error message is
appended to the strbuf *err" (don't pay too much attention to "0" or
"non-0" in this example, as I do not know what you chose to assign
the return values to signal what to the callers; this is merely to
illustrate the degree of details I would expect).
Thanks.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 3/5] ref-filter: change parsing function error handling
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
` (2 preceding siblings ...)
2018-03-19 13:01 ` [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-19 13:01 ` Olga Telezhnaya
2018-03-19 19:41 ` Junio C Hamano
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-19 13:01 UTC (permalink / raw)
To: git
Continue removing any printing from ref-filter formatting logic,
so that it could be more general.
Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
Return value means the same except negative values: they indicate
errors (previous version could return only non-negative value).
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index d120360104806..2313a33f0baa4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
- const char *atom, const char *ep)
+ const char *atom, const char *ep,
+ struct strbuf *err)
{
const char *sp;
const char *arg;
@@ -406,8 +407,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
- if (ep <= sp)
- die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+ if (ep <= sp) {
+ strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -432,8 +435,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
break;
}
- if (ARRAY_SIZE(valid_atom) <= i)
- die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+ if (ARRAY_SIZE(valid_atom) <= i) {
+ strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -725,17 +730,21 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ at = parse_ref_filter_atom(format, sp + 2, ep, &err);
+ if (at < 0)
+ die("%s", err.buf);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
format->need_color_reset_at_eol = !!strcmp(color, "reset");
+ strbuf_release(&err);
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,13 +2163,15 @@ int format_ref_array_item(struct ref_array_item *info,
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+ int pos;
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+ if (pos < 0)
+ return -1;
+ get_ref_atom_value(info, pos, &atomv);
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
@@ -2215,7 +2226,12 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ struct strbuf err = STRBUF_INIT;
+ int res = parse_ref_filter_atom(&dummy, atom, end, &err);
+ if (res < 0)
+ die("%s", err.buf);
+ strbuf_release(&err);
+ return res;
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v3 3/5] ref-filter: change parsing function error handling
2018-03-19 13:01 ` [PATCH v3 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-19 19:41 ` Junio C Hamano
0 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2018-03-19 19:41 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.
>
> Change the signature of parse_ref_filter_atom() by adding
> strbuf parameter for error message.
> Return value means the same except negative values: they indicate
> errors (previous version could return only non-negative value).
"The same" meaning? The same as before? It shouldn't be too much
work to describe what it means instead (and mention that you are not
changing the meaning as a sidenote) to help the readers, something
like "The function returns the position in the used_atom[] array (as
before) for the given atom, or a negative value to signal an error".
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v4 1/5] ref-filter: start adding strbufs with errors
2018-03-19 13:01 ` [PATCH v3 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
` (3 preceding siblings ...)
2018-03-19 13:01 ` [PATCH v3 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-20 16:05 ` Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
` (4 more replies)
4 siblings, 5 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-20 16:05 UTC (permalink / raw)
To: git
This is a first step in removing die() calls from ref-filter
formatting logic, so that it could be used by other commands
that do not want to die during formatting process.
die() calls related to bugs in code will not be touched in this patch.
Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.
This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
builtin/branch.c | 7 +++++--
ref-filter.c | 17 ++++++++++++-----
ref-filter.h | 7 ++++---
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
- struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
ref_array_sort(sorting, &array);
for (i = 0; i < array.nr; i++) {
- format_ref_array_item(array.items[i], format, &out);
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ if (format_ref_array_item(array.items[i], format, &out, &err))
+ die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are incompatible");
/* format to a string_list to let print_columns() do its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+ strbuf_release(&err);
strbuf_release(&out);
}
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..54fae00bdd410 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
}
}
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
const struct ref_format *format,
- struct strbuf *final_buf)
+ struct strbuf *final_buf,
+ struct strbuf *error_buf)
{
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(&resetv, &state);
}
- if (state.stack->prev)
- die(_("format: %%(end) atom missing"));
+ if (state.stack->prev) {
+ strbuf_addstr(error_buf, _("format: %(end) atom missing"));
+ return -1;
+ }
strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+ return 0;
}
void show_ref_array_item(struct ref_array_item *info,
const struct ref_format *format)
{
struct strbuf final_buf = STRBUF_INIT;
+ struct strbuf error_buf = STRBUF_INIT;
- format_ref_array_item(info, format, &final_buf);
+ if (format_ref_array_item(info, format, &final_buf, &error_buf))
+ die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ strbuf_release(&error_buf);
strbuf_release(&final_buf);
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *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 struct ref_format *format,
- struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
/* Print the ref using the given format and quote_style */
void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
/* Parse a single sort specifier and add it to the list */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v4 3/5] ref-filter: change parsing function error handling
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
@ 2018-03-20 16:05 ` Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 4/5] ref-filter: add return value to parsers Olga Telezhnaya
` (3 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-20 16:05 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
The function returns the position in the used_atom[] array
(as before) for the given atom, or -1 to signal an error (there
could be more negative error codes further if necessary).
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index d120360104806..2313a33f0baa4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
- const char *atom, const char *ep)
+ const char *atom, const char *ep,
+ struct strbuf *err)
{
const char *sp;
const char *arg;
@@ -406,8 +407,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
- if (ep <= sp)
- die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+ if (ep <= sp) {
+ strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -432,8 +435,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,
break;
}
- if (ARRAY_SIZE(valid_atom) <= i)
- die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+ if (ARRAY_SIZE(valid_atom) <= i) {
+ strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), atom);
+ return -1;
+ }
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -725,17 +730,21 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ at = parse_ref_filter_atom(format, sp + 2, ep, &err);
+ if (at < 0)
+ die("%s", err.buf);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
format->need_color_reset_at_eol = !!strcmp(color, "reset");
+ strbuf_release(&err);
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,13 +2163,15 @@ int format_ref_array_item(struct ref_array_item *info,
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+ int pos;
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+ if (pos < 0)
+ return -1;
+ get_ref_atom_value(info, pos, &atomv);
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
@@ -2215,7 +2226,12 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ struct strbuf err = STRBUF_INIT;
+ int res = parse_ref_filter_atom(&dummy, atom, end, &err);
+ if (res < 0)
+ die("%s", err.buf);
+ strbuf_release(&err);
+ return res;
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v4 4/5] ref-filter: add return value to parsers
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-20 16:05 ` Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
` (2 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-20 16:05 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parsers by adding return value and
strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure (there
could be more error codes further if necessary).
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 118 insertions(+), 59 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 2313a33f0baa4..b90cec1056954 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,28 @@ static struct used_atom {
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *color_value, struct strbuf *err)
{
- if (!color_value)
- die(_("expected format: %%(color:<color>)"));
- if (color_parse(color_value, atom->u.color) < 0)
- die(_("unrecognized color: %%(color:%s)"), color_value);
+ if (!color_value) {
+ strbuf_addstr(err, _("expected format: %(color:<color>)"));
+ return -1;
+ }
+ if (color_parse(color_value, atom->u.color) < 0) {
+ strbuf_addf(err, _("unrecognized color: %%(color:%s)"), color_value);
+ return -1;
+ }
/*
* We check this after we've parsed the color, which lets us complain
* about syntactically bogus color names even if they won't be used.
*/
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+ return 0;
}
-static void refname_atom_parser_internal(struct refname_atom *atom,
- const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,
+ const char *name, struct strbuf *err)
{
if (!arg)
atom->option = R_NORMAL;
@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
else if (skip_prefix(arg, "lstrip=", &arg) ||
skip_prefix(arg, "strip=", &arg)) {
atom->option = R_LSTRIP;
- if (strtol_i(arg, 10, &atom->lstrip))
- die(_("Integer value expected refname:lstrip=%s"), arg);
+ if (strtol_i(arg, 10, &atom->lstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:lstrip=%s"), arg);
+ return -1;
+ }
} else if (skip_prefix(arg, "rstrip=", &arg)) {
atom->option = R_RSTRIP;
- if (strtol_i(arg, 10, &atom->rstrip))
- die(_("Integer value expected refname:rstrip=%s"), arg);
- } else
- die(_("unrecognized %%(%s) argument: %s"), name, arg);
+ if (strtol_i(arg, 10, &atom->rstrip)) {
+ strbuf_addf(err, _("Integer value expected refname:rstrip=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, arg);
+ return -1;
+ }
+ return 0;
}
-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
if (!arg) {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
- return;
+ return refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err);
}
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
+ if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err))
+ return -1;
}
}
string_list_clear(¶ms, 0);
+ return 0;
}
-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(body) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(body) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_BODY_DEP;
+ return 0;
}
-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- if (arg)
- die(_("%%(subject) does not take arguments"));
+ if (arg) {
+ strbuf_addstr(err, _("%(subject) does not take arguments"));
+ return -1;
+ }
atom->u.contents.option = C_SUB;
+ return 0;
}
-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.trailer_opts.unfold = 1;
else if (!strcmp(s, "only"))
atom->u.contents.trailer_opts.only_trailers = 1;
- else
- die(_("unknown %%(trailers) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
+ return -1;
+ }
}
}
atom->u.contents.option = C_TRAILERS;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.contents.option = C_BARE;
@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.option = C_SUB;
else if (skip_prefix(arg, "trailers", &arg)) {
skip_prefix(arg, ":", &arg);
- trailers_atom_parser(format, atom, *arg ? arg : NULL);
+ if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+ return -1;
} else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
- if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
- die(_("positive value expected contents:lines=%s"), arg);
- } else
- die(_("unrecognized %%(contents) argument: %s"), arg);
+ if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {
+ strbuf_addf(err, _("positive value expected contents:lines=%s"), arg);
+ return -1;
+ }
+ } else {
+ strbuf_addf(err, _("unrecognized %%(contents) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.objectname.option = O_FULL;
@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
else if (skip_prefix(arg, "short=", &arg)) {
atom->u.objectname.option = O_LENGTH;
if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
- atom->u.objectname.length == 0)
- die(_("positive value expected objectname:short=%s"), arg);
+ atom->u.objectname.length == 0) {
+ strbuf_addf(err, _("positive value expected objectname:short=%s"), arg);
+ return -1;
+ }
if (atom->u.objectname.length < MINIMUM_ABBREV)
atom->u.objectname.length = MINIMUM_ABBREV;
- } else
- die(_("unrecognized %%(objectname) argument: %s"), arg);
+ } else {
+ strbuf_addf(err, _("unrecognized %%(objectname) argument: %s"), arg);
+ return -1;
+ }
+ return 0;
}
-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
+ return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
}
static align_type parse_align_position(const char *s)
@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)
return -1;
}
-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct align *align = &atom->u.align;
struct string_list params = STRING_LIST_INIT_DUP;
int i;
unsigned int width = ~0U;
- if (!arg)
- die(_("expected format: %%(align:<width>,<position>)"));
+ if (!arg) {
+ strbuf_addstr(err, _("expected format: %(align:<width>,<position>)"));
+ return -1;
+ }
align->position = ALIGN_LEFT;
@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
if (skip_prefix(s, "position=", &s)) {
position = parse_align_position(s);
- if (position < 0)
- die(_("unrecognized position:%s"), s);
+ if (position < 0) {
+ strbuf_addf(err, _("unrecognized position:%s"), s);
+ return -1;
+ }
align->position = position;
} else if (skip_prefix(s, "width=", &s)) {
- if (strtoul_ui(s, 10, &width))
- die(_("unrecognized width:%s"), s);
+ if (strtoul_ui(s, 10, &width)) {
+ strbuf_addf(err, _("unrecognized width:%s"), s);
+ return -1;
+ }
} else if (!strtoul_ui(s, 10, &width))
;
else if ((position = parse_align_position(s)) >= 0)
align->position = position;
- else
- die(_("unrecognized %%(align) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unrecognized %%(align) argument: %s"), s);
+ return -1;
+ }
}
- if (width == ~0U)
- die(_("positive width expected with the %%(align) atom"));
+ if (width == ~0U) {
+ strbuf_addstr(err, _("positive width expected with the %(align) atom"));
+ return -1;
+ }
align->width = width;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg) {
atom->u.if_then_else.cmp_status = COMPARE_NONE;
- return;
+ return 0;
} else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
} else {
- die(_("unrecognized %%(if) argument: %s"), arg);
+ strbuf_addf(err, _("unrecognized %%(if) argument: %s"), arg);
+ return -1;
}
+ return 0;
}
-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
+ return 0;
}
static struct {
const char *name;
cmp_type cmp_type;
- void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
+ int (*parser)(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
@@ -457,8 +516,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
}
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
- if (valid_atom[i].parser)
- valid_atom[i].parser(format, &used_atom[at], arg);
+ if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
+ return -1;
if (*atom == '*')
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-20 16:05 ` [PATCH v4 4/5] ref-filter: add return value to parsers Olga Telezhnaya
@ 2018-03-20 16:05 ` Olga Telezhnaya
2018-03-20 18:18 ` Eric Sunshine
2018-03-20 16:05 ` [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-20 16:05 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of handlers by adding return value
and strbuf parameter for errors.
Return value equals 0 upon success and -1 upon failure (there could be
more error codes further if necessary). Upon failure, error message is
appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 48 insertions(+), 23 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 54fae00bdd410..d120360104806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
+ int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
};
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
}
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
/*
* Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
quote_formatting(&state->stack->output, v->s, state->quote_style);
else
strbuf_addstr(&state->stack->output, v->s);
+ return 0;
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)
strbuf_release(&s);
}
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = &atomv->atom->u.align;
+ return 0;
}
static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
free(if_then_else);
}
-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+ return 0;
}
static int is_empty(const char *s)
@@ -596,19 +603,24 @@ static int is_empty(const char *s)
return 1;
}
-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(then) atom used more than once"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(then) atom used after %%(else)"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(then) atom used without an %(if) atom"));
+ return -1;
+ } else if (if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used more than once"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(then) atom used after %(else)"));
+ return -1;
+ }
if_then_else->then_atom_seen = 1;
/*
* If the 'equals' or 'notequals' attribute is used then
@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
+ return 0;
}
-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
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_seen)
- die(_("format: %%(else) atom used without a %%(then) atom"));
- if (if_then_else->else_atom_seen)
- die(_("format: %%(else) atom used more than once"));
+ if (!if_then_else) {
+ strbuf_addstr(err, _("format: %(else) atom used without an %(if) atom"));
+ return -1;
+ } else if (!if_then_else->then_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used without a %(then) atom"));
+ return -1;
+ } else if (if_then_else->else_atom_seen) {
+ strbuf_addstr(err, _("format: %(else) atom used more than once"));
+ return -1;
+ }
if_then_else->else_atom_seen = 1;
push_stack_element(&state->stack);
state->stack->at_end_data = prev->at_end_data;
state->stack->at_end = prev->at_end;
+ return 0;
}
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *current = state->stack;
struct strbuf s = STRBUF_INIT;
- if (!current->at_end)
- die(_("format: %%(end) atom used without corresponding atom"));
+ if (!current->at_end) {
+ strbuf_addstr(err, _("format: %(end) atom used without corresponding atom"));
+ return -1;
+ }
current->at_end(&state->stack);
/* Stack may have been popped within at_end(), hence reset the current pointer */
@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
}
strbuf_release(&s);
pop_stack_element(&state->stack);
+ return 0;
}
/*
@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,
get_ref_atom_value(info,
parse_ref_filter_atom(format, sp + 2, ep),
&atomv);
- atomv->handler(atomv, &state);
+ if (atomv->handler(atomv, &state, error_buf))
+ return -1;
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (format->need_color_reset_at_eol) {
struct atom_value resetv;
resetv.s = GIT_COLOR_RESET;
- append_atom(&resetv, &state);
+ if (append_atom(&resetv, &state, error_buf))
+ return -1;
}
if (state.stack->prev) {
strbuf_addstr(error_buf, _("format: %(end) atom missing"));
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers
2018-03-20 16:05 ` [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-20 18:18 ` Eric Sunshine
0 siblings, 0 replies; 55+ messages in thread
From: Eric Sunshine @ 2018-03-20 18:18 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: Git List
On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
<olyatelezhnaya@gmail.com> wrote:
> Continue removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
> [...]
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -596,19 +603,24 @@ static int is_empty(const char *s)
> +static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
> + struct strbuf *err)
> {
> 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_seen)
> - die(_("format: %%(then) atom used more than once"));
> - if (if_then_else->else_atom_seen)
> - die(_("format: %%(then) atom used after %%(else)"));
> + if (!if_then_else) {
> + strbuf_addstr(err, _("format: %(then) atom used without an %(if) atom"));
> + return -1;
> + } else if (if_then_else->then_atom_seen) {
> + strbuf_addstr(err, _("format: %(then) atom used more than once"));
> + return -1;
> + } else if (if_then_else->else_atom_seen) {
> + strbuf_addstr(err, _("format: %(then) atom used after %(else)"));
> + return -1;
> + }
This pattern of transforming:
if (cond)
die("...");
to:
if (cond) {
strbuf_add*(...);
return -1;
}
is repeated many, many times throughout this patch series and makes
for quite noisy diffs. Such repetition and noise suggests that this
patch series (and other similar ones) could benefit from a convenience
function similar to the existing error() function. For instance:
int strbuf_error(struct strbuf *buf, const char *fmt, ...);
which appends the formatted message to 'buf' and unconditionally
returns -1. Thus, the above transformation simplifies to:
if (cond)
return strbuf_error(&err, "...", ...);
which makes for a much less noisy diff, thus is easier to review.
A new patch introducing such a function at the head of this patch
series might be welcome.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
` (2 preceding siblings ...)
2018-03-20 16:05 ` [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-20 16:05 ` Olga Telezhnaya
2018-03-20 18:19 ` Eric Sunshine
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
4 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-20 16:05 UTC (permalink / raw)
To: git
Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure (there
could be more error codes further if necessary).
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index b90cec1056954..85f971663a4aa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
- if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
+ if (!buf) {
+ strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+ ref->refname);
+ if (!eaten)
+ free(buf);
+ return -1;
+ }
+ if (!*obj) {
+ strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ if (!eaten)
+ free(buf);
+ return -1;
+ }
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+ return 0;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
if (pos < 0)
return -1;
- get_ref_atom_value(info, pos, &atomv);
+ if (get_ref_atom_value(info, pos, &atomv, error_buf))
+ return -1;
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-20 16:05 ` [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
@ 2018-03-20 18:19 ` Eric Sunshine
2018-03-20 22:30 ` Junio C Hamano
0 siblings, 1 reply; 55+ messages in thread
From: Eric Sunshine @ 2018-03-20 18:19 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: Git List
On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
<olyatelezhnaya@gmail.com> wrote:
> ref-filter: get_ref_atom_value() error handling
This doesn't tell us much about what this patch is doing. Perhaps a
better subject would be:
ref-filter: libify get_ref_atom_value()
> Finish removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
> Return value equals 0 upon success and -1 upon failure (there
> could be more error codes further if necessary).
> Upon failure, error message is appended to the strbuf.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> + int deref, struct object **obj, struct strbuf *err)
> {
> void *buf = get_obj(oid, obj, &size, &eaten);
> - if (!buf)
> - die(_("missing object %s for %s"),
> - oid_to_hex(oid), ref->refname);
> - if (!*obj)
> - die(_("parse_object_buffer failed on %s for %s"),
> - oid_to_hex(oid), ref->refname);
> -
> + if (!buf) {
> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
> + ref->refname);
> + if (!eaten)
> + free(buf);
Since we are inside the 'if (!buf)' conditional, we _know_ that 'buf'
is NULL, therefore this free(buff) is unnecessary.
> + return -1;
> + }
> + if (!*obj) {
> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> + oid_to_hex(oid), ref->refname);
> + if (!eaten)
> + free(buf);
> + return -1;
> + }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> + return 0;
> }
Overall, with the need for resource cleanup, this function becomes
unusually noisy after this change. It could be tamed by doing
something like this:
int ret = 0;
void *buf = get_obj(oid, obj, &size, &eaten);
if (!buf)
ret = strbuf_error(_("missing object %s for %s"),
oid_to_hex(oid), ref->refname);
else if (!*obj)
ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
oid_to_hex(oid), ref->refname);
else
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
return ret;
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-20 18:19 ` Eric Sunshine
@ 2018-03-20 22:30 ` Junio C Hamano
2018-03-20 22:50 ` Eric Sunshine
0 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2018-03-20 22:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Olga Telezhnaya, Git List
Eric Sunshine <sunshine@sunshineco.com> writes:
> Overall, with the need for resource cleanup, this function becomes
> unusually noisy after this change. It could be tamed by doing
> something like this:
>
> int ret = 0;
> void *buf = get_obj(oid, obj, &size, &eaten);
> if (!buf)
> ret = strbuf_error(_("missing object %s for %s"),
> oid_to_hex(oid), ref->refname);
> else if (!*obj)
> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
> oid_to_hex(oid), ref->refname);
> else
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> return ret;
I have no idea what strbuf_error() that does not take any strbuf is
doing, but I think you can initialize ret to -1 (i.e. assume the
worst at the beginning), and then make the "ok, we didn't get any
errors" case do
else {
grab_values(...);
ret = 0;
}
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-20 22:30 ` Junio C Hamano
@ 2018-03-20 22:50 ` Eric Sunshine
2018-03-21 19:30 ` Junio C Hamano
0 siblings, 1 reply; 55+ messages in thread
From: Eric Sunshine @ 2018-03-20 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Olga Telezhnaya, Git List
On Tue, Mar 20, 2018 at 6:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> int ret = 0;
>> void *buf = get_obj(oid, obj, &size, &eaten);
>> if (!buf)
>> ret = strbuf_error(_("missing object %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else if (!*obj)
>> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else
>> grab_values(ref->value, deref, *obj, buf, size);
>> if (!eaten)
>> free(buf);
>> return ret;
>
> I have no idea what strbuf_error() that does not take any strbuf is
> doing,...
strbuf_error() was a possibility proposed in [1], and it does take a
strbuf. Failure to pass in a strbuf here is just a typo.
> ... but I think you can initialize ret to -1 (i.e. assume the
> worst at the beginning), and then make the "ok, we didn't get any
> errors" case do
>
> else {
> grab_values(...);
> ret = 0;
> }
Yes, that also works.
[1]: https://public-inbox.org/git/CAPig+cT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V+jYZow@mail.gmail.com/
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-20 22:50 ` Eric Sunshine
@ 2018-03-21 19:30 ` Junio C Hamano
2018-03-21 19:58 ` Eric Sunshine
0 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2018-03-21 19:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Olga Telezhnaya, Git List
Eric Sunshine <sunshine@sunshineco.com> writes:
>> I have no idea what strbuf_error() that does not take any strbuf is
>> doing,...
>
> strbuf_error() was a possibility proposed in [1], and it does take a
> strbuf. Failure to pass in a strbuf here is just a typo.
I've seen it; I just thought it was a joke and not a serious
suggestion.
A macro or helper function that is local to the file might be OK,
but I do not think "strbuf_error()" is a useful abstraction that is
generic enough in the first place (the questions to ask yourself to
think about it are: Why should it be limited to return -1? Why
should it be limited to always do the addf() to a strbuf?).
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
2018-03-21 19:30 ` Junio C Hamano
@ 2018-03-21 19:58 ` Eric Sunshine
0 siblings, 0 replies; 55+ messages in thread
From: Eric Sunshine @ 2018-03-21 19:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Olga Telezhnaya, Git List
On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> strbuf_error() was a possibility proposed in [1], and it does take a
>> strbuf. Failure to pass in a strbuf here is just a typo.
>
> I've seen it; I just thought it was a joke and not a serious
> suggestion.
> A macro or helper function that is local to the file might be OK,
My thought all along was that this convenience helper (in whatever
form) should start life local to ref-filter.c, and only be published
more widely if found to be generally useful. Unfortunately, I forgot
to state so explicitly when writing the review. Suggesting the name
strbuf_error() didn't help to convey that thought either. My bad.
> but I do not think "strbuf_error()" is a useful abstraction that is
> generic enough in the first place (the questions to ask yourself to
> think about it are: Why should it be limited to return -1? Why
> should it be limited to always do the addf() to a strbuf?).
There is some precedent in the existing error() function. As with
error(), as a _convenience_ function, it does not necessarily have to
be universally general. That it simplifies a reasonably large body of
code may be justification enough, despite it shortcomings. I don't
feel strongly about it, though, and, as noted above, agree that it can
be local to ref-filter.c.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 1/6] strbuf: add shortcut to work with error messages
2018-03-20 16:05 ` [PATCH v4 1/5] ref-filter: start adding strbufs with errors Olga Telezhnaya
` (3 preceding siblings ...)
2018-03-20 16:05 ` [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 5/6] ref-filter: add return value to parsers Olga Telezhnaya
` (6 more replies)
4 siblings, 7 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
Add function strbuf_error() that helps to save few lines of code.
Function expands fmt with placeholders, append resulting error message
to strbuf *err, and return error code ret.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
strbuf.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/strbuf.h b/strbuf.h
index e6cae5f4398c8..fa66d4835f1a7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
__attribute__((format (printf, 1, 2)))
char *xstrfmt(const char *fmt, ...);
+/*
+ * Expand error message, append it to strbuf *err, then return error code ret.
+ * Allow to save few lines of code.
+ */
+static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ strbuf_vaddf(err, fmt, ap);
+ va_end(ap);
+ return ret;
+}
+
#endif /* STRBUF_H */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 5/6] ref-filter: add return value to parsers
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
` (5 subsequent siblings)
6 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parsers by adding return value and
strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 112 ++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 68 insertions(+), 44 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index ca38728b4c998..0ef7386b5fd20 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,25 @@ static struct used_atom {
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *color_value, struct strbuf *err)
{
if (!color_value)
- die(_("expected format: %%(color:<color>)"));
+ return strbuf_error(err, -1, _("expected format: %%(color:<color>)"));
if (color_parse(color_value, atom->u.color) < 0)
- die(_("unrecognized color: %%(color:%s)"), color_value);
+ return strbuf_error(err, -1, _("unrecognized color: %%(color:%s)"),
+ color_value);
/*
* We check this after we've parsed the color, which lets us complain
* about syntactically bogus color names even if they won't be used.
*/
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+ return 0;
}
-static void refname_atom_parser_internal(struct refname_atom *atom,
- const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,
+ const char *name, struct strbuf *err)
{
if (!arg)
atom->option = R_NORMAL;
@@ -126,16 +129,18 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
skip_prefix(arg, "strip=", &arg)) {
atom->option = R_LSTRIP;
if (strtol_i(arg, 10, &atom->lstrip))
- die(_("Integer value expected refname:lstrip=%s"), arg);
+ return strbuf_error(err, -1, _("Integer value expected refname:lstrip=%s"), arg);
} else if (skip_prefix(arg, "rstrip=", &arg)) {
atom->option = R_RSTRIP;
if (strtol_i(arg, 10, &atom->rstrip))
- die(_("Integer value expected refname:rstrip=%s"), arg);
+ return strbuf_error(err, -1, _("Integer value expected refname:rstrip=%s"), arg);
} else
- die(_("unrecognized %%(%s) argument: %s"), name, arg);
+ return strbuf_error(err, -1, _("unrecognized %%(%s) argument: %s"), name, arg);
+ return 0;
}
-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +150,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
if (!arg) {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
- return;
+ return refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err);
}
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +174,36 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
+ if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err))
+ return -1;
}
}
string_list_clear(¶ms, 0);
+ return 0;
}
-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (arg)
- die(_("%%(body) does not take arguments"));
+ return strbuf_error(err, -1, _("%%(body) does not take arguments"));
atom->u.contents.option = C_BODY_DEP;
+ return 0;
}
-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (arg)
- die(_("%%(subject) does not take arguments"));
+ return strbuf_error(err, -1, _("%%(subject) does not take arguments"));
atom->u.contents.option = C_SUB;
+ return 0;
}
-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -206,14 +217,16 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at
else if (!strcmp(s, "only"))
atom->u.contents.trailer_opts.only_trailers = 1;
else
- die(_("unknown %%(trailers) argument: %s"), s);
+ return strbuf_error(err, -1, _("unknown %%(trailers) argument: %s"), s);
}
}
atom->u.contents.option = C_TRAILERS;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.contents.option = C_BARE;
@@ -225,16 +238,19 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.option = C_SUB;
else if (skip_prefix(arg, "trailers", &arg)) {
skip_prefix(arg, ":", &arg);
- trailers_atom_parser(format, atom, *arg ? arg : NULL);
+ if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+ return -1;
} else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
- die(_("positive value expected contents:lines=%s"), arg);
+ return strbuf_error(err, -1, _("positive value expected contents:lines=%s"), arg);
} else
- die(_("unrecognized %%(contents) argument: %s"), arg);
+ return strbuf_error(err, -1, _("unrecognized %%(contents) argument: %s"), arg);
+ return 0;
}
-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.objectname.option = O_FULL;
@@ -244,16 +260,18 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
atom->u.objectname.option = O_LENGTH;
if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
atom->u.objectname.length == 0)
- die(_("positive value expected objectname:short=%s"), arg);
+ return strbuf_error(err, -1, _("positive value expected objectname:short=%s"), arg);
if (atom->u.objectname.length < MINIMUM_ABBREV)
atom->u.objectname.length = MINIMUM_ABBREV;
} else
- die(_("unrecognized %%(objectname) argument: %s"), arg);
+ return strbuf_error(err, -1, _("unrecognized %%(objectname) argument: %s"), arg);
+ return 0;
}
-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
+ return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
}
static align_type parse_align_position(const char *s)
@@ -267,7 +285,8 @@ static align_type parse_align_position(const char *s)
return -1;
}
-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct align *align = &atom->u.align;
struct string_list params = STRING_LIST_INIT_DUP;
@@ -275,7 +294,7 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
unsigned int width = ~0U;
if (!arg)
- die(_("expected format: %%(align:<width>,<position>)"));
+ return strbuf_error(err, -1, _("expected format: %%(align:<width>,<position>)"));
align->position = ALIGN_LEFT;
@@ -287,48 +306,53 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
if (skip_prefix(s, "position=", &s)) {
position = parse_align_position(s);
if (position < 0)
- die(_("unrecognized position:%s"), s);
+ return strbuf_error(err, -1, _("unrecognized position:%s"), s);
align->position = position;
} else if (skip_prefix(s, "width=", &s)) {
if (strtoul_ui(s, 10, &width))
- die(_("unrecognized width:%s"), s);
+ return strbuf_error(err, -1, _("unrecognized width:%s"), s);
} else if (!strtoul_ui(s, 10, &width))
;
else if ((position = parse_align_position(s)) >= 0)
align->position = position;
else
- die(_("unrecognized %%(align) argument: %s"), s);
+ return strbuf_error(err, -1, _("unrecognized %%(align) argument: %s"), s);
}
if (width == ~0U)
- die(_("positive width expected with the %%(align) atom"));
+ return strbuf_error(err, -1, _("positive width expected with the %%(align) atom"));
align->width = width;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg) {
atom->u.if_then_else.cmp_status = COMPARE_NONE;
- return;
+ return 0;
} else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
- } else {
- die(_("unrecognized %%(if) argument: %s"), arg);
- }
+ } else
+ return strbuf_error(err, -1, _("unrecognized %%(if) argument: %s"), arg);
+ return 0;
}
-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
+ return 0;
}
static struct {
const char *name;
cmp_type cmp_type;
- void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
+ int (*parser)(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
@@ -455,8 +479,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
}
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
- if (valid_atom[i].parser)
- valid_atom[i].parser(format, &used_atom[at], arg);
+ if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
+ return -1;
if (*atom == '*')
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 6/6] ref-filter: libify get_ref_atom_value()
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 5/6] ref-filter: add return value to parsers Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
` (4 subsequent siblings)
6 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 49 ++++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 0ef7386b5fd20..52bb84398dc8d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1398,28 +1398,30 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
+ int ret = 0;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
- grab_values(ref->value, deref, *obj, buf, size);
+ ret = strbuf_error(err, -1, _("missing object %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ else if (!*obj)
+ ret = strbuf_error(err, -1, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ else
+ grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+ return ret;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1541,16 +1543,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1564,20 +1567,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2101,9 +2107,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2183,7 +2193,8 @@ int format_ref_array_item(struct ref_array_item *info,
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
if (pos < 0)
return -1;
- get_ref_atom_value(info, pos, &atomv);
+ if (get_ref_atom_value(info, pos, &atomv, error_buf))
+ return -1;
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 4/6] ref-filter: change parsing function error handling
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 5/6] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 20:36 ` Junio C Hamano
2018-03-21 18:28 ` [PATCH v5 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
` (3 subsequent siblings)
6 siblings, 1 reply; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
The function returns the position in the used_atom[] array
(as before) for the given atom, or -1 to signal an error.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index f79c8c477f1dc..ca38728b4c998 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
- const char *atom, const char *ep)
+ const char *atom, const char *ep,
+ struct strbuf *err)
{
const char *sp;
const char *arg;
@@ -407,7 +408,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
if (*sp == '*' && sp < ep)
sp++; /* deref */
if (ep <= sp)
- die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+ return strbuf_error(err, -1, _("malformed field name: %.*s"),
+ (int)(ep-atom), atom);
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -433,7 +435,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
if (ARRAY_SIZE(valid_atom) <= i)
- die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+ return strbuf_error(err, -1, _("unknown field name: %.*s"),
+ (int)(ep-atom), atom);
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -715,17 +718,21 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ at = parse_ref_filter_atom(format, sp + 2, ep, &err);
+ if (at < 0)
+ die("%s", err.buf);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
format->need_color_reset_at_eol = !!strcmp(color, "reset");
+ strbuf_release(&err);
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2144,13 +2151,15 @@ int format_ref_array_item(struct ref_array_item *info,
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+ int pos;
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+ if (pos < 0)
+ return -1;
+ get_ref_atom_value(info, pos, &atomv);
if (atomv->handler(atomv, &state, error_buf))
return -1;
}
@@ -2203,7 +2212,12 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ struct strbuf err = STRBUF_INIT;
+ int res = parse_ref_filter_atom(&dummy, atom, end, &err);
+ if (res < 0)
+ die("%s", err.buf);
+ strbuf_release(&err);
+ return res;
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v5 4/6] ref-filter: change parsing function error handling
2018-03-21 18:28 ` [PATCH v5 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-21 20:36 ` Junio C Hamano
2018-03-23 6:56 ` Оля Тележная
0 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2018-03-21 20:36 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> @@ -2144,13 +2151,15 @@ int format_ref_array_item(struct ref_array_item *info,
>
> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> struct atom_value *atomv;
> + int pos;
>
> ep = strchr(sp, ')');
> if (cp < sp)
> append_literal(cp, sp, &state);
> - get_ref_atom_value(info,
> - parse_ref_filter_atom(format, sp + 2, ep),
> - &atomv);
> + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
> + if (pos < 0)
> + return -1;
> + get_ref_atom_value(info, pos, &atomv);
> if (atomv->handler(atomv, &state, error_buf))
> return -1;
> }
These error returns leave the formatting state "state" on the stack
holding onto its resources, no?
The only thing the caller of format_ref_array_item() that notices an
error return does is to die even after this series, so in that sense
it does not matter (yet), but it still feels somewhat wrong.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v5 4/6] ref-filter: change parsing function error handling
2018-03-21 20:36 ` Junio C Hamano
@ 2018-03-23 6:56 ` Оля Тележная
0 siblings, 0 replies; 55+ messages in thread
From: Оля Тележная @ 2018-03-23 6:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
2018-03-21 23:36 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> @@ -2144,13 +2151,15 @@ int format_ref_array_item(struct ref_array_item *info,
>>
>> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>> struct atom_value *atomv;
>> + int pos;
>>
>> ep = strchr(sp, ')');
>> if (cp < sp)
>> append_literal(cp, sp, &state);
>> - get_ref_atom_value(info,
>> - parse_ref_filter_atom(format, sp + 2, ep),
>> - &atomv);
>> + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
>> + if (pos < 0)
>> + return -1;
>> + get_ref_atom_value(info, pos, &atomv);
>> if (atomv->handler(atomv, &state, error_buf))
>> return -1;
>> }
>
> These error returns leave the formatting state "state" on the stack
> holding onto its resources, no?
Yes, you are right, thanks a lot!
>
> The only thing the caller of format_ref_array_item() that notices an
> error return does is to die even after this series, so in that sense
> it does not matter (yet), but it still feels somewhat wrong.
>
I am sure I need to fix it anyway, thanks.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 2/6] ref-filter: start adding strbufs with errors
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
` (2 preceding siblings ...)
2018-03-21 18:28 ` [PATCH v5 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 18:28 ` [PATCH v5 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
` (2 subsequent siblings)
6 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
This is a first step in removing die() calls from ref-filter
formatting logic, so that it could be used by other commands
that do not want to die during formatting process.
die() calls related to bugs in code will not be touched in this patch.
Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.
This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
builtin/branch.c | 7 +++++--
ref-filter.c | 13 +++++++++----
ref-filter.h | 7 ++++---
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
- struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
ref_array_sort(sorting, &array);
for (i = 0; i < array.nr; i++) {
- format_ref_array_item(array.items[i], format, &out);
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ if (format_ref_array_item(array.items[i], format, &out, &err))
+ die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are incompatible");
/* format to a string_list to let print_columns() do its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+ strbuf_release(&err);
strbuf_release(&out);
}
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..2c3fb2f003708 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
}
}
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
const struct ref_format *format,
- struct strbuf *final_buf)
+ struct strbuf *final_buf,
+ struct strbuf *error_buf)
{
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2149,18 +2150,22 @@ void format_ref_array_item(struct ref_array_item *info,
append_atom(&resetv, &state);
}
if (state.stack->prev)
- die(_("format: %%(end) atom missing"));
+ return strbuf_error(error_buf, -1, _("format: %%(end) atom missing"));
strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+ return 0;
}
void show_ref_array_item(struct ref_array_item *info,
const struct ref_format *format)
{
struct strbuf final_buf = STRBUF_INIT;
+ struct strbuf error_buf = STRBUF_INIT;
- format_ref_array_item(info, format, &final_buf);
+ if (format_ref_array_item(info, format, &final_buf, &error_buf))
+ die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ strbuf_release(&error_buf);
strbuf_release(&final_buf);
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *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 struct ref_format *format,
- struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
/* Print the ref using the given format and quote_style */
void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
/* Parse a single sort specifier and add it to the list */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 3/6] ref-filter: add return value && strbuf to handlers
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
` (3 preceding siblings ...)
2018-03-21 18:28 ` [PATCH v5 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
@ 2018-03-21 18:28 ` Olga Telezhnaya
2018-03-21 20:20 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Junio C Hamano
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
6 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-21 18:28 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of handlers by adding return value
and strbuf parameter for errors.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 47 +++++++++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 2c3fb2f003708..f79c8c477f1dc 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
+ int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
};
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
}
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
/*
* Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
quote_formatting(&state->stack->output, v->s, state->quote_style);
else
strbuf_addstr(&state->stack->output, v->s);
+ return 0;
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)
strbuf_release(&s);
}
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = &atomv->atom->u.align;
+ return 0;
}
static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
free(if_then_else);
}
-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+ return 0;
}
static int is_empty(const char *s)
@@ -596,7 +603,8 @@ static int is_empty(const char *s)
return 1;
}
-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *cur = state->stack;
struct if_then_else *if_then_else = NULL;
@@ -604,11 +612,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
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"));
+ return strbuf_error(err, -1, _("format: %%(then) atom used without an %%(if) atom"));
if (if_then_else->then_atom_seen)
- die(_("format: %%(then) atom used more than once"));
+ return strbuf_error(err, -1, _("format: %%(then) atom used more than once"));
if (if_then_else->else_atom_seen)
- die(_("format: %%(then) atom used after %%(else)"));
+ return strbuf_error(err, -1, _("format: %%(then) atom used after %%(else)"));
if_then_else->then_atom_seen = 1;
/*
* If the 'equals' or 'notequals' attribute is used then
@@ -624,9 +632,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
+ return 0;
}
-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *prev = state->stack;
struct if_then_else *if_then_else = NULL;
@@ -634,24 +644,26 @@ static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_st
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"));
+ return strbuf_error(err, -1, _("format: %%(else) atom used without an %%(if) atom"));
if (!if_then_else->then_atom_seen)
- die(_("format: %%(else) atom used without a %%(then) atom"));
+ return strbuf_error(err, -1, _("format: %%(else) atom used without a %%(then) atom"));
if (if_then_else->else_atom_seen)
- die(_("format: %%(else) atom used more than once"));
+ return strbuf_error(err, -1, _("format: %%(else) atom used more than once"));
if_then_else->else_atom_seen = 1;
push_stack_element(&state->stack);
state->stack->at_end_data = prev->at_end_data;
state->stack->at_end = prev->at_end;
+ return 0;
}
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *current = state->stack;
struct strbuf s = STRBUF_INIT;
if (!current->at_end)
- die(_("format: %%(end) atom used without corresponding atom"));
+ return strbuf_error(err, -1, _("format: %%(end) atom used without corresponding atom"));
current->at_end(&state->stack);
/* Stack may have been popped within at_end(), hence reset the current pointer */
@@ -668,6 +680,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
}
strbuf_release(&s);
pop_stack_element(&state->stack);
+ return 0;
}
/*
@@ -2138,7 +2151,8 @@ int format_ref_array_item(struct ref_array_item *info,
get_ref_atom_value(info,
parse_ref_filter_atom(format, sp + 2, ep),
&atomv);
- atomv->handler(atomv, &state);
+ if (atomv->handler(atomv, &state, error_buf))
+ return -1;
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2147,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (format->need_color_reset_at_eol) {
struct atom_value resetv;
resetv.s = GIT_COLOR_RESET;
- append_atom(&resetv, &state);
+ if (append_atom(&resetv, &state, error_buf))
+ return -1;
}
if (state.stack->prev)
return strbuf_error(error_buf, -1, _("format: %%(end) atom missing"));
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
` (4 preceding siblings ...)
2018-03-21 18:28 ` [PATCH v5 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-21 20:20 ` Junio C Hamano
2018-03-23 6:48 ` Оля Тележная
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
6 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2018-03-21 20:20 UTC (permalink / raw)
To: Olga Telezhnaya; +Cc: git
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> Add function strbuf_error() that helps to save few lines of code.
> Function expands fmt with placeholders, append resulting error message
> to strbuf *err, and return error code ret.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> strbuf.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/strbuf.h b/strbuf.h
> index e6cae5f4398c8..fa66d4835f1a7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
> __attribute__((format (printf, 1, 2)))
> char *xstrfmt(const char *fmt, ...);
>
> +/*
> + * Expand error message, append it to strbuf *err, then return error code ret.
> + * Allow to save few lines of code.
> + */
> +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...)
> +{
With this function, err does not have to be an error message, and
ret does not have to be negative. Hence strbuf_error() is a wrong
name for the wrapper.
It somewhat is bothersome to see that this is inlined; if it is
meant for error codepath, it probably shouldn't have to be.
> + va_list ap;
> + va_start(ap, fmt);
> + strbuf_vaddf(err, fmt, ap);
> + va_end(ap);
> + return ret;
> +}
> +
> #endif /* STRBUF_H */
Quite honestly, I am not sure if it is worth to be in strbuf.h; it
feels a bit too specific to the immediate need for these five
patches and nowhere else. Are there many existing calls to
strbuf_addf() immediately followed by an "return" of an integer,
that can be simplified by using this helper? I see quite a many
instances of addf() soon followed by "return -1" in
refs/files-backend.c, but they are not immediately adjacent to each
other, and won't be helped.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages
2018-03-21 20:20 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Junio C Hamano
@ 2018-03-23 6:48 ` Оля Тележная
0 siblings, 0 replies; 55+ messages in thread
From: Оля Тележная @ 2018-03-23 6:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
2018-03-21 23:20 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Add function strbuf_error() that helps to save few lines of code.
>> Function expands fmt with placeholders, append resulting error message
>> to strbuf *err, and return error code ret.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> ---
>> strbuf.h | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/strbuf.h b/strbuf.h
>> index e6cae5f4398c8..fa66d4835f1a7 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
>> __attribute__((format (printf, 1, 2)))
>> char *xstrfmt(const char *fmt, ...);
>>
>> +/*
>> + * Expand error message, append it to strbuf *err, then return error code ret.
>> + * Allow to save few lines of code.
>> + */
>> +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...)
>> +{
>
> With this function, err does not have to be an error message, and
> ret does not have to be negative. Hence strbuf_error() is a wrong
> name for the wrapper.
>
> It somewhat is bothersome to see that this is inlined; if it is
> meant for error codepath, it probably shouldn't have to be.
>
>> + va_list ap;
>> + va_start(ap, fmt);
>> + strbuf_vaddf(err, fmt, ap);
>> + va_end(ap);
>> + return ret;
>> +}
>> +
>> #endif /* STRBUF_H */
>
> Quite honestly, I am not sure if it is worth to be in strbuf.h; it
> feels a bit too specific to the immediate need for these five
> patches and nowhere else. Are there many existing calls to
> strbuf_addf() immediately followed by an "return" of an integer,
> that can be simplified by using this helper? I see quite a many
> instances of addf() soon followed by "return -1" in
> refs/files-backend.c, but they are not immediately adjacent to each
> other, and won't be helped.
Summarizing all that we discussed: I have 2 options how to continue
this patch. I can revert to v4, or I can replace new function in
strbuf.h with similar macro in ref-filter.c (I mean, there would be no
changes in strbuf.h and one new macro in ref-filter.c). What do you
like more?
Thanks!
Olga
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs
2018-03-21 18:28 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Olga Telezhnaya
` (5 preceding siblings ...)
2018-03-21 20:20 ` [PATCH v5 1/6] strbuf: add shortcut to work with error messages Junio C Hamano
@ 2018-03-29 12:49 ` Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 5/6] ref-filter: add return value to parsers Olga Telezhnaya
` (4 more replies)
6 siblings, 5 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
Add function strbuf_addf_ret() that helps to save a few lines of code.
Function expands fmt with placeholders, append resulting message
to strbuf *sb, and return error code ret.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..0c8d1589cf316 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,6 +101,19 @@ static struct used_atom {
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
+/*
+ * Expand string, append it to strbuf *sb, then return error code ret.
+ * Allow to save few lines of code.
+ */
+static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ strbuf_vaddf(sb, fmt, ap);
+ va_end(ap);
+ return ret;
+}
+
static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
{
if (!color_value)
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v6 5/6] ref-filter: add return value to parsers
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
@ 2018-03-29 12:49 ` Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
` (3 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parsers by adding return value and
strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 138 ++++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 89 insertions(+), 49 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 93fa6b4e5e63d..3f85ef64267d9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -114,22 +114,25 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
return ret;
}
-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *color_value, struct strbuf *err)
{
if (!color_value)
- die(_("expected format: %%(color:<color>)"));
+ return strbuf_addf_ret(err, -1, _("expected format: %%(color:<color>)"));
if (color_parse(color_value, atom->u.color) < 0)
- die(_("unrecognized color: %%(color:%s)"), color_value);
+ return strbuf_addf_ret(err, -1, _("unrecognized color: %%(color:%s)"),
+ color_value);
/*
* We check this after we've parsed the color, which lets us complain
* about syntactically bogus color names even if they won't be used.
*/
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+ return 0;
}
-static void refname_atom_parser_internal(struct refname_atom *atom,
- const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,
+ const char *name, struct strbuf *err)
{
if (!arg)
atom->option = R_NORMAL;
@@ -139,16 +142,18 @@ static void refname_atom_parser_internal(struct refname_atom *atom,
skip_prefix(arg, "strip=", &arg)) {
atom->option = R_LSTRIP;
if (strtol_i(arg, 10, &atom->lstrip))
- die(_("Integer value expected refname:lstrip=%s"), arg);
+ return strbuf_addf_ret(err, -1, _("Integer value expected refname:lstrip=%s"), arg);
} else if (skip_prefix(arg, "rstrip=", &arg)) {
atom->option = R_RSTRIP;
if (strtol_i(arg, 10, &atom->rstrip))
- die(_("Integer value expected refname:rstrip=%s"), arg);
+ return strbuf_addf_ret(err, -1, _("Integer value expected refname:rstrip=%s"), arg);
} else
- die(_("unrecognized %%(%s) argument: %s"), name, arg);
+ return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), name, arg);
+ return 0;
}
-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -158,9 +163,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
if (!arg) {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
- return;
+ return refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err);
}
atom->u.remote_ref.nobracket = 0;
@@ -183,29 +187,38 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
- refname_atom_parser_internal(&atom->u.remote_ref.refname,
- arg, atom->name);
+ if (refname_atom_parser_internal(&atom->u.remote_ref.refname,
+ arg, atom->name, err)) {
+ string_list_clear(¶ms, 0);
+ return -1;
+ }
}
}
string_list_clear(¶ms, 0);
+ return 0;
}
-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (arg)
- die(_("%%(body) does not take arguments"));
+ return strbuf_addf_ret(err, -1, _("%%(body) does not take arguments"));
atom->u.contents.option = C_BODY_DEP;
+ return 0;
}
-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (arg)
- die(_("%%(subject) does not take arguments"));
+ return strbuf_addf_ret(err, -1, _("%%(subject) does not take arguments"));
atom->u.contents.option = C_SUB;
+ return 0;
}
-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -218,15 +231,20 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.trailer_opts.unfold = 1;
else if (!strcmp(s, "only"))
atom->u.contents.trailer_opts.only_trailers = 1;
- else
- die(_("unknown %%(trailers) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s);
+ string_list_clear(¶ms, 0);
+ return -1;
+ }
}
}
atom->u.contents.option = C_TRAILERS;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.contents.option = C_BARE;
@@ -238,16 +256,19 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at
atom->u.contents.option = C_SUB;
else if (skip_prefix(arg, "trailers", &arg)) {
skip_prefix(arg, ":", &arg);
- trailers_atom_parser(format, atom, *arg ? arg : NULL);
+ if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))
+ return -1;
} else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
- die(_("positive value expected contents:lines=%s"), arg);
+ return strbuf_addf_ret(err, -1, _("positive value expected contents:lines=%s"), arg);
} else
- die(_("unrecognized %%(contents) argument: %s"), arg);
+ return strbuf_addf_ret(err, -1, _("unrecognized %%(contents) argument: %s"), arg);
+ return 0;
}
-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg)
atom->u.objectname.option = O_FULL;
@@ -257,16 +278,18 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
atom->u.objectname.option = O_LENGTH;
if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
atom->u.objectname.length == 0)
- die(_("positive value expected objectname:short=%s"), arg);
+ return strbuf_addf_ret(err, -1, _("positive value expected objectname:short=%s"), arg);
if (atom->u.objectname.length < MINIMUM_ABBREV)
atom->u.objectname.length = MINIMUM_ABBREV;
} else
- die(_("unrecognized %%(objectname) argument: %s"), arg);
+ return strbuf_addf_ret(err, -1, _("unrecognized %%(objectname) argument: %s"), arg);
+ return 0;
}
-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
- refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
+ return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);
}
static align_type parse_align_position(const char *s)
@@ -280,7 +303,8 @@ static align_type parse_align_position(const char *s)
return -1;
}
-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
struct align *align = &atom->u.align;
struct string_list params = STRING_LIST_INIT_DUP;
@@ -288,7 +312,7 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
unsigned int width = ~0U;
if (!arg)
- die(_("expected format: %%(align:<width>,<position>)"));
+ return strbuf_addf_ret(err, -1, _("expected format: %%(align:<width>,<position>)"));
align->position = ALIGN_LEFT;
@@ -299,49 +323,65 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom
if (skip_prefix(s, "position=", &s)) {
position = parse_align_position(s);
- if (position < 0)
- die(_("unrecognized position:%s"), s);
+ if (position < 0) {
+ strbuf_addf(err, _("unrecognized position:%s"), s);
+ string_list_clear(¶ms, 0);
+ return -1;
+ }
align->position = position;
} else if (skip_prefix(s, "width=", &s)) {
- if (strtoul_ui(s, 10, &width))
- die(_("unrecognized width:%s"), s);
+ if (strtoul_ui(s, 10, &width)) {
+ strbuf_addf(err, _("unrecognized width:%s"), s);
+ string_list_clear(¶ms, 0);
+ return -1;
+ }
} else if (!strtoul_ui(s, 10, &width))
;
else if ((position = parse_align_position(s)) >= 0)
align->position = position;
- else
- die(_("unrecognized %%(align) argument: %s"), s);
+ else {
+ strbuf_addf(err, _("unrecognized %%(align) argument: %s"), s);
+ string_list_clear(¶ms, 0);
+ return -1;
+ }
}
- if (width == ~0U)
- die(_("positive width expected with the %%(align) atom"));
+ if (width == ~0U) {
+ string_list_clear(¶ms, 0);
+ return strbuf_addf_ret(err, -1, _("positive width expected with the %%(align) atom"));
+ }
align->width = width;
string_list_clear(¶ms, 0);
+ return 0;
}
-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err)
{
if (!arg) {
atom->u.if_then_else.cmp_status = COMPARE_NONE;
- return;
+ return 0;
} else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
- } else {
- die(_("unrecognized %%(if) argument: %s"), arg);
- }
+ } else
+ return strbuf_addf_ret(err, -1, _("unrecognized %%(if) argument: %s"), arg);
+ return 0;
}
-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *unused_err)
{
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
+ return 0;
}
static struct {
const char *name;
cmp_type cmp_type;
- void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
+ int (*parser)(const struct ref_format *format, struct used_atom *atom,
+ const char *arg, struct strbuf *err);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
@@ -468,8 +508,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
}
memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
- if (valid_atom[i].parser)
- valid_atom[i].parser(format, &used_atom[at], arg);
+ if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
+ return -1;
if (*atom == '*')
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v6 6/6] ref-filter: libify get_ref_atom_value()
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 5/6] ref-filter: add return value to parsers Olga Telezhnaya
@ 2018-03-29 12:49 ` Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
` (2 subsequent siblings)
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 54 ++++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 3f85ef64267d9..3bc65e49358ee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1427,28 +1427,30 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
- int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+ int deref, struct object **obj, struct strbuf *err)
{
int eaten;
+ int ret = 0;
unsigned long size;
void *buf = get_obj(oid, obj, &size, &eaten);
if (!buf)
- die(_("missing object %s for %s"),
- oid_to_hex(oid), ref->refname);
- if (!*obj)
- die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(oid), ref->refname);
-
- grab_values(ref->value, deref, *obj, buf, size);
+ ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ else if (!*obj)
+ ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+ oid_to_hex(oid), ref->refname);
+ else
+ grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+ return ret;
}
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
{
struct object *obj;
int i;
@@ -1570,16 +1572,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
- return;
+ return 0;
- get_object(ref, &ref->objectname, 0, &obj);
+ if (get_object(ref, &ref->objectname, 0, &obj, err))
+ return -1;
/*
* If there is no atom that wants to know about tagged
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1593,20 +1596,23 @@ static void populate_value(struct ref_array_item *ref)
* is not consistent with what deref_tag() does
* which peels the onion to the core.
*/
- get_object(ref, tagged, 1, &obj);
+ return get_object(ref, tagged, 1, &obj, err);
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
{
if (!ref->value) {
- populate_value(ref);
+ if (populate_value(ref, err))
+ return -1;
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return 0;
}
/*
@@ -2130,9 +2136,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+ struct strbuf err = STRBUF_INIT;
- get_ref_atom_value(a, s->atom, &va);
- get_ref_atom_value(b, s->atom, &vb);
+ if (get_ref_atom_value(a, s->atom, &va, &err))
+ die("%s", err.buf);
+ if (get_ref_atom_value(b, s->atom, &vb, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2210,12 +2220,8 @@ int format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, &state);
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
- if (pos < 0) {
- pop_stack_element(&state.stack);
- return -1;
- }
- get_ref_atom_value(info, pos, &atomv);
- if (atomv->handler(atomv, &state, error_buf)) {
+ if (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) ||
+ atomv->handler(atomv, &state, error_buf)) {
pop_stack_element(&state.stack);
return -1;
}
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v6 4/6] ref-filter: change parsing function error handling
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 5/6] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya
@ 2018-03-29 12:49 ` Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
The function returns the position in the used_atom[] array
(as before) for the given atom, or -1 to signal an error.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index a18c86961f08c..93fa6b4e5e63d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -410,7 +410,8 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
- const char *atom, const char *ep)
+ const char *atom, const char *ep,
+ struct strbuf *err)
{
const char *sp;
const char *arg;
@@ -420,7 +421,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
if (*sp == '*' && sp < ep)
sp++; /* deref */
if (ep <= sp)
- die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+ return strbuf_addf_ret(err, -1, _("malformed field name: %.*s"),
+ (int)(ep-atom), atom);
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -446,7 +448,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
}
if (ARRAY_SIZE(valid_atom) <= i)
- die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+ return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
+ (int)(ep-atom), atom);
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -728,17 +731,21 @@ int verify_ref_format(struct ref_format *format)
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ at = parse_ref_filter_atom(format, sp + 2, ep, &err);
+ if (at < 0)
+ die("%s", err.buf);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
format->need_color_reset_at_eol = !!strcmp(color, "reset");
+ strbuf_release(&err);
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2157,13 +2164,17 @@ int format_ref_array_item(struct ref_array_item *info,
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+ int pos;
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+ if (pos < 0) {
+ pop_stack_element(&state.stack);
+ return -1;
+ }
+ get_ref_atom_value(info, pos, &atomv);
if (atomv->handler(atomv, &state, error_buf)) {
pop_stack_element(&state.stack);
return -1;
@@ -2222,7 +2233,12 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ struct strbuf err = STRBUF_INIT;
+ int res = parse_ref_filter_atom(&dummy, atom, end, &err);
+ if (res < 0)
+ die("%s", err.buf);
+ strbuf_release(&err);
+ return res;
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v6 3/6] ref-filter: add return value && strbuf to handlers
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
` (2 preceding siblings ...)
2018-03-29 12:49 ` [PATCH v6 4/6] ref-filter: change parsing function error handling Olga Telezhnaya
@ 2018-03-29 12:49 ` Olga Telezhnaya
2018-03-29 12:49 ` [PATCH v6 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.
Change the signature of handlers by adding return value
and strbuf parameter for errors.
Return value equals 0 upon success and -1 upon failure.
Upon failure, error message is appended to the strbuf.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
ref-filter.c | 51 +++++++++++++++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 9833709dbefe3..a18c86961f08c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -400,7 +400,8 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
+ int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
};
@@ -494,7 +495,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
}
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
/*
* Quote formatting is only done when the stack has a single
@@ -506,6 +508,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
quote_formatting(&state->stack->output, v->s, state->quote_style);
else
strbuf_addstr(&state->stack->output, v->s);
+ return 0;
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -540,7 +543,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)
strbuf_release(&s);
}
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
@@ -548,6 +552,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = &atomv->atom->u.align;
+ return 0;
}
static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -585,7 +590,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
free(if_then_else);
}
-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *unused_err)
{
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
@@ -597,6 +603,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+ return 0;
}
static int is_empty(const char *s)
@@ -609,7 +616,8 @@ static int is_empty(const char *s)
return 1;
}
-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *cur = state->stack;
struct if_then_else *if_then_else = NULL;
@@ -617,11 +625,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
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"));
+ return strbuf_addf_ret(err, -1, _("format: %%(then) atom used without an %%(if) atom"));
if (if_then_else->then_atom_seen)
- die(_("format: %%(then) atom used more than once"));
+ return strbuf_addf_ret(err, -1, _("format: %%(then) atom used more than once"));
if (if_then_else->else_atom_seen)
- die(_("format: %%(then) atom used after %%(else)"));
+ return strbuf_addf_ret(err, -1, _("format: %%(then) atom used after %%(else)"));
if_then_else->then_atom_seen = 1;
/*
* If the 'equals' or 'notequals' attribute is used then
@@ -637,9 +645,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(&cur->output);
+ return 0;
}
-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *prev = state->stack;
struct if_then_else *if_then_else = NULL;
@@ -647,24 +657,26 @@ static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_st
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"));
+ return strbuf_addf_ret(err, -1, _("format: %%(else) atom used without an %%(if) atom"));
if (!if_then_else->then_atom_seen)
- die(_("format: %%(else) atom used without a %%(then) atom"));
+ return strbuf_addf_ret(err, -1, _("format: %%(else) atom used without a %%(then) atom"));
if (if_then_else->else_atom_seen)
- die(_("format: %%(else) atom used more than once"));
+ return strbuf_addf_ret(err, -1, _("format: %%(else) atom used more than once"));
if_then_else->else_atom_seen = 1;
push_stack_element(&state->stack);
state->stack->at_end_data = prev->at_end_data;
state->stack->at_end = prev->at_end;
+ return 0;
}
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
+ struct strbuf *err)
{
struct ref_formatting_stack *current = state->stack;
struct strbuf s = STRBUF_INIT;
if (!current->at_end)
- die(_("format: %%(end) atom used without corresponding atom"));
+ return strbuf_addf_ret(err, -1, _("format: %%(end) atom used without corresponding atom"));
current->at_end(&state->stack);
/* Stack may have been popped within at_end(), hence reset the current pointer */
@@ -681,6 +693,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
}
strbuf_release(&s);
pop_stack_element(&state->stack);
+ return 0;
}
/*
@@ -2151,7 +2164,10 @@ int format_ref_array_item(struct ref_array_item *info,
get_ref_atom_value(info,
parse_ref_filter_atom(format, sp + 2, ep),
&atomv);
- atomv->handler(atomv, &state);
+ if (atomv->handler(atomv, &state, error_buf)) {
+ pop_stack_element(&state.stack);
+ return -1;
+ }
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2160,7 +2176,10 @@ int format_ref_array_item(struct ref_array_item *info,
if (format->need_color_reset_at_eol) {
struct atom_value resetv;
resetv.s = GIT_COLOR_RESET;
- append_atom(&resetv, &state);
+ if (append_atom(&resetv, &state, error_buf)) {
+ pop_stack_element(&state.stack);
+ return -1;
+ }
}
if (state.stack->prev) {
pop_stack_element(&state.stack);
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v6 2/6] ref-filter: start adding strbufs with errors
2018-03-29 12:49 ` [PATCH v6 1/6] ref-filter: add shortcut to work with strbufs Olga Telezhnaya
` (3 preceding siblings ...)
2018-03-29 12:49 ` [PATCH v6 3/6] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
@ 2018-03-29 12:49 ` Olga Telezhnaya
4 siblings, 0 replies; 55+ messages in thread
From: Olga Telezhnaya @ 2018-03-29 12:49 UTC (permalink / raw)
To: git
This is a first step in removing die() calls from ref-filter
formatting logic, so that it could be used by other commands
that do not want to die during formatting process.
die() calls related to bugs in code will not be touched in this patch.
Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.
This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
builtin/branch.c | 7 +++++--
ref-filter.c | 17 ++++++++++++-----
ref-filter.h | 7 ++++---
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
- struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
ref_array_sort(sorting, &array);
for (i = 0; i < array.nr; i++) {
- format_ref_array_item(array.items[i], format, &out);
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ if (format_ref_array_item(array.items[i], format, &out, &err))
+ die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are incompatible");
/* format to a string_list to let print_columns() do its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+ strbuf_release(&err);
strbuf_release(&out);
}
diff --git a/ref-filter.c b/ref-filter.c
index 0c8d1589cf316..9833709dbefe3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2131,9 +2131,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
}
}
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
const struct ref_format *format,
- struct strbuf *final_buf)
+ struct strbuf *final_buf,
+ struct strbuf *error_buf)
{
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2161,19 +2162,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(&resetv, &state);
}
- if (state.stack->prev)
- die(_("format: %%(end) atom missing"));
+ if (state.stack->prev) {
+ pop_stack_element(&state.stack);
+ return strbuf_addf_ret(error_buf, -1, _("format: %%(end) atom missing"));
+ }
strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+ return 0;
}
void show_ref_array_item(struct ref_array_item *info,
const struct ref_format *format)
{
struct strbuf final_buf = STRBUF_INIT;
+ struct strbuf error_buf = STRBUF_INIT;
- format_ref_array_item(info, format, &final_buf);
+ if (format_ref_array_item(info, format, &final_buf, &error_buf))
+ die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ strbuf_release(&error_buf);
strbuf_release(&final_buf);
putchar('\n');
}
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *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 struct ref_format *format,
- struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
/* Print the ref using the given format and quote_style */
void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
/* Parse a single sort specifier and add it to the list */
--
https://github.com/git/git/pull/466
^ permalink raw reply related [flat|nested] 55+ messages in thread