Re: Be more careful about updating refs

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2008, 02:01 UTC
Message-ID: <7v63xrki29.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vbq7jkixg.fsf@gitster.siamese.dyndns.org>

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

Show 5 quoted lines

> The following patch seems to fix the issue for me, but this is
> primarily meant for discussion, as I do not quite understand why
> the same issue does not manifest itself when NO_MMAP is not
> used.
> ...

BTW, the lookup of the object that dies is in update_branch(). The call to write_ref_sha1() at the last tries to verify b->sha1 is available and is a commit.

        static int update_branch(struct branch *b)
        {
                static const char *msg = "fast-import";
                struct ref_lock *lock;
                unsigned char old_sha1[20];

                if (read_ref(b->name, old_sha1))
                        hashclr(old_sha1);
                lock = lock_any_ref_for_update(b->name, old_sha1, 0);
                if (!lock)
                        return error("Unable to lock %s", b->name);
                if (!force_update && !is_null_sha1(old_sha1)) {
                        struct commit *old_cmit, *new_cmit;

                        old_cmit = lookup_commit_reference_gently(old_sha1, 0);
                        new_cmit = lookup_commit_reference_gently(b->sha1, 0);
                        if (!old_cmit || !new_cmit) {
                                unlock_ref(lock);
                                return error("Branch %s is missing commits.", b->name);
                        }

                        if (!in_merge_bases(old_cmit, &new_cmit, 1)) {
                                unlock_ref(lock);
                                warning("Not updating %s"
                                        " (new tip %s does not contain %s)",
                                        b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
                                return -1;
                        }
                }
                if (write_ref_sha1(lock, b->sha1, msg) < 0)
                        return error("Unable to update %s", b->name);
                return 0;
        }

The write_ref_sha1() function previously did not even check if the object pointed at by b->sha1 actually existed, but now it parses the object and makes sure it is a commit, if the updated ref is under ref/heads/ hierarchy.

What I do not quite understand is how this can be a new issue.

The codepath to allow updating an existing branch shown above (i.e. "if it is not force and old is not NULL") uses the usual lookup_commit_reference_gently() interface to access b->sha1, and does not use gfi-aware gfi_unpack_entry() or anything magical, which means it would have passed the same codepath down to trigger the same issue. IOW, even before this tightening of write_ref_sha1(), we already should have had the issue of not being to able to grab the object b->sha1 refers to out of the newly built packfile.

Is it just nobody seriously exercised the codepath yet, or is there a difference between these two calls that is more subtle than that?

Quite confused...