On Fri, 29 Feb 2008, Junio C Hamano wrote:
> Nicolas Pitre <nico@xxxxxxx> writes:
>
> > ..., but it is still
> > a bit more "correct" to leak it implicitly rather than explicitly.
>
> I do not follow this logic to debate which incorrectness is more
> correct, but I do not mind the removal of free() there.
Freeing the pack structure here without cleaning the resources it still
holds is plain wrong. Not freeing it might be a bit wrong as well, but
it is inline with the rest of Git which relies on program termination to
clean everything.
> I am not sure about the install_packed_git() piece, though.
>
> This part of the code predates Shawn's windowed mmap and all
> other recent code improvements, but the original motivation of
> not installing the pack was to make sure that codepaths outside
> verify_packfile() would not see the objects from the pack being
> verified at all. IOW, the omission originally was intentional.
>
> I just quickly looked at verify_packfile() after applying your
> series, and it seems that nothing tries to access objects with
> only their SHA-1 names without explicitly telling which pack to
> read from, so it should still be safe even if we did not install
> the packed git (iow, the normal codepath would not try to pick
> up objects from the suspect pack that is being validated).
>
> But it made me feel a bit worried.
The problem is with the patch that follows, which calls init_revindex().
Maybe the following patch (on top of the other ones) should bring back
that isolation property for the actual verification pass, and
install_packed_git() called only for the verbose output.
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 4958bbb..cefa4d7 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -40,7 +40,6 @@ static int verify_one_pack(const char *path, int verbose)
if (!pack)
return error("packfile %s not found.", arg);
- install_packed_git(pack);
err = verify_pack(pack, verbose);
return err;
diff --git a/pack-check.c b/pack-check.c
index 0f8ad2c..32176b2 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -105,6 +105,7 @@ static void show_pack_info(struct packed_git *p)
nr_objects = p->num_objects;
memset(chain_histogram, 0, sizeof(chain_histogram));
+ install_packed_git(p);
init_pack_revindex();
for (i = 0; i < nr_objects; i++) {
--
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
