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: branch: on create/set/rename, assume deleted tracking local branch still exists #3884

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

yuja
Copy link
Collaborator

@yuja yuja commented Jun 14, 2024

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

cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
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.

I am still undecided on the best possible message, but it's OK already, and that can be fixed up later. The rest of the PR LGTM.

Thank you!

Copy link
Collaborator Author

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

FWIW, this PR can go away if we decide to reorganize create + set as something like set (upsert) + move.

#3584

The weirdness I'm trying to fix by this patch is that "create" preserves the existing tracking state. If the command name were "set", the behavior makes sense.

cli/src/commands/branch.rs Outdated Show resolved Hide resolved
cli/src/commands/branch.rs Outdated Show resolved Hide resolved
@yuja
Copy link
Collaborator Author

yuja commented Jun 16, 2024

FWIW, this PR can go away if we decide to reorganize create + set as something like set (upsert) + move.

Created #3895

I personally prefer it over this PR since it seems more useful and will have a fewer special cases.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 17, 2024

Wouldn't this problem affect jj branch set --new just as much as jj branch create?

@yuja
Copy link
Collaborator Author

yuja commented Jun 17, 2024

Wouldn't this problem affect jj branch set --new just as much as jj branch create?

My feeling is that set --new is okay to preserve existing tracking state (and maybe show warning?) --new might be a bad name, but it's the flag to not move existing branches. I feel "create" has stronger guarantee.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 17, 2024

I don't think I would agree if jj branch set --create only created branches.

If jj branch set --create creates-or-moves (aka upserts, I think) branches, this makes more sense. I'm guessing this is your preference.

Some less solidified thoughts:

This might create a (minor? not minor?) problem if the user deleted a branch mybranch, thought it was gone, and then did jj branch set --create mybranch with the intention of creating a new branch. Instead, they would get a branch that's tracked by random remote branches they didn't know about. So, this seems like a (minor?) problem with the whole upsert idea that would be avoided by having jj branch create (possibly in addition to jj branch set --create).

I am not sure this is a fatal problem. The same problem would also happen if you jj branch set --create-d a new mybranch before there were any remote-tracking branches, and then fetched from a remote that has mybranch on it. So, maybe it's OK to have the same problem in the jj branch set --create mybranch case, I'm unsure.

Finally, if jj branch set --create upserts branches, I am realizing I do have a mild preference for keeping jj branch create for one more reason. As I was writing last paragraph, it was much more natural to write "if you jj branch create mybranch" as opposed to "if you jj branch set --create-d a new mybranch".

If we keep jj branch create in any form, I do think something like this PR would be useful.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 17, 2024

(Update: I belatedly realized I should have posted this in #3884 (comment), now this post is more or less duplicated)

As an aside, I'm pretty sure I want jj branch forget --local to exist. It's not actually exactly the same as jj branch untrack glob:branch@* if the branch exists. Once it exists, we could put it into the hints of this PR (only if we continue with this PR, of course, I was mostly thinking about this before I saw the other one).

Most likely, I'll require people to specify either --local or --global (and perhaps --from-remotes that lists specific remotes or a glob would be a third option eventually). In the long term, I think I prefer either --local being the default or there being no default.

Let me know if you prefer not doing this or find this controversial enough to benefit from filing an FR before trying to implement a PR.

@yuja
Copy link
Collaborator Author

yuja commented Jun 17, 2024

I don't think I would agree if jj branch set --create only created branches.

If jj branch set --create creates-or-moves (aka upserts, I think) branches, this makes more sense. I'm guessing this is your preference.

My plan is:

  • make jj branch set do upsert by default (i.e. create-or-move)
  • add --new (or deny-move) flag for users who want to ensure that the branch doesn't exist.

The reason behind the idea is that jj config set does upsert, and if we add jj tag set, I think it would do upsert with --allow-overwrite flag or something. (It wouldn't make sense to add tag set for very narrow use case, in addition to tag create.)

This might create a (minor? not minor?) problem if the user deleted a branch mybranch, thought it was gone, and then did jj branch set --create mybranch with the intention of creating a new branch. Instead, they would get a branch that's tracked by random remote branches they didn't know about.

Yeah. That's why I think we might want some warning message.

Another reason I'm getting skeptical about this PR is that jj branch rename old new && jj branch rename new old will no longer work if old had tracking remotes and renaming back to deleted tracking branch is a hard error.

ilyagr
ilyagr previously approved these changes Jun 28, 2024
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 great, thank you!

cli/src/commands/branch/rename.rs Outdated Show resolved Hide resolved
cli/tests/test_branch_command.rs Outdated Show resolved Hide resolved
@ilyagr ilyagr dismissed their stale review June 28, 2024 05:13

A few messages to fix, sorry.

cli/src/commands/branch/create.rs Outdated Show resolved Hide resolved
cli/src/commands/branch/set.rs Show resolved Hide resolved
yuja added 2 commits June 28, 2024 19:03
…d remotes

We use "tracked remote branches" in the doc, and hints are usually capitalized.
While explaining branch tracking behavior, I find it's bad UX that a deleted
branch can be re-"create"d with tracking state preserved. It's rather a "set"
operation. Since deleted tracking branch is still listed, I think it's better
to assume that the local branch name is reserved.

martinvonz#3871

Renaming to deleted tracking branch is still allowed (with warning) because the
"rename" command can't handle tracked remotes very well. If it were banned, bad
rename couldn't be reverted by using "jj branch rename". It would be confusing
if "rename a b" succeeded with warning, but the following "rename b a" failed.
Copy link
Collaborator Author

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

Thanks for suggestions.

cli/src/commands/branch/create.rs Outdated Show resolved Hide resolved
cli/src/commands/branch/rename.rs Outdated Show resolved Hide resolved
cli/src/commands/branch/set.rs Show resolved Hide resolved
@yuja yuja enabled auto-merge (rebase) June 28, 2024 10:31
@yuja yuja merged commit af986a5 into martinvonz:main Jun 28, 2024
17 checks passed
@yuja yuja deleted the push-kkwqkqnutwzm branch June 28, 2024 10:36
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