Add optional depth parameter to ancestors() revset function

[sullyj3]

Allows ancestors(rev, n) in the revset language.

⮞ jj log -r 'ancestors(@, 5)'
@  korrtmol [email protected] 2023-09-02 17:37:15.000 +10:00 c9dca8e9
│  (empty) Add test for revset n ancestors
◉  owvqrlnu [email protected] 2023-09-02 17:35:20.000 +10:00 main* HEAD@git 498cd57b
│  Add optional argument n to ancestors() in revset language
◉  yoslkysy [email protected] 2023-09-02 17:09:53.000 +10:00 80456ec4
│  Add generations argument to RevsetExpression::ancestors method
◉  uqwupmro [email protected] 2023-09-01 16:49:23.000 -07:00 main@upstream e3825495
│  store: don't include cached objects in `Debug` format
◉  ostmnpsu [email protected] 2023-09-01 16:23:39.000 -07:00 16c897d8
│  conflicts: leverage `Merge::map()` in `materialize_merge_result()`
~

See #1984 for some context.

Since I care most about the git log -n 5 use-case, I chose to implement the simplest option of those discussed, returning the last n changes, including the "descendent" change.

Support for ancestors(x, m, n) can be added later, since it will be backwards compatible. However, if we later went for the ancestors(x, m..n) syntax, I believe that would require a breaking change, unless it was allowed to support either a number or a range argument. I'm not familiar enough with the language to know if that's possible.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[yuja]

Thanks, the code looks good to me. Can you also update docs/revsets.md (and add a couple of tests)?

Maybe we need to call the count argument differently in the doc since it's the limit of the ancestor lookup. Mercurial calls it a depth. I think generation is also good.

Support for ancestors(x, m, n) can be added later, since it will be backwards compatible. However, if we later went for the ancestors(x, m..n) syntax, I believe that would require a breaking change, unless it was allowed to support either a number or a range argument.

Actually adding ancestors(x, m, n) would be a bit trickier than m..n since the arguments order differs from ancestors(x, n).

[sullyj3]

For sure! For the tests I was thinking just a couple different invocations of jj log, with and without the optional argument. Should I stick them in cli/tests/test_revset_output.rs, or cli/tests/test_log_command.rs?

[yuja]

For the tests I was thinking just a couple different invocations of jj log, with and without the optional argument. Should I stick them in cli/tests/test_revset_output.rs, or cli/tests/test_log_command.rs?

I think lib/tests/test_revset.rs is better place since we don't change the CLI behavior. test_evaluate_expression_ancestors() have some tests for :x (or ancestors(x).)

[sullyj3]

Ah perfect, I missed the lib tests somehow.

[sullyj3]

Maybe we need to call the count argument differently in the doc since it's the limit of the ancestor lookup. Mercurial calls it a depth. I think generation is also good.

I like depth. In the code, "generation" is used to refer to a range, whereas I think "depth" implies a single number. I'll also change the code to keep it consistent.

[sullyj3]

Just gotta add docs, and I believe the config schema is not applicable to this

[sullyj3]

rebased on main

[martinvonz]

Thanks for your PR. I think you can squash all 6 commits into one, per
https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews

[sullyj3]

Thanks for your PR. I think you can squash all 6 commits into one, per https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews

I'm happy to squash, but on my reading contributing.md says not to! It might need updating if that's your preference, unless I'm misunderstanding.

[yuja]

Thanks for your PR. I think you can squash all 6 commits into one, per https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews

I'm happy to squash, but on my reading contributing.md says not to!

Perhaps "we don't squash them when the PR is ready" could be misinterpreted, but it means the PR isn't squashed when integrating, so it's up to contributor to organize commits in meaningful way.

I think the first RevsetExpression::ancestors() change can be a separate commit, but the other 5 commits do related code + test + doc changes separately, so they can be squashed into one.

[sullyj3]

Ahh, I see, that makes sense.

[yuja]

I'm not sure whether exclusive depth is intuitive or not, but other than that, the change LGTM, thanks.

You'll probably need to cargo fmt the first commit.

[martinvonz]

I'm not sure whether exclusive depth is intuitive or not, but other than that, the change LGTM, thanks.

I'm also not sure, but to be clear, I don't want that to block this PR. We can always change it later if necessary.

[sullyj3]

cargo fmt done!

[martinvonz]

Looks good, thanks!

[sullyj3]

Auspicious occasion, that was my first rust open source contribution. Thanks both for your help!