fast-import: clean up pack_data pointer in end_packfile

We have a global pointer pack_data pointing to the current
pack we have open. Inside end_packfile we have two new
pointers, old_p and new_p. The latter points to pack_data,
and the former points to the new "installed" version of the
packfile we get when we hand the file off to the regular
sha1_file machinery. When then free old_p.

Presumably the extra old_p pointer was there so that we
could overwrite pack_data with new_p and still free old_p,
but we don't do that. We just leave pack_data pointing to
bogus memory, and don't overwrite it until we call
start_packfile again (if ever).

This can cause problems for our die routine, which calls
end_packfile to clean things up. If we die at the wrong
moment, we can end up looking at invalid memory in
pack_data left after the last end_packfile().

Instead, let's make sure we set pack_data to NULL after we
free it, and make calling endfile() again with a NULL
pack_data a noop (there is nothing to end).

We can further make things less confusing by dropping old_p
entirely, and moving new_p closer to its point of use.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fast-import.c b/fast-import.c
index fb4738d373a5a1048c2ff388a729e6088515ca9b..92b3186326bfcfc8338c3c5cc56fb44774a63265 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -945,10 +945,12 @@ static void unkeep_all_packs(void)
 
 static void end_packfile(void)
 {
 
 static void end_packfile(void)
 {

-       struct packed_git *old_p = pack_data, *new_p;
+       if (!pack_data)
+               return;

 
        clear_delta_base_cache();
        if (object_count) {
 
        clear_delta_base_cache();
        if (object_count) {

+               struct packed_git *new_p;

                unsigned char cur_pack_sha1[20];
                char *idx_name;
                int i;
                unsigned char cur_pack_sha1[20];
                char *idx_name;
                int i;


@@ -990,10 +992,11 @@ static void end_packfile(void)
                pack_id++;
        }
        else {
                pack_id++;
        }
        else {

-               close(old_p->pack_fd);
-               unlink_or_warn(old_p->pack_name);
+               close(pack_data->pack_fd);
+               unlink_or_warn(pack_data->pack_name);

        }
        }

-       free(old_p);
+       free(pack_data);
+       pack_data = NULL;

 
        /* We can't carry a delta across packfiles. */
        strbuf_release(&last_blob.data);
 
        /* We can't carry a delta across packfiles. */
        strbuf_release(&last_blob.data);