mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Mike Hommey <>
To: Junio C Hamano <>
Cc:, Shawn Pearce <>,
	Jonathan Nieder <>, Jeff King <>
Subject: Re: [PATCH] fast-import: Do less work when given "from" matches current branch head
Date: Fri, 10 Jul 2015 07:30:18 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Jul 09, 2015 at 01:37:02PM -0700, Junio C Hamano wrote:
> Mike Hommey <> 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?

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.


  reply	other threads:[~2015-07-09 22:30 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-06 22:07 suboptimal behavior of fast-import in some cases with "from" Mike Hommey
2015-07-06 22:54 ` Junio C Hamano
2015-07-09  5:03   ` Mike Hommey
2015-07-09  5:52     ` Mike Hommey
2015-07-09  6:50       ` [PATCH] fast-import: Do less work when given "from" matches current branch head Mike Hommey
2015-07-09 20:37         ` Junio C Hamano
2015-07-09 22:30           ` Mike Hommey [this message]
2015-07-09 22:44             ` Junio C Hamano
2015-07-09 22:47               ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).