patchfsck.c: fix bogus "empty tree" check

ba002f3 (builtin-fsck: move common object checking code to fsck.c) did more than what it claimed to. Most notably, it wrongly made an empty tree object an error by pretending to only move code from fsck_tree() in builtin-fsck.c to fsck_tree() in fsck.c, but in fact adding a bogus check to barf on an empty tree.

An empty tree object is _unusual_. Recent porcelains try reasonably hard not to let the user create a commit that contains such a tree. Perhaps warning about them in git-fsck may have some merit.

However, being unusual and being errorneous are two quite different things. This is especially true now we seem to use the same fsck_$object() code in places other than git-fsck itself. For example, receive-pack should not reject unusual objects, even if it would be a good idea to tighten it to reject incorrect ones.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I've wasted a few hours tonight hunting for random breakages in "git
   push", the symptom of which is "fatal: unresolved deltas left after
   unpacking."  I was hoping this patch would fix it, but it seems that
   the problem is elsewhere.

   I'll revert the following two commits for now:

   d5ef408 (unpack-objects: prevent writing of inconsistent objects)
   28f72a0 (receive-pack: use strict mode for unpacking objects)

   as I have verified that running with receive.fsckobjects set to false
   fixes the issues for me, and the repository at the receiving end (both
   before and after the push) pass git-fsck without problems.  Needless to
   say, I am not a happy camper right now.

 fsck.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

On Tue, 04 Mar 2008 03:21:16 -0800 Junio C Hamano wrote:

This part of commit d5ef408 changes is bogus:

The write_sha1_file() call here was calculating obj_list[nr].sha1; now it is removed, but added_object() needs this value:

| static void added_object(unsigned nr, enum object_type type,
| 			 void *data, unsigned long size)
| {
| 	struct delta_info **p = &delta_list;
| 	struct delta_info *info;
|
| 	while ((info = *p) != NULL) {
| 		if (!hashcmp(info->base_sha1, obj_list[nr].sha1) ||
					      ^^^^^^^^^^^^^^^^^
| 		    info->base_offset == obj_list[nr].offset) {
| 			*p = info->next;
| 			p = &delta_list;
| 			resolve_delta(info->nr, type, data, size,
| 				      info->delta, info->size);
| 			free(info);
| 			continue;
| 		}
| 		p = &info->next;
| 	}
| }

However, I do not have time to create a proper test case for this.

The simplest way to fix this would be to duplicate the added_object() call in all branches; invoking hash_sha1_file() unconditionally will work too, but may be wasteful if we need to call write_sha1_file() afterwards.

Sergey Vlasov <vsu@altlinux.ru> writes:

Thanks, somehow I missed that when merging it up for 'next'.

That's Ok. What we need is a fix but it is not that urgent as the stuff is now reverted for now.

Sorry for being a sloppy maintainer. I have to admit that I did not read every single line of patches in a few topics merged to 'master' recently, due to workload pressure, and some extra eyeballs after-the-fact are greatly appreciated.

This adds a test for unpacking deltified objects with --strict option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Junio C Hamano <gitster@pobox.com> writes:

 > Sergey Vlasov <vsu@altlinux.ru> writes:
 > ...
 >> However, I do not have time to create a proper test case for this.
 >
 > That's Ok.  What we need is a fix but it is not that urgent as the stuff
 > is now reverted for now.

 t/t5300-pack-object.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

This adds test for unpacking deltified objects with --strict option.

 - unpacking full trees with --strict should pass;

 - unpacking only trees with --strict should be rejected due to
   missing blobs;

 - unpacking only trees with --strict into an existing
   repository with necessary blobs should succeed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The pack created by the test of the original one contained
   only trees and unpacked into an empty repository, and --strict
   has every right to complain.  It was not a good test.

 t/t5300-pack-object.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

On Tue, Mar 04, 2008 at 03:26:35PM +0300, Sergey Vlasov wrote:

This is only a part of the problem. Moving added_object only makes forward reference for deltas work.

unpack_delta_entry checks for OBJ_REF_DELTA, if there is a sha1 file. This must not be true, if --strict is passed. It needs to check the cache too.

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Tue, 4 Mar 2008 22:38:21 +0100
Subject: [PATCH] unpack-objects: fix delta handling

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 builtin-unpack-objects.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)