Re: [PATCH 4/4] notes: only append a blob to a blob

[Jeff King <peff@peff.net>]

On Thu, May 10, 2012 at 10:31:23PM +0700, Nguyen Thai Ngoc Duy wrote:

> >> +static const char *type_name(enum object_type type)
> >> +{
> >> +     switch (type) {
> >> +     case OBJ_BLOB: return _("a blob");
> >> +     case OBJ_TAG: return _("a tag");
> >> +     case OBJ_COMMIT: return _("a commit");
> >> +     case OBJ_TREE: return _("a tree");
> >> +     default:
> >> +             die("BUG: put a new string for type %d here", type);
> >> +     }
> >> +}
> >
> > Don't we have object.c:typename for this
> 
> The key difference here is _() with an article. It's i18n friendly. I
> wanted to make 15 combinations (blob/blob cannot happen) of "cannot
> append %s to %s", which is best for translators but probably too much
> for C developers.

I do not pay much attention to the translation details, but I would
think that we would keep terms like "tree" and "blob" universal, as they
are technical terms. IOW, you would not expect the "blob" in "git
cat-file blob $sha1" to be internationalized, and this seems like the
same level of technical detail.

> >> @@ -204,8 +216,12 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,
> >>               strbuf_grow(&(msg->buf), size + 1);
> >>               if (msg->buf.len && prev_buf && size)
> >>                       strbuf_insert(&(msg->buf), 0, "\n", 1);
> >> -             if (prev_buf && size)
> >> +             if (prev_buf && size) {
> >> +                     if (type != OBJ_BLOB || msg->type != OBJ_BLOB)
> >> +                             die(_("cannot append %s to %s"),
> >> +                                 type_name(type), type_name(msg->type));
> >>                       strbuf_insert(&(msg->buf), 0, prev_buf, size);
> >> +             }
> >
> > I think this is wrong for the reasons above. You would not want to
> > concatenate a tree to a tree.
> 
> Hmm.. that would become "if (tree != blob || tree != blob) die();",
> exactly your point.

Oh, I totally misread this as "type != msg->type" for some reason.
Sorry.

I think the behavior is correct, but the message confused me. If I see
"cannot append a foo to a bar", it implies to me that it is the
combination of the elements that is the limiting factor. But it is not.
It is that one (or more) of the elements is not a blob, regardless of
what the other element is. So I think this would be more accurate:

  if (type != OBJ_BLOB)
          die(_("cannot append to a non-blob note"));
  if (msg->type != OBJ_BLOB)
          die(_("cannot append a non-blob object to a note"));

-Peff