bookmark: Make `bookmark {create, set, move}` unhide hidden commits #5050

PhilipMetzger · 2024-12-07T23:20:02Z

This was discussed in Discord a while ago, and this is the logical and consistent
conclusion. Implementing it as such makes it consistent with both jj edit and jj new
which unhide any predecessor.

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

tim-janik · 2024-12-08T00:14:04Z

This was discussed in Discord a while ago, while I'm still in favor of making it a hard error down the line, it currently is just implemented as a warning to get user feedback.

TODO: write tests and update CHANGELOG.md

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

IMHO, adding a tag or a bookmark to a hidden commit should make it visible.
It is one of the first things newbies try to resurrect a hidden commit.
I wonder what the rationale is for issuing a warning or an error in this case.
As far as my experience goes, adding tags or bookmarks to hidden commits is normally a deliberate action, do you know of cases where this would frequently happen in error?
And even if so, what is the rationale for jj new <hiddencommit> being OK and jj b c <hiddencommit> not?
(Honestly asking, I really don't get it)

PhilipMetzger · 2024-12-08T00:36:27Z

IMHO, adding a tag or a bookmark to a hidden commit should make it visible.
It is one of the first things newbies try to resurrect a hidden commit.

Have a reference for that, beside you?

I wonder what the rationale is for issuing a warning or an error in this case.
As far as my experience goes, adding tags or bookmarks to hidden commits is normally a deliberate action, do you know of cases where this would frequently happen in error?

While it works, it shouldn't really be a option since those commits should be addressable in the perfect model anyway. It is a mistake to address a predecessor as existing in the repo.

And even if so, what is the rationale for jj new <hiddencommit> being OK and jj b c <hiddencommit> not?
(Honestly asking, I really don't get it)

jj new resurfaces it back in the log, while any bookmark command won't.

yuja

adding a tag or a bookmark to a hidden commit should make it visible.

That's a good point. I'm not sure it's useful in practice, but the behavior is consistent.
(Un-hidden commits can be warned, but that would also apply to jj new.)

cli/src/commands/bookmark/create.rs

tim-janik · 2024-12-08T01:05:20Z

IMHO, adding a tag or a bookmark to a hidden commit should make it visible.
It is one of the first things newbies try to resurrect a hidden commit.

Have a reference for that, beside you?

You might find some if you look for "branch" and "hidden" on discord.

And even if so, what is the rationale for jj new <hiddencommit> being OK and jj b c <hiddencommit> not?
(Honestly asking, I really don't get it)

jj new resurfaces it back in the log, while any bookmark command won't.

I know the current state of affairs.
I am asking for a reason, because I don't see the logic behind the current inconsistency.
Can you give any, please?

joyously · 2024-12-08T01:33:02Z

I'm unsure what makes a commit hidden, and if there could be confusion between "commit" and "change" in this context.

It is a mistake to address a predecessor as existing in the repo.

Would this apply to the use case of taking the current jj repo and making a bookmark for each release that has come out?

martinvonz · 2024-12-09T00:51:09Z

I am asking for a reason, because I don't see the logic behind the current inconsistency.
Can you give any, please?

The set of visible commits is defined by a set of visible heads. Some pretty low-level library code adds new commits to that set because we generally want new commits to be visible. Until #4427, jj edit wouldn't even update the set (because it doesn't create a new commit). So I think the inconsistency is not because we decided that that's best for the user as it is because we haven't thought much about it. I think it makes sense to make a commit visible if we move a bookmark to it.

PhilipMetzger · 2024-12-11T19:32:06Z

IMHO, adding a tag or a bookmark to a hidden commit should make it visible.
It is one of the first things newbies try to resurrect a hidden commit.

Have a reference for that, beside you?

You might find some if you look for "branch" and "hidden" on discord.

The only conversation which talks about this behavior is already mentioned, so this clearly doesn't qualify for a "reference beside you".

So I think the inconsistency is not because we decided that that's best for the user as it is because we haven't thought much about it. I think it makes sense to make a commit visible if we move a bookmark to it.

That also works for me,

I actually made this patchset explicitly a warning to see how many complaints it will receive when it lands, since we don't know how many people actually use the workflow @tim-janik argues for.

tim-janik · 2024-12-11T20:23:35Z

Have a reference for that, beside you?

You might find some if you look for "branch" and "hidden" on discord.

The only conversation which talks about this behavior is already mentioned, so this clearly doesn't qualify for a "reference beside you".

There are several discussions if you bother searching for "hidden branch" or "branch hidden" on the jj discord server, but AFAIK there is no way to link to searches on discord. You could also search on GH and might find #4537, etc, but really, this point is nothing more than a distraction.

PhilipMetzger · 2024-12-11T20:47:41Z

There are several discussions if you bother searching for "hidden branch" or "branch hidden" on the jj discord server, but AFAIK there is no way to link to searches on discord.

You can link the individual messages, so provide them if you care so much instead calling my argument a shallow dismissal.

To be used in the next diff. Also simplify the `hidden()` revset with it.

This was discussed in the Discord a while ago, and this is the logical and consistent conclusion. Implementing it as such makes it consistent with both `jj edit` and `jj new` which unhide any predecessor.

martinvonz

I wonder if we should make the commit at a lower level so we don't risk missing it. Like this: main...push-uulxprvmmtsw

The drawback is that if some caller really wants to point a bookmark to a hidden commit, then they'll have to set the bookmark and then make the commit hidden afterwards (and be careful to only do that if it was already hidden).

martinvonz · 2024-12-20T21:28:17Z

cli/src/commit_templater.rs

+                let value = commit.is_hidden(repo);
+                value.unwrap_or_default()


nit: inline value? It's so short now that I think it's easier to read as one expression

martinvonz · 2024-12-20T21:31:00Z

cli/src/commands/bookmark/create.rs

Implementing it as such makes it consistent with both jj edit and jj new
which unhide any predecessor.

I find "predecessor" a bit confusing the unhidden commit doesn't need to be a predecessor of the current commit, or a predecessor of any commit at all for that matter (maybe it was just abandoned).

martinvonz · 2024-12-20T21:33:12Z

cli/src/commands/bookmark/create.rs

@@ -53,7 +53,9 @@ pub fn cmd_bookmark_create(
    let mut workspace_command = command.workspace_helper(ui)?;
    let target_commit = workspace_command
        .resolve_single_rev(ui, args.revision.as_ref().unwrap_or(&RevisionArg::AT))?;
+    let repo = workspace_command.repo();
    let view = workspace_command.repo().view();


nit: use repo here

yuja · 2024-12-21T01:10:44Z

I wonder if we should make the commit at a lower level so we don't risk missing it. Like this: main...push-uulxprvmmtsw

It might be good to add repo method that updates bookmark refs and makes targets visible, but I would expect there's an escape hatch to do low-level thing.

yuja · 2024-12-21T01:01:13Z

cli/src/commands/bookmark/create.rs

    let view = workspace_command.repo().view();
+    let is_hidden = target_commit.is_hidden(repo.as_ref())?;


nit: I think is_hidden() test is redundant (in all commands.) iirc, these commands check if bookmarks are moved/created, so we can just do .add_head() if there are any bookmark changes.

True, the repo seems like a better level to do it. I've updated my branch. I should also add tests for it in jj-lib. Or do you want to take my commit and add tests, @PhilipMetzger?

I think is_hidden() test is redundant (in all commands.) iirc, these commands check if bookmarks are moved/created, so we can just do .add_head() if there are any bookmark changes.

I agree, but it was the simplest way to implement it.

True, the repo seems like a better level to do it. I've updated my branch. I should also add tests for it in jj-lib. Or do you want to take my commit and add tests, @PhilipMetzger?

I in general agree with the direction we take here, so I probably should take over Martin's branch and drop the first commit, if it otherwise doesn't provide value.

PhilipMetzger · 2024-12-21T18:57:14Z

The drawback is that if some caller really wants to point a bookmark to a hidden commit, then they'll have to set the bookmark and then make the commit hidden afterwards (and be careful to only do that if it was already hidden).

Is that a thing we actually want? As it doesn't make sense from the viewpoint of the CLI, like we can provide a API for that but it should be a error otherwise.

yuja · 2024-12-22T01:13:06Z

The drawback is that if some caller really wants to point a bookmark to a hidden commit, then they'll have to set the bookmark and then make the commit hidden afterwards (and be careful to only do that if it was already hidden).

Is that a thing we actually want? As it doesn't make sense from the viewpoint of the CLI, like we can provide a API for that but it should be a error otherwise.

Callers might depends on property that set() + unset() is noop. It's probably better to rename the function if it does set() + add_heads().

yuja reviewed Dec 8, 2024

View reviewed changes

cli/src/commands/bookmark/create.rs Outdated Show resolved Hide resolved

commit: Add commit.is_hidden(repo)

c8fe996

To be used in the next diff. Also simplify the `hidden()` revset with it.

PhilipMetzger force-pushed the pm/push-tpqqxxvkxsok branch 2 times, most recently from 83208e0 to a4802a6 Compare December 20, 2024 21:23

PhilipMetzger changed the title ~~bookmark: Make moving a bookmark onto a hidden commit a warning~~ bookmark: Make bookmark {create, set, move} unhide hidden commits Dec 20, 2024

PhilipMetzger force-pushed the pm/push-tpqqxxvkxsok branch from a4802a6 to 85b98e9 Compare December 20, 2024 21:27

bookmark: Make bookmark {create, set, move} unhide hidden commits

e08a04e

This was discussed in the Discord a while ago, and this is the logical and consistent conclusion. Implementing it as such makes it consistent with both `jj edit` and `jj new` which unhide any predecessor.

PhilipMetzger force-pushed the pm/push-tpqqxxvkxsok branch from 85b98e9 to e08a04e Compare December 20, 2024 21:34

PhilipMetzger marked this pull request as ready for review December 20, 2024 21:46

martinvonz reviewed Dec 20, 2024

View reviewed changes

yuja reviewed Dec 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bookmark: Make `bookmark {create, set, move}` unhide hidden commits #5050

bookmark: Make `bookmark {create, set, move}` unhide hidden commits #5050

PhilipMetzger commented Dec 7, 2024 •

edited

Loading

tim-janik commented Dec 8, 2024

Checklist

PhilipMetzger commented Dec 8, 2024

yuja left a comment

tim-janik commented Dec 8, 2024

joyously commented Dec 8, 2024

martinvonz commented Dec 9, 2024

PhilipMetzger commented Dec 11, 2024

tim-janik commented Dec 11, 2024

PhilipMetzger commented Dec 11, 2024

martinvonz left a comment

martinvonz Dec 20, 2024

martinvonz Dec 20, 2024

martinvonz Dec 20, 2024

yuja commented Dec 21, 2024

yuja Dec 21, 2024

martinvonz Dec 21, 2024

PhilipMetzger Dec 21, 2024

PhilipMetzger commented Dec 21, 2024

yuja commented Dec 22, 2024

		let value = commit.is_hidden(repo);
		value.unwrap_or_default()

		let view = workspace_command.repo().view();
		let is_hidden = target_commit.is_hidden(repo.as_ref())?;

bookmark: Make bookmark {create, set, move} unhide hidden commits #5050

Are you sure you want to change the base?

bookmark: Make bookmark {create, set, move} unhide hidden commits #5050

Conversation

PhilipMetzger commented Dec 7, 2024 • edited Loading

Checklist

tim-janik commented Dec 8, 2024

Checklist

PhilipMetzger commented Dec 8, 2024

yuja left a comment

Choose a reason for hiding this comment

tim-janik commented Dec 8, 2024

joyously commented Dec 8, 2024

martinvonz commented Dec 9, 2024

PhilipMetzger commented Dec 11, 2024

tim-janik commented Dec 11, 2024

PhilipMetzger commented Dec 11, 2024

martinvonz left a comment

Choose a reason for hiding this comment

martinvonz Dec 20, 2024

Choose a reason for hiding this comment

martinvonz Dec 20, 2024

Choose a reason for hiding this comment

martinvonz Dec 20, 2024

Choose a reason for hiding this comment

yuja commented Dec 21, 2024

yuja Dec 21, 2024

Choose a reason for hiding this comment

martinvonz Dec 21, 2024

Choose a reason for hiding this comment

PhilipMetzger Dec 21, 2024

Choose a reason for hiding this comment

PhilipMetzger commented Dec 21, 2024

yuja commented Dec 22, 2024

bookmark: Make `bookmark {create, set, move}` unhide hidden commits #5050

bookmark: Make `bookmark {create, set, move}` unhide hidden commits #5050

PhilipMetzger commented Dec 7, 2024 •

edited

Loading