git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Martin Ågren <martin.agren@gmail.com>
To: Olga Telezhnaya <olyatelezhnaya@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
Date: Thu, 15 Mar 2018 21:47:09 +0100
Message-ID: <20180315204709.1900787-1-martin.agren@gmail.com> (raw)
In-Reply-To: <0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000@eu-west-1.amazonses.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. :-)

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


  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 10:16 [RFC 1/4] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-13 10:16 ` [RFC 2/4] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-13 19:13   ` Martin Ågren
2018-03-13 10:16 ` [RFC 3/4] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-13 19:18   ` Martin Ågren
2018-03-14 13:36     ` Оля Тележная
2018-03-13 10:16 ` [RFC 4/4] ref-filter: add return value to parsers Olga Telezhnaya
2018-03-13 19:12 ` [RFC 1/4] ref-filter: start adding strbufs with errors Martin Ågren
2018-03-14 13:30   ` Оля Тележная
2018-03-14 19:04 ` [PATCH v2 1/5] " Olga Telezhnaya
2018-03-14 19:04   ` [PATCH v2 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-15 22:48     ` Junio C Hamano
2018-03-16  7:22       ` Оля Тележная
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   ` [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
2018-03-15 20:47     ` Martin Ågren [this message]
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       ` Оля Тележная
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 3/5] ref-filter: change parsing function error handling Olga Telezhnaya
2018-03-19 19:41       ` Junio C Hamano
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
2018-03-19 13:01     ` [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling Olga Telezhnaya
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 2/5] ref-filter: add return value && strbuf to handlers Olga Telezhnaya
2018-03-20 18:18         ` Eric Sunshine
2018-03-20 16:05       ` [PATCH v4 4/5] ref-filter: add return value to parsers 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 5/5] ref-filter: get_ref_atom_value() " Olga Telezhnaya
2018-03-20 18:19         ` Eric Sunshine
2018-03-20 22:30           ` Junio C Hamano
2018-03-20 22:50             ` Eric Sunshine
2018-03-21 19:30               ` Junio C Hamano
2018-03-21 19:58                 ` Eric Sunshine
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 3/6] ref-filter: add return value && strbuf to handlers 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         ` [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             ` Оля Тележная
2018-03-21 18:28         ` [PATCH v5 2/6] ref-filter: start adding strbufs with errors Olga Telezhnaya
2018-03-21 18:28         ` [PATCH v5 5/6] ref-filter: add return value to parsers Olga Telezhnaya
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           ` Оля Тележная
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 4/6] ref-filter: change parsing function error handling 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 2/6] ref-filter: start adding strbufs with errors 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 6/6] ref-filter: libify get_ref_atom_value() Olga Telezhnaya

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180315204709.1900787-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=olyatelezhnaya@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox