suboptimal behavior of fast-import in some cases with "from"

> Mike Hommey <mh@glandium.org> writes:
> 
> > One of the first things parse_from does is unconditionally throw away
> > the tree for the given branch, and then the "from" tree is loaded. So
> > when the "from" commit is the current head of the branch, that make
> > fast-import do more work than necessary.
> 
> If it is very common that the next commit the input stream wants to
> create is often on top of the commit that was just created, and if
> it is very common that the input stream producer knows what the
> commit object name of the commit that was just created, then
> optimising for that case does not sound too bad.  It really depends
> on two factors:
> 
>  - How likely is it that other people make the same mistake?

> On Mon, Jul 06, 2015 at 03:54:35PM -0700, Junio C Hamano wrote:
> > Mike Hommey <mh@glandium.org> writes:
> > 
> > > One of the first things parse_from does is unconditionally throw away
> > > the tree for the given branch, and then the "from" tree is loaded. So
> > > when the "from" commit is the current head of the branch, that make
> > > fast-import do more work than necessary.
> > 
> > If it is very common that the next commit the input stream wants to
> > create is often on top of the commit that was just created, and if
> > it is very common that the input stream producer knows what the
> > commit object name of the commit that was just created, then
> > optimising for that case does not sound too bad.  It really depends
> > on two factors:
> > 
> >  - How likely is it that other people make the same mistake?
> 
> Looks like the answer is: very. Assuming my quick glance at the code is
> not mistaken, the same mistake is made in at least git-p4.py (yes, the
> one that comes with git), felipec's git-remote-hg, and hg-fast-export,
> and that's 100% of the sample I looked at.
> 
> I won't claim to know what fast-import is doing, not having looked at
> more than the parse_from* functions and the commit message for 4cabf858,
> but it seems plausible this also skips making tree deltas for those
> trees.

> >  - How bad the damage to parse_from() would be if we wanted to
> >    optimize for this case?
> 
> I /think/ it would look like this (untested), which doesn't seem too
> damaging:

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

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

> @@ -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;
> +	}
> +

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

>> 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?
>
> I guess the question is whether branch_tree.tree can be in a state that
> doesn't match that of branch_tree.versions[1].sha1. If not, then if s
> and b have the same branch_tree.versions[1].sha1 for some reason, then
> keeping the old branch_tree.tree makes no practical difference from
> resetting it. Except it skips the busy-work.

> Mike Hommey <mh@glandium.org> writes:
>
>>> 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?
>>
>> I guess the question is whether branch_tree.tree can be in a state that
>> doesn't match that of branch_tree.versions[1].sha1. If not, then if s
>> and b have the same branch_tree.versions[1].sha1 for some reason, then
>> keeping the old branch_tree.tree makes no practical difference from
>> resetting it. Except it skips the busy-work.
>
> Perhaps my comment was misleading.  I _think_ the state at the end
> of this function (i.e. the latter hunk you added to the function
> makes the above issue I raised go away).  It just made me feel