From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Thu, 10 May 2012 15:31:23 GMT
Subject: Re: [PATCH 4/4] notes: only append a blob to a blob
Message-ID: <CACsJy8ADE1PBYsQcJnO6U4OFoWQjaEW7=6=3V_UO=t5KNinDCA@mail.gmail.com>
In-Reply-To: <20120510151904.GB20639@sigill.intra.peff.net>

On Thu, May 10, 2012 at 10:19 PM, Jeff King <peff@peff.net> wrote:
> On Thu, May 10, 2012 at 09:43:35PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>>  Yeah. It made me look again to see if that was true, and I found that
>>  my last patch was flawed. Reading object content in memory in "add
>>  -C" is nonsense, not so much in "append -C".
>
> Yeah. Although you would not want to "append -C" anything but blobs.
> While the tree syntax concatenates, I believe the entries are supposed
> to be in sorted order, no? And you would not want to concatenate commits
> at all.

Exactly. Even for blobs, there are non-safe cases (e.g. binaries) but
that's out of our control (or my attention).

>> +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.

>> @@ -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.
-- 
Duy