Re: [PATCH 4/4] notes: only append a blob to a blob
- From
Jeff King <peff@peff.net>
- Date
- May 10, 2012, 15:45 UTC
- Message-ID
- <20120510154519.GB23941@sigill.intra.peff.net>
- In-Reply-To
- <CACsJy8ADE1PBYsQcJnO6U4OFoWQjaEW7=6=3V_UO=t5KNinDCA@mail.gmail.com>
On Thu, May 10, 2012 at 10:31:23PM +0700, Nguyen Thai Ngoc Duy wrote:
Show 18 quoted lines
> >> +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.
Show 17 quoted lines
> >> @@ -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