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

cli: say "(forgotten)" if branch is listed just because it's in git_refs #1739

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

yuja
Copy link
Collaborator

@yuja yuja commented Jun 25, 2023

@ilyagr, can you take a look?

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 3 commits June 25, 2023 20:40
It only applies to local branches, and None doesn't always mean deleted as
we have pseudo "git" remote.
I thought we would need additional bookkeeping to detect forgotten branches,
but I was wrong. If a branch exists only in git_refs, it is forgotten (but not
yet exported.)
Inspired by d01ecc5 "more detailed message describing deleted branches."

And yes, "jj git export" does propagate "jj branch forget" to the underlying
Git repository, which strengthen my feeling that git::export_refs() should
also remove "forgotten" remote tracking refs.
Copy link
Collaborator

@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.

Looks good, thanks!

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 25, 2023

Re commit message.

And yes, "jj git export" does propagate "jj branch forget" to the underlying
Git repository,

It does. I did say otherwise once, I hope I corrected myself but I'm unsure.

which strengthen my feeling that git::export_refs() should also remove "forgotten" remote tracking refs.

I hope to have a more educated opinion on this soon. I might even add this functionality to #1700 to fix a bug. The other option that a priori (i.e. in my uneducated opinion) also seems reasonable to me would be to have jj git push do that (as I said before).

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 26, 2023

which strengthen my feeling that git::export_refs() should also remove "forgotten" remote tracking refs.

I hope to have a more educated opinion on this soon.

If we have export_refs affect git repo's remote-tracking branches, and if we want jj fetch/push to behave similarly in colocated and normal repos, I think we need to run a limited jj git import and jj git export (on remote-tracking branches only) before and after every command in a normal jj repo.

(In principle, we could restrict it to undo, fetch, push, maybe import and export themselves, and branch forget, but keeping a list of such commands constantly up to date seems dangerous to me. There might be other ways to optimize it.)

I went through one example: undo of push from Martin's #1541. I think that if we also do what I said in the previous paragraph, such a change would get rid of the conflict in that test. This is likely a good thing.

I haven't yet thought through what other good (or bad) consequences this change might have, but the one example gave me the sense that this export seems make undo work better with fetch/push when undo restores remote-tracking branches (as it does now). So, this might be a much better alternative to my suggestion from #1541 to have undo preserve remote-tracking branches (which, as we discussed there, is problematic).

I don't like my idea of only importing/exporting these refs on fetch and push as much anymore. In that test, I think we'd make sure there'd be an export of remote-tracking branches before the fetch for things to work out neatly, and that seems messy.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 26, 2023

