Skip to content

cli: fix version test when @ has multiple parents #6311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
winterqt wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cli: fix version test when @ has multiple parents #6311

winterqt wants to merge 1 commit into from
+1 −0

Conversation

@winterqt
Copy link
Member

@winterqt winterqt commented Apr 10, 2025

The jj log command ran by the build script returns the commit IDs concatenated together, while the test expects only one ID in the version string.

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

@winterqt
cli: fix version test when @ has multiple parents
4361965 
The `jj log` command ran by the build script returns the commit IDs
concatenated together, while the test expects only one ID in the
version string.
@winterqt winterqt requested a review from a team as a code owner April 10, 2025 21:43
@thoughtpolice
Copy link
Member

thoughtpolice commented Apr 10, 2025

This has hit people before (normally I don't run into it, because when I do a mega-merge I always make my @ sit on top of the merge rather than be the merge itself) but I wonder if we shouldn't just delete the test instead and make the version string more obvious in the build.rs file... I honestly don't know if the version test is worth its weight for stuff like this. /cc @ilyagr

@thoughtpolice thoughtpolice requested a review from ilyagr April 11, 2025 00:47
ilyagr
ilyagr approved these changes May 9, 2025
View reviewed changes
Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This has bothered me a few times, I think it's worth the quick fix.

cli/build.rs
"--color=never",
"log",
"--no-graph",
"-r=@-",
Copy link
Contributor

@ilyagr ilyagr May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could be slightly more deterministic and do -r=latest(@-).

My initial idea was to include all the commits with a separator, but I guess this is simpler.

There are still many things wrong with it, e.g. it doesn't detect whether @ is non-empty, I believe, so I think the loss of precision is OK.

@ilyagr
Copy link
Contributor

ilyagr commented May 9, 2025 •
edited
Loading

Some less actionable and more wishful thoughts.

I wonder if we should implement jj version --verbose that would include all the parents and/or, perhaps,

In an ideal world, I think jj version --verbose would mention things like:

  • Number of commits since last release, similarly to git describe.
  • Id(s) of the nearest non-empty ancestor of @ (or @ itself if it's non-empty)
  • Id(s) of root(@..main), if that's different from the above commit, and the number of non-empty commits since then (including @ if it's non-empty).

It's somewhat tedious to do, though.

The reason for --verbose is that I'm now leaning against changing the output of jj version too much, since that can mess up packaging scripts and other integrations that try to parse jj version.

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented May 9, 2025

I wonder if we should implement jj version --verbose that would include all the parents and/or, perhaps,

In an ideal world, I think jj version --verbose would mention things like:

  • Number of commits since last release, similarly to git describe.
  • Id(s) of the nearest non-empty ancestor of @ (or @ itself if it's non-empty)
  • Id(s) of root(@..main), if that's different from the above commit, and the number of non-empty commits since then (including @ if it's non-empty).

It's somewhat tedious to do, though.

There is @thoughtpolice PR which brings some of these features here, #3638 imo longterm its still the correct fix, even though nothing has happened with it for half a year.

ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
fb10a78 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
@ilyagr ilyagr mentioned this pull request Jul 5, 2025
Merged
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
a2200da 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
9a3c317 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
7a37644 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
73a66a6 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
5db4b93 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
769d062 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
2b1cfb7 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
637140e 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
ilyagr added a commit that referenced this pull request Jul 5, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
0d434db 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
github-merge-queue bot pushed a commit that referenced this pull request Jul 8, 2025
@ilyagr
build.rs: separate commit ids if compiling at a merge commit
083fdaa 
This makes the `test_version` test failure when run at a merge commit
less confusing. The test could be fixed, but I'm not sure it's worth it,
as we probably don't want to release a version of `jj` compiled at a
merge commit.

Now, if `jj` is compiled at a merge commit, this would print:

```
$ jj --version
jj 0.31.0-fb10a78cb359d52c0eda518a42ab07f117909004-ff9c9d0d8b4e929843eb683709fc1717645796df-99e035c5054e71906d7293ed98b542a9055057ef
```

Admittedly, this is a bit of a quick hack, but I've been confused by
`jj --version` output when compiled at a merge commit enough times
to want it.

Prior art: #6311 and
#4033 both pick one commit id hash
to print. I think I prefer printing all of them, even if it will fail
tests at a merge commit. After this commit, the failure will at least
be easy to understand.

However, if we prefer to merge one of the others, I'm OK with that too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ilyagr ilyagr ilyagr approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@winterqt @thoughtpolice @ilyagr @PhilipMetzger