Re: What's cooking in git.git (topics)
- From
Junio C Hamano <junkio@cox.net>
- Date
- Jan 11, 2007, 09:38 UTC
- Message-ID
- <7v1wm16gyd.fsf@assigned-by-dhcp.cox.net>
- In-Reply-To
- <20070111080035.GA28222@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 9 quoted lines
> There is almost no additional penalty for a simple strand of pearls > with the tag placed along that strand; only one possible tag will be > found. But my code does an unnecessary revision list in this case. > > Where we really get hit is the large number of possible tags. The > master branch is turning up 14 tags, some dating back to v1.4.1-rc1. > I do try to abort the revision list as soon as one of those tags > cannot give me a better selection than the one I have currently, > but I still had to generate a revision list to reach that point.
I looked at the algorithm, and after scratching my head for a while, I finally found it sane (modulo leaks, which I think I fixed with the attached patch).
One minor problem that you inherited from the original algorithm is the name priority. If you have an annotated tag A and a lightweight tag b, and ask "git describe --tags" in this graph:
---o---o---o---o---x
A byou would still want to describe 'x' with A, not b. Unfortunately you don't (and the original doesn't either).
I think this is probably easy to solve in the loop that finds "all_matches"; once you hit an annotated tag, your traversal should stop in any case. But if you were asked for --tags, and if your "initialized" piece did find both lightweight and annotated tags, you do not stop when you saw a lightweight one, but keep looking for an annotated one, ignoring any further lightweight ones.
Another thought. I often do "git describe maint master next pu", and I see an opportunity for optimizing for such a multi-ref query. Once you traversed the commits in "all_matches" loop, you know which strands of pearls are reachable to what tags, so you could hang that information somewhere (perhaps ->utils) in each commit. But I think optimizing for a multi-ref query is probably not worth it.
-- >8 -- [PATCH] plug a few leaks in revision walking used in describe.
Signed-off-by: Junio C Hamano <junkio@cox.net> --- builtin-describe.c | 1 + revision.c | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin-describe.c b/builtin-describe.c index d65c7d2..a8c98ce 100644 --- a/builtin-describe.c +++ b/builtin-describe.c @@ -183,6 +183,7 @@ static void describe(const char *arg, int last_one) cur_match->depth++; if (!min_match || cur_match->depth < min_match->depth) min_match = cur_match; + free_commit_list(revs.commits); } printf("%s-g%s\n", min_match->name->path, find_unique_abbrev(cmit->object.sha1, abbrev)); diff --git a/revision.c b/revision.c index 1e3b29a..f2ddd95 100644 --- a/revision.c +++ b/revision.c @@ -1121,21 +1121,23 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch void prepare_revision_walk(struct rev_info *revs) { int nr = revs->pending.nr; - struct object_array_entry *list = revs->pending.objects; + struct object_array_entry *e, *list; + e = list = revs->pending.objects; revs->pending.nr = 0; revs->pending.alloc = 0; revs->pending.objects = NULL; while (--nr >= 0) { - struct commit *commit = handle_commit(revs, list->item, list->name); + struct commit *commit = handle_commit(revs, e->item, e->name); if (commit) { if (!(commit->object.flags & SEEN)) { commit->object.flags |= SEEN; insert_by_date(commit, &revs->commits); } } - list++; + e++; } + free(list); if (revs->no_walk) return;