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.

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

git: on external HEAD move, do not abandon old branch #1592

Merged
merged 2 commits into from
May 11, 2023

Conversation

yuja
Copy link
Contributor

@yuja yuja commented May 10, 2023

Checklist

If applicable:

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

yuja added 2 commits May 10, 2023 21:57
I'm going to change the behavior of _without_ref() case to mitigate jj-vcs#1042.
The current behavior was introduced by 20eb9ec "git: don't abandon
HEAD commit when it loses a branch." While the change made HEAD mutation
behavior more consistent with a plain ref operation, HEAD can also move on
checkout, and checkout shouldn't be considered a history rewriting operation.

I'm not saying the new behavior is always correct, but I think it's safer
than losing old HEAD branch. I also think this change will help if we want
to extract HEAD management function from git::import_refs().

Fixes jj-vcs#1042.
@martinvonz
Copy link
Member

Does this mean that if you create a temporary detached HEAD, it will remain visible? Something like this:

$ git checkout v1.0.0 # check out a commit to test something
$ <add printf debugging>
$ git commit -m testing
$ jj log
$ git checkout main # done testing
$ jj log # Is the "testing" commit still there?

It seems inconsistent to only add detached HEAD commits and never delete them, but maybe that's the pragmatic thing to do - it's a lot easier to notice the unwanted commit and run jj abandon on it than it is to notice a missing commit and recover it.

@yuja
Copy link
Contributor Author

yuja commented May 10, 2023

Does this mean that if you create a temporary detached HEAD, it will remain visible? Something like this:

$ git checkout v1.0.0 # check out a commit to test something
$ <add printf debugging>
$ git commit -m testing
$ jj log
$ git checkout main # done testing
$ jj log # Is the "testing" commit still there?

Correct. I don't think the testing commit should be discarded in that scenario because detached (or anonymous) branch isn't hidden in jj world.

It seems inconsistent to only add detached HEAD commits and never delete them, but maybe that's the pragmatic thing to do - it's a lot easier to notice the unwanted commit and run jj abandon on it than it is to notice a missing commit and recover it.

Yes, I agree it's inconsistent, but I think it's less scary.

@martinvonz
Copy link
Member

Does this mean that if you create a temporary detached HEAD, it will remain visible? Something like this:

$ git checkout v1.0.0 # check out a commit to test something
$ <add printf debugging>
$ git commit -m testing
$ jj log
$ git checkout main # done testing
$ jj log # Is the "testing" commit still there?

Correct. I don't think the testing commit should be discarded in that scenario because detached (or anonymous) branch isn't hidden in jj world.

My thinking behind the current behavior (before this PR) was that it used to be visible in Git and now it's no longer visible, so we should propagate that change into jj. But as I said, I see how changing the behavior is more pragmatic.

@ilyagr
Copy link
Contributor

ilyagr commented May 11, 2023

Yeah, this seems like a good idea.

@yuja yuja merged commit 92cfffd into jj-vcs:main May 11, 2023
@yuja yuja deleted the push-vxwqyknxyqlq branch May 11, 2023 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants