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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
