From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 18 Sep 2011 19:17:41 GMT
Subject: Re: [PATCH 1/3] fast-import: die if we produce too many (MAX_PACK_ID) packs
Message-ID: <20110918191741.GD2308@elie>
In-Reply-To: <1316372508-7173-2-git-send-email-divanorama@gmail.com>

Dmitry Ivankov wrote:

> In fast-import pack_id is 16-bit with MAX_PACK_ID reserved to identify
> pre-existing objects. It is unlikely to wrap under reasonable settings
> but still things in fast-import will break once it happens.
>
> Add a check and immediate die() as the simplest reaction to being unable
> to continue the import.

Makes a lot of sense.  A few possible minor clarity improvements:

 - missing commas after "In fast-import" and before "with MAX_PACK_ID
   reserved"
 - "pre-existing objects": it would be clearer to say something like
   "objects this fast-import process instance did not write out to a
   packfile", like the comment before gfi_unpack_entry() does
 - I suppose "under reasonable settings" means "with a reasonable
   max-pack-size setting"?
 - "things will break" is a bit vague.
 - "immediate" -> "immediately"

Maybe:

	In fast-import, pack_id is a 16-bit unsigned integer, with MAX_PACK_ID
	(2^16 - 1) reserved for use by objects that are not in a packfile that
	this fast-import process instance wrote.  It is unusual for pack_id to
	hit MAX_PACK_ID with a reasonable --max-pack-size setting, but when it
	does, the pack_id stored in each "struct object_entry" wraps and
	fast-import gets utterly confused.

	Add a check and immediately die() so the operator can at least see what
	went wrong instead of experiencing an unexplained broken import.

With or without that clarification,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks!  A test would still be nice, if someone has time to write one.