Skip to content
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

bookmark: Make moving a bookmark onto a hidden commit a warning #5050

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

bookmark: Make moving a bookmark onto a hidden commit a warning #5050

wants to merge 2 commits into from
+54 −2

Conversation

PhilipMetzger
Copy link
Collaborator

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

@PhilipMetzger
commit: Add commit.is_hidden(repo)
f76f496 
To be used in the next diff. Also simplify the `hidden()` revset with it.
@PhilipMetzger
bookmarks: Make all bookmark creating commands onto a hidden commit a…
e024ca7 
… warning

This was discussed in the Discord a while ago. While I still think we
should make it a hard error at some point, lets get some feedback to
see how often it actually occurs.
@tim-janik
Copy link
Collaborator

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
Copy link
Collaborator Author

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
yuja reviewed Dec 8, 2024
View reviewed changes
Copy link
Collaborator

@yuja yuja left a comment

Choose a reason for hiding this comment

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

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
let view = workspace_command.repo().view();
let is_hidden = target_commit.is_hidden(repo.deref())?;
Copy link
Collaborator

@yuja yuja Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: .as_ref() (or something like &*repo.)

@tim-janik
Copy link
Collaborator

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
Copy link

joyously commented Dec 8, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja left review comments

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.
5 participants
@PhilipMetzger @tim-janik @joyously @martinvonz @yuja