Also, the fact that import already import git's remote-tracking branches and export doesn't export them is clearly bad. Fixing that would take some care so that jj git push still works (I haven't looked in detail into how that works yet).

I previously thought this is OK because I thought that import of remote-tracking branches is always a no-op except for git fetch, but that's not true for undo. Also, I now think it's not OK that the import of remote-tracking branches doesn't happen after every command, since it seems to make a difference

I'll try writing some versions of Martin's test that demo this. (Update: done, see the second commit of #1700).

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 26, 2023

Fixing that would take some care so that jj git push still works

Another thing to worry about is whether this is safe while jj git push doesn't check whether the branch is at the expected place on the remote. Alternatively (or additionally), when we do have jj git push check that, export of remote-tracking branches could create weird conflicts.

These things should be solvable, but it might take time and effort.

@yuja
Copy link
Collaborator Author

yuja commented Jun 26, 2023

Also, the fact that import already import git's remote-tracking branches and export doesn't export them is clearly bad.

Yes, that's also my concern.

Another way around is to not import nor export .git/refs/remotes/* by jj git import/export. Only jj git fetch/push will update these refs. This will remove the need of view.git_refs()["refs/remotes/*"] as we always trust remote's/jj's refs.
I think this will be close to the model where local git repo is a pseudo remote named "git".

  • jj git import imports delta between view.git_refs()["refs/heads/$name"] and git_repo.references()["refs/heads/$name"]
    (if local git repo were a remote, view.git_refs()["refs/heads/$name"] would be view.branches()[$name].remote_targets["git"])
  • jj git fetch imports delta between view.branches()[$name].remote_targets[$remote] and git_repo.references()["refs/remotes/$remote/$name"] after fetching from the remote

(FWIW, I use git fetch (because libgit2 is super slow), so the current jj git import behavior is convenient for me. I don't think we need to support such weird scenario.)

If we have export_refs affect git repo's remote-tracking branches, and if we want jj fetch/push to behave similarly in colocated and normal repos, I think we need to run a limited jj git import and jj git export (on remote-tracking branches only) before and after every command in a normal jj repo.

If that means jj's remote-tracking branches should be always kept in sync with backed git repo, can't we blindly trust jj's remote branch targets?

@yuja yuja merged commit 3c2657c into martinvonz:main Jun 26, 2023
@yuja yuja deleted the push-yyqnruszvxmt branch June 26, 2023 04:56
@ilyagr
Copy link
Collaborator

ilyagr commented Jun 26, 2023

Another way around is to not import nor export .git/refs/remotes/* by jj git import/export. Only jj git fetch/push will update these refs. This will remove the need of view.git_refs()["refs/remotes/*"] as we always trust remote's/jj's refs.

I think this would mean that, in a colocated repo, using git for a git fetch would not work well as the changes wouldn't be imported by jj. Using git's git pull would result in the local branch being imported but not the remote-tracking branch, creating problems on the next jj git fetch/jj git push.

Update: I'm unsure, but perhaps adding some sort of post-fetch & pre-push git hook could help here. I bet not all git clients execute those hooks, though.

@yuja
Copy link
Collaborator Author

yuja commented Jun 27, 2023

I think this would mean that, in a colocated repo, using git for a git fetch would not work well as the changes wouldn't be imported by jj.

Yes. We'll need to run jj git fetch or something like jj git import --remote.

Using git's git pull would result in the local branch being imported but not the remote-tracking branch, creating problems on the next jj git fetch/jj git push.

Good point about git pull. To support git pull, maybe we shouldn't import half of local/remote ref pairs. And local git repo can't be modeled as a pseudo remote?

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 27, 2023

And local git repo can't be modeled as a pseudo remote?

I don't think it can. I used to think otherwise, back when I also thought that we might be able to stop tracking git repo's remote-tracking branches in jj's git-tracking refs, but I think we have to keep track of them.

One alternative that seems pretty clean to me would be to have jj git fetch and jj git push work as you described above (ignore git-tracking refs and git repo's remote-tracking branches and only look at jj's remote-tracking branch) and have jj git import/export be 100% responsible for syncing jj's refs (both local and remote-tracking) with the backing git repo's refs in colocated repos. (This uses jj's git-tracking refs for both local and remote-tracking branches.) I don't understand libgit2 enough to know whether implementing jj git fetch this way is practically possible, though.

If you use git fetch in a normal jj repo, this would mean that you'd need to run jj git import manually. Also, this makes the backing git repo quite different from a normal remote conceptually.

BTW, I think it's quite important to support people using git's git fetch in colocated repos. I imagine there will be situations where jj git fetch doesn't implement some bespoke git fetch feature somebody is required to use for their work (some strange credential helper, say), and that would make jj useless to them if there isn't a workaround.

@yuja
Copy link
Collaborator Author

yuja commented Jun 28, 2023

And local git repo can't be modeled as a pseudo remote?

I don't think it can. I used to think otherwise, back when I also thought that we might be able to stop tracking git repo's remote-tracking branches in jj's git-tracking refs, but I think we have to keep track of them.

I see. As the one who actually use git fetch, the current import behavior is preferred. ;)

I think the problem of the current model is that the last-known git_refs are opaque to user.
@git branch in jj branch list helps to visualize local git_refs state. We'll need something similar to that for remote refs. (Maybe that's what you've described in #1666?)

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 28, 2023

Exactly, that's what I was thinking in #1666 (comment).

ilyagr added a commit that referenced this pull request Jul 3, 2023
The original test is copied from @martinvonz 's [PR draft] (thanks!).

The three versions show differences in behavior due to import/export
of remote-tracking branches, and due to repo being colocated.

The former is relevant for [the discussion] of whether `jj git export` should
export remote-tracking branches. The latter will change in a follow-up commit.

Outstanding TODO: check if we have similar tests for undoing `fetch`

[PR draft]: #1541
[the discussion]: #1739
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.

2 participants