Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from

Nguyễn Thái Ngọc Duy [off-list ref] writes:

We currently know if an object is loose or packed. We do not know if
it's from the repo's object database, or via alternates
mechanism. With this patch, sha1_object_info_extended() can tell if an
object comes from alternates source (and which one).

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 How about this way instead: we keep track of where objects come from
 so we can verify object source when we create or update something
 that contains SHA-1.

The overall approach taken by this series may be worth considering, but
I do not think the check implemented here is correct.

An object may be found in an alternate odb but we may also have our
own copy of the same object.  You need to prove that a suspicious
object is visible to us *ONLY* through add_submodule_odb().

Once you do add_submodule_odb() to contaminate our object pool, you
make everything a suspicious object that needs to be checked; that
is the worst part of the story.

If I understand the test case that triggers the issue correctly, it
makes a merge in the top-level superproject, and while doing so,
tries to three-way merge gitlink changes, so that it can detect
fast-forward situation inside the submodule.  It should just be done
with running "git merge-base A B" there; after all, we are in the
tree-wide merge of the top-level superproject, which is already a
heavy-weight operation. I do not see a reason to make it more brittle
than necessary.

If we rip out add_submodule_odb() we do not have to ever worry about
this kind of object database breakages that are hard to track down.
It is an optimization to allow the code become more brittle, violate
the project boundary, and break the object database faster as a
result.  That is not a good optimization at all.

On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano [off-list ref] wrote:

quoted

 How about this way instead: we keep track of where objects come from
 so we can verify object source when we create or update something
 that contains SHA-1.

The overall approach taken by this series may be worth considering, but
I do not think the check implemented here is correct.

An object may be found in an alternate odb but we may also have our
own copy of the same object.  You need to prove that a suspicious
object is visible to us *ONLY* through add_submodule_odb().

The way alt odbs are linked (new odbs area always at the end), if we
have the same copy, their copy will never be read (we check out alt
odbs from head to tail). So those duplicate suspicious objects are
actually invisible to us.

Once you do add_submodule_odb() to contaminate our object pool, you
make everything a suspicious object that needs to be checked; that
is the worst part of the story.

And because we never really touch their alt copy, the returned alt
source is "ours", trusted and not checked. The check should only occur
on objects that we do not have.
-- 
Duy