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

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

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

quoted

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 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)

quoted

+                         (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