2018-03-15 23:47 GMT+03:00 Martin Ågren <martin.agren@xxxxxxxxx>:
> 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@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.
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
>
