Open Bug 1122188 Opened 6 years ago Updated 4 years ago

Template keywords for identifying commits that are Firefox tree heads

Categories

(Developer Services :: Mercurial: firefoxtree, defect)

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: gps, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

firefoxtree needs template keywords to display firefox tree heads and nearest firefox tree head.
This patch implements {fxheads} to return a list of string Firefox trees
whose heads were last seen on a changeset.

The caching logic was split out to facilitate more templates in the
future.
Attachment #8550123 - Flags: review?(ted)
Assignee: nobody → gps
Status: NEW → ASSIGNED
I want to change this a bit before it lands. So only asking for
feedback.
Attachment #8550131 - Flags: feedback?(ted)
Comment on attachment 8550123 [details] [diff] [review]
firefoxtree: implement {fxheads} template keyword ()

Review of attachment 8550123 [details] [diff] [review]:
-----------------------------------------------------------------

::: hgext/firefoxtree/__init__.py
@@ +396,5 @@
> +
> +    res = set()
> +    for tag, node, tree, uri in labels:
> +        if node == ctx.node():
> +            res.add(tag)

Couldn't you do this all in the set constructor with a generator expression?
res = set(t for t,n,tr,u in labels if n == ctx.node())
Attachment #8550123 - Flags: review?(ted) → review+
Comment on attachment 8550131 [details] [diff] [review]
firefoxtree: implement {nearestfxhead} for finding what tree a changeset is on ()

Review of attachment 8550131 [details] [diff] [review]:
-----------------------------------------------------------------

I think this could be okay with a small change.

::: hgext/firefoxtree/__init__.py
@@ +406,5 @@
> +def template_nearestfxhead(repo, ctx, templ, cache, revcache, **args):
> +    """:nearestfxhead: String. Nearest Firefox tree in the ancestry of this
> +    changeset.
> +
> +    This template only works on unpublished changesets due to performance

This seems kind of odd and not the behavior I'd expect. Since the original request here was for something that'd work with the prompt extension, I would expect this to return a consistent result regardless.

Otherwise, if you had your prompt set to "{nearestfxhead}", you could wind up with behavior like:

luser central:/build/mozilla-central$ hg pull inbound
<...>
luser central:/build/mozilla-central$ hg up inbound
luser :/build/mozilla-central$ # wtf

or
luser inbound:/build/mozilla-central$ hg push inbound
<...>
luser :/build/mozilla-central$ # wtf

After thinking about it I understand why you're doing this, but I think a minor tweak to your restriction here could fix this issue and still not make it do unbounded DAG walks--simply say that it only works on unpublished changesets *and* the known Firefox tree heads. Then you can just quickly check if ctx.rev() is one of the known labels and fall through to your existing logic.

@@ +416,5 @@
> +
> +    if ctx.phase() == phases.public:
> +        return None
> +
> +    for rev in repo.changelog.ancestors([ctx.rev()]):

I was toying with reproducing this in `hg log` using the following command:
hg log -r "first(reverse(fxheads() & ancestors(.)))"

Is it possible to write the equivalent of that revset expression here to make it performant?

@@ +418,5 @@
> +        return None
> +
> +    for rev in repo.changelog.ancestors([ctx.rev()]):
> +        actx = repo[rev]
> +        for tag, node, tree, uri in labels:

I would probably just make a dict of node->label up above and use that here.
Attachment #8550131 - Flags: feedback?(ted) → feedback-
Comment on attachment 8550131 [details] [diff] [review]
firefoxtree: implement {nearestfxhead} for finding what tree a changeset is on ()

Review of attachment 8550131 [details] [diff] [review]:
-----------------------------------------------------------------

::: hgext/firefoxtree/__init__.py
@@ +418,5 @@
> +        return None
> +
> +    for rev in repo.changelog.ancestors([ctx.rev()]):
> +        actx = repo[rev]
> +        for tag, node, tree, uri in labels:

Furthering that point, maybe it makes sense to just have _getcachedlabels return that dict?
Your revset suggestion for the template implementation is a pretty good one.

Thinking about the "nearest tree" problem, I think there are 2 use cases with the difference being the direction of the DAG walk:

1. Find the nearest tree this changeset is based on (walking towards roots) (useful for a shell prompt, local development, etc)
2. Find the trees a changeset is part of (walking towards heads) (useful for archeology)

I think we should implement 2 templates for these different use cases. But I'm not sure what to call them. Naming is hard.
Here is a slightly better implementation of the walk-towards-roots
template.
Attachment #8550131 - Attachment is obsolete: true
I think your {nearestfxhead} naming is fine for use case #1, keeping {fxheads} for #2.
Well, there is a 3rd use case: show me the labels for Firefox tree heads that are on this specific commit. That's what {fxheads} currently does.
I'm not actively working on this.
Assignee: gps → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.