Re: [PATCH] fast-import: Do less work when given "from" matches current branch head

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2015, 20:37 UTC
Message-ID: <xmqqk2u9uky9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1436424609-26159-1-git-send-email-mh@glandium.org>

Mike Hommey <mh@glandium.org> 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).

Show 11 quoted lines

>  	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 ...

Show 25 quoted lines

> @@ -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.

Show 9 quoted lines

> @@ -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.