Mike Hommey <mh@xxxxxxxxxxxx> writes:
Cc'ed a few people who appear at the top of "shortlog --no-merges";
I think the end result is not incorrect, but I want to hear second
opinions on this one. I do not know Shawn still remembers this
code, but what is under discussion seems to have come mostly from
ea5e370a (fast-import: Support reusing 'from' and brown paper bag
fix reset., 2007-02-12).
> if (!skip_prefix(command_buf.buf, "from ", &from))
> return 0;
>
> - if (b->branch_tree.tree) {
> - release_tree_content_recursive(b->branch_tree.tree);
> - b->branch_tree.tree = NULL;
> - }
> + hashcpy(sha1, b->branch_tree.versions[1].sha1);
>
> s = lookup_branch(from);
> if (b == s)
The part that deals with a branch that is different from the current
one is not visible in the context (i.e. when s = lookup_branch(from)
returned a non-NULL result that is different from b) but it used to,
and continues to with this patch, copy sha1 from branch_tree.sha1
and branch_tree.versions[] from sha1 and branch_tree.versions[1] of
the specified branch.
That codepath used to release the contents of branch_tree.tree when
it did so, but it no longer does so after this patch because of the
removal we see above.
Does that mean the original code was doing a release that was
unnecessary? Or does it mean this patch changes what happens on
that codepath, namely (1) leaking resource, and/or (2) keeping a
tree of the original 'b' that does not have anything to do with the
tree of 's', preventing the later lazy-load code from reading the
tree of 's' and instead of building on top of a wrong tree content?
... me goes and reads on ...
> @@ -2610,14 +2608,16 @@ static int parse_from(struct branch *b)
> struct object_entry *oe = find_mark(idnum);
> if (oe->type != OBJ_COMMIT)
> die("Mark :%" PRIuMAX " not a commit", idnum);
> - hashcpy(b->sha1, oe->idx.sha1);
> - if (oe->pack_id != MAX_PACK_ID) {
> - unsigned long size;
> - char *buf = gfi_unpack_entry(oe, &size);
> - parse_from_commit(b, buf, size);
> - free(buf);
> - } else
> - parse_from_existing(b);
> + if (hashcmp(b->sha1, oe->idx.sha1)) {
> + hashcpy(b->sha1, oe->idx.sha1);
> + if (oe->pack_id != MAX_PACK_ID) {
> + unsigned long size;
> + char *buf = gfi_unpack_entry(oe, &size);
> + parse_from_commit(b, buf, size);
> + free(buf);
> + } else
> + parse_from_existing(b);
> + }
> } else if (!get_sha1(from, b->sha1)) {
> parse_from_existing(b);
> if (is_null_sha1(b->sha1))
This part is straight-forward.
> @@ -2626,6 +2626,11 @@ static int parse_from(struct branch *b)
> else
> die("Invalid ref name or SHA1 expression: %s", from);
>
> + if (b->branch_tree.tree && hashcmp(sha1, b->branch_tree.versions[1].sha1)) {
> + release_tree_content_recursive(b->branch_tree.tree);
> + b->branch_tree.tree = NULL;
> + }
> +
This looks like an attempt to compensate for that "what happens if
(s != NULL && s != b)?" issue, and also for the surviving codepaths.
As both parse_from_commit() and parse_from_existing() only touch
branch_tree.versions[] and they do not seem to get affected if
b->branch_tree.tree holds a stale and unrelated content, this looks
OK to me from a cursory reading, but it does make me feel dirty that
it has to put *b temporarily into an inconsistent state.
--
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