1122188 - Template keywords for identifying commits that are Firefox tree heads

Status: NEW

(Developer Services :: Mercurial: firefoxtree, defect)

Description

[Gregory Szorc [:gps]]

firefoxtree needs template keywords to display firefox tree heads and nearest firefox tree head.

Comment 1

[Gregory Szorc [:gps]]

Attachment: firefoxtree: implement {fxheads} template keyword ()

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: firefoxtree: implement {nearestfxhead} for finding what tree a changeset is on ()

I want to change this a bit before it lands. So only asking for
feedback.

Comment 3

[(not currently active) Ted Mielczarek]

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

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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?

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/0dfbdacd7a45

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Gregory Szorc [:gps]]

Attachment: firefoxtree: implement {nearestfxhead} for finding what tree a changeset is on

Here is a slightly better implementation of the walk-towards-roots
template.

Comment 9

[(not currently active) Ted Mielczarek]

I think your {nearestfxhead} naming is fine for use case #1, keeping {fxheads} for #2.

Comment 10

[Gregory Szorc [:gps]]

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.

Comment 11

[Gregory Szorc [:gps]]

I'm not actively working on this.