"Shawn O. Pearce" <spearce@xxxxxxxxxxx> writes:
> 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 b
you 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@xxxxxxx>
---
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;
-
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