On Fri, Jan 25, 2013 at 2:15 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
>> ---
>> read-cache.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index fda78bc..4579215 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce,
>> ce->name + common, ce_namelen(ce) - common);
>> }
>>
>> + if (object_database_contaminated) {
>> + struct object_info oi;
>> + switch (ce->ce_mode) {
>> + case S_IFGITLINK:
>> + break;
>> + case S_IFDIR:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_TREE ||
>
> This case should never happen. Do we store any tree object in the
> index in the first place?
No it was copy/paste mistake (from cache-tree.c, before I deleted it
and added check_sha1_file_for_external_source in 3/7)
>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an external tree %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + default:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != OBJ_BLOB ||
>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an external blob %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + }
>> + }
>> +
>> result = ce_write(c, fd, ondisk, size);
>> free(ondisk);
>> return result;
>
> I think the check you want to add is to the cache-tree code; before
> writing the new index out, if you suspect you might have called
> cache_tree_update() before, invalidate any hierarchy that is
> recorded to produce a tree object that refers to an object that is
> only available through external object store.
cache-tree is covered by check_sha1_file_for_external_source() when it
actually writes a tree (dry-run mode goes through unchecked). Even
when cache-tree is not involved, I do not want the index to point to
an non-existing SHA-1 ("git diff --cached" may fail next time, for
example).
> As to the logic to check if a object is something that we should
> refuse to create a new dependent, I think you should not butcher
> sha1_object_info_extended(), and instead add a new call that checks
> if a given SHA-1 yields an object if you ignore alternate object
> databases that are marked as "external", whose signature may
> resemble:
>
> int has_sha1_file_proper(const unsigned char *sha1);
>
> or something.
Agreed.
> This is a tangent, but in addition, you may want to add an even
> narrower variant that checks the same but ignoring _all_ alternate
> object databases, "external" or not:
>
> int has_sha1_file_local(const unsigned char *sha1);
>
> That may be useful if we want to later make the alternate safer to
> use; instead of letting the codepath to create an object ask
> has_sha1_file() to see if it already exists and refrain from writing
> a new copy, we switch to ask has_sha1_file_locally() and even if an
> alternate object database we borrow from has it, we keep our own
> copy in our repository.
>
> Not tested beyond syntax, but rebasing something like this patch on
> top of your "mark external sources" change may take us closer to
> that.
Thanks, will incorporate in the reroll.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html