Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects

Duy Nguyen [off-list ref] writes:

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

I think we have tests that explicitly add SHA-1 that names an object
that does not exist to the index and check failures; you may have to
think what to do with them.

quoted

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.

This is not a tangent, but if you want to go this "forbid making our
repository depend on objects we do not have but we know about after
we peek submodule odb" route [*1*], write_sha1_file() needs to be
told about has_sha1_file_proper().  We may "git add" a new blob in
the superproject, the blob may not yet exist in *our* repository,
but may happen to already exist in the submodue odb.  In such a
case, write_sha1_file() has to write that blob in our repository,
without the existing has_sha1_file() check bypassing it.  Otherwise
our attempt to create a tree that contains that blob will fail,
saying that the blob only seems to exist to us via submodule odb but
not in our repository.


[Footnote]

*1* which I do not necessarily agree with---I am in favor of getting
rid of add_submodule_odb() to fix these issues at the root cause of
them.

On Sat, Jan 26, 2013 at 2:00 AM, Junio C Hamano [off-list ref] wrote:

This is not a tangent, but if you want to go this "forbid making our
repository depend on objects we do not have but we know about after
we peek submodule odb" route [*1*], write_sha1_file() needs to be
told about has_sha1_file_proper().  We may "git add" a new blob in
the superproject, the blob may not yet exist in *our* repository,
but may happen to already exist in the submodue odb.  In such a
case, write_sha1_file() has to write that blob in our repository,
without the existing has_sha1_file() check bypassing it.  Otherwise
our attempt to create a tree that contains that blob will fail,
saying that the blob only seems to exist to us via submodule odb but
not in our repository.

Another thing needs to be done for this to work. The current reading
order is packs first, loose objects next. If we create a local loose
duplicate of an alternate packed object, our local version will never
be read. Regardless the submodule odb issue, I think we should prefer
reading local loose objects over alternate packed ones.
-- 
Duy

On Mon, Jan 28, 2013 at 12:48 PM, Duy Nguyen [off-list ref] wrote:

Regardless the submodule odb issue, I think we should prefer
reading local loose objects over alternate packed ones.

I think I went from one problem to another and did not make it clear.
The reason behind this preference is security. With "all packs first"
reading order, someone can create a pack in the alternate source and
that pack will override the same local loose objects (local packed
ones are safe). If someone can create a malicious version with the
same SHA-1, we (well, the user) are in trouble. If the user uses this
repository directly then the malicious object can be used without
detected, even if it does not match the original SHA-1, as we do not
always verify content against its SHA-1 for common commands.
-- 
Duy