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

From: Jeff King <peff@peff.net>
Date: May 10, 2012, 15:19 UTC
Message-ID: <20120510151904.GB20639@sigill.intra.peff.net>
In-Reply-To: <1336661015-14149-1-git-send-email-pclouds@gmail.com>

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.

Show 11 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?

Show 11 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.

-Peff