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

FR: Upsert branch (update branch or create if it does not exist) #3584

Closed
Cretezy opened this issue Apr 27, 2024 · 18 comments · May be fixed by #3585
Closed

FR: Upsert branch (update branch or create if it does not exist) #3584

Cretezy opened this issue Apr 27, 2024 · 18 comments · May be fixed by #3585
Labels
good first issue Good for newcomers polish🪒🐃 Make existing features more convenient and more consistent

Comments

@Cretezy
Copy link
Collaborator

Cretezy commented Apr 27, 2024

Is your feature request related to a problem? Please describe.
When working with Git, I need to remember if a branch is already created before updating it.

For example, jj branch set my-branch -r @- gives a Error: No such branch error. I then need to change the command to jj branch create my-branch -r @-

Describe the solution you'd like

An option on jj branch set such as -c/--create to create the branch if it does not exist.

Example: jj branch set my branch -r @- -c

Describe alternatives you've considered

Writing a shell alias for jj branch set $args || jj branch create $args (pseudo-code).

@PhilipMetzger PhilipMetzger added the polish🪒🐃 Make existing features more convenient and more consistent label Apr 27, 2024
@arxanas
Copy link
Collaborator

arxanas commented Apr 27, 2024

I believe we've pretty much achieved consensus on adding --create to jj branch set, just nobody has implemented it yet.

It should be fairly straightforward to add if you want to give it a try (check the existing implementations for set and create; I wouldn't be surprised if set had a specific check that the branch exists first, which you could just omit in this case — that's how set used to function.)

@arxanas arxanas added the good first issue Good for newcomers label Apr 27, 2024
@joyously
Copy link

Why have two separate subcommands (set and create)? Aren't they really the same thing?

@arxanas
Copy link
Collaborator

arxanas commented Apr 28, 2024

@joyously only from an implementation perspective, I think.

  • It's fairly arbitrary that creating a branch happens to be the same as updating a branch. Why should that be the case from the user's perspective?
  • If branches can diverge (that is, a single branch can potentially address multiple commits), then that's especially reason to not consider setting and creating to be the same thing.
    • If we were used to Monotone instead of Git, then we might feel the same way naturally due to its branching approach.
  • Many situations don't use "creating" and "updating" as the same thing. To take the "upsert" example from the title, in traditional SQL, INSERT and UPDATE are distinct operations, and you can't use them interchangeably.
  • A certain class of Git users reports doing typos in their branch setting and not realizing it until much later, so the separation of the commands addresses a real usage problem.

@yuja
Copy link
Collaborator

yuja commented Apr 28, 2024

I think it's okay to deprecate branch create if we add set --create/--allow-new. To me, the verb set can also mean creation, and the typo problem is mostly addressed by requiring --allow flags for unusual movements.

@Cretezy
Copy link
Collaborator Author

Cretezy commented Apr 28, 2024

I agree with making set also create branches by default.

The end result of both commands is the same: a branch exists and is set to a revision. I can't think of any use case where we would want to throw an error when setting a branch that doesn't exist.

We could add a --no-create flag to set which would error when the branch doesn't exist, restoring the behavior of branch create.

Once the we decide on the behavior, I can take up creating the PR.

@arxanas
Copy link
Collaborator

arxanas commented Apr 28, 2024

the typo problem is mostly addressed by requiring --allow flags for unusual movements.

The typo problem is addressed for the case when you've typoed a branch name but accidentally produced another valid branch name (which happens), but it doesn't address the case when you create a new branch entirely unintentionally. (I recall cases earlier in my career where I spent some time trying to figure out, for example, why a push wasn't doing anything, where it turned out that I didn't actually update the branch that I thought I did, although this is somewhat mitigated by better tools for visualization.) (cc @ilyagr who I thought had complained about typos as well?)

I can't think of any use case where we would want to throw an error when setting a branch that doesn't exist.

The most obvious error case is when someone makes a typo in a branch name and they intended to update an existing branch.

I'm opposed to making set the default way to create branches unless

  • we determine who jj is intended to serve:
    • Does it include non-professional users, such as academics and hobbyists? Do they have the same computing idioms as we do?
      • In particular, web interfaces and mobile operating systems have significantly changed the UI landscape over the last 10-15 years, and we should expect trends to continue. There was a time when you could expect most computing users to know what a "file" is, but anecdotal reports from teachers indicate that young students no longer operate on that level of abstraction, probably due to mobile computing idioms.
  • the UI paradigm is natural for jj's intended audience
    • Is the branch-setting behavior really intuitive, or are we just used to Git/Unix tooling being quirky like that?
      • To take an example, does it really make sense that the touch command is also a primary way to create files? Does that resemble how anyone uses the filesystem in a GUI? They are very different; only Unixbrains would consider them the same operation. I think we should re-examine many of those biases, natural to us, before we bring them into jj, which might server a broader audience.
    • Does it resemble some other similar UI metaphor already in common use? Does that metaphor match users' mental models about branching?
      • Possible analogues: shortcuts, labels, starring/pinning, (social) sharing, virtual addressing (PO boxes, phone books), ...
  • the UI paradigm is effective for jj's intended audience
    • Making the invocation shorter (that is, jj set to create) is likely more convenient than having separate commands, so I don't think this criterion is contended.
  • philosophically, we decide how much redundancy is "good" to have in the UI.

(Another solution is to completely drop branches as a core feature, adopt topics or something else, and stop worrying about whether the branch UI makes sense 🤣)

@joyously
Copy link

Another solution is to completely drop branches as a core feature, adopt topics or something else

I'm all for this, but you still have the problem of defining what it is and how it works. If jj has the branch concept for every backend, it should work the same for every backend. The same goes for topics and tags. I think what confuses things is that the log is shown as a graph, so branch means a line of development. Having it as a pointer to a revision is more like a tag or bookmark, so it doesn't match the graph definition. (yes, I understand how Git does the same thing, but the Git branch moves)
Since there is no "current" branch in jj the only use is to label the graph and push to Git.
I personally wouldn't hang the design on the fact that people sometimes mistype a name. There is undo and also can't you rename a branch?

@PhilipMetzger
Copy link
Collaborator

PhilipMetzger commented Apr 28, 2024

My opinion on the whole create/set split is that it generally helps users like myself who tend to litter the branch names with typos. It was really nice when branch set stopped to create seemingly "correct" branches, which were just typos. I'm in general for keeping both jj branch set --create and jj branch create, as they're a good UI. (See a previous discussion here)

I'm also in favor of renaming the current branches concept to bookmarks, as they're totally misleading for long term Git users, and actually accurately describing the current behavior.

@khionu
Copy link
Collaborator

khionu commented Apr 29, 2024

I think there's a good amount of reflection from @arxanas's post, but I think there's also one element missing: just because how people work with computers changes doesn't mean everything should adapt. I think keeping creation and setting separate makes a lot of sense from a correctness standpoint. I'd much more strongly prefer we use --allow-new instead of --create, to encourage creating branches (or topics, or whatever they become) explicitly.

Using branch set --create as a default will lead to people blurring out the fact that they are opening themselves to creating new branches, because they'll mentally erase --create when they think they aren't actually going to be creating. branch set --allow-new doesn't have the risk of seeing --allow-new as not being part of the specific context, as it's adding permission, not changing the nature of the operation, from the user's perspective.

@yuja
Copy link
Collaborator

yuja commented Apr 29, 2024

the typo problem is mostly addressed by requiring --allow flags for unusual movements.

To be clear, I don't mean jj branch set should create new branch by default nor --allow-new/--create should be the default. It's a footgun.

[EDIT] I was wondering if we would add jj tag set/create pair, which seemed redundant because tags usually don't move, but we'll need something like tag set --allow-move to fix up mistake. It might be better to make "set" be a create-or-overwrite-if-allowed command, and rename the current jj branch set to e.g. move, but I'm not sure.

@Cretezy
Copy link
Collaborator Author

Cretezy commented Apr 29, 2024

I've opened a PR to add the create option to set: #3585

I think the best point for this is that adding -c at the end of the previous command is quick if the set command fails due to the branch not existing.

Another alternative could be prompting the user in the CLI to create the branch after set fails.

@ilyagr
Copy link
Collaborator

ilyagr commented May 2, 2024

I'm a little late to the discussion, but we could have jj branch move to move branches (what set does now) and jj branch set to move or create branches.

(Indeed, as Waleed mentioned, I do regularly make typos and care about them)

@Cretezy
Copy link
Collaborator Author

Cretezy commented May 3, 2024

From my usage, it's often that I want to quickly create a branch after seeing it doesn't exist. For example:

$ jj branch set test -r @
Error: No such branch: test
Hint: Use `jj branch create` to create it.
$ jj branch set test -r @ -c

Edit: Additionally, it matches the behaviour of other tools like git switch

@ilyagr
Copy link
Collaborator

ilyagr commented May 3, 2024

That's a good point, -c sounds better in this case.

@yuja
Copy link
Collaborator

yuja commented May 3, 2024

I want to quickly create a branch after seeing it doesn't exist.

jj branch create exactly does that.

(I thought create could be combined into new (create-or-move) set, but maybe I'm wrong.)

@ilyagr
Copy link
Collaborator

ilyagr commented May 3, 2024

I think the point was that adding -c to a command-line is easier than replacing "set" with "create". At least, this would avoid a minor hassle for me.

@yuja
Copy link
Collaborator

yuja commented May 4, 2024

I think the point was that adding -c to a command-line is easier than replacing "set" with "create".

Yeah, I agree, but if the user doesn't want to overwrite an existing branch, he doesn't have to try jj branch set at all. And if he want to do upsert, create-or-move version of set will do that.

yuja added a commit to yuja/jj that referenced this issue Jun 16, 2024
… name

This basically supersedes the current "branch set" command. The plan is to turn
"branch set" into an "upsert" command, and deprecate "branch create". (martinvonz#3584)
Maybe we can also add "branch set --new" flag to only allow creation of new
branches. One reason behind this proposed change is that "set" usually allows
both "creation" and "update". However, we also need a typo-safe version of
"set" to not create new branches by accident.

"jj branch move" is useful when advancing ancestor branches. Let's say you've
added a couple of commits on top of an existing PR branch, you can advance the
branch by "jj branch move --from 'heads(::@- & branches())' --to @-". If this
pattern is super common, maybe we can add --advance flag for short.

One drawback of this change is that "git branch --move" is equivalent to
"jj branch rename". I personally don't find this is confusing, but it's true
that "move" sometimes means "rename".
yuja added a commit to yuja/jj that referenced this issue Jun 17, 2024
… name

This basically supersedes the current "branch set" command. The plan is to turn
"branch set" into an "upsert" command, and deprecate "branch create". (martinvonz#3584)
Maybe we can also add "branch set --new" flag to only allow creation of new
branches. One reason behind this proposed change is that "set" usually allows
both "creation" and "update". However, we also need a typo-safe version of
"set" to not create new branches by accident.

"jj branch move" is useful when advancing ancestor branches. Let's say you've
added a couple of commits on top of an existing PR branch, you can advance the
branch by "jj branch move --from 'heads(::@- & branches())' --to @-". If this
pattern is super common, maybe we can add --advance flag for short.

One drawback of this change is that "git branch --move" is equivalent to
"jj branch rename". I personally don't find this is confusing, but it's true
that "move" sometimes means "rename".
yuja added a commit to yuja/jj that referenced this issue Jun 18, 2024
… name

This basically supersedes the current "branch set" command. The plan is to turn
"branch set" into an "upsert" command, and deprecate "branch create". (martinvonz#3584)
Maybe we can also add "branch set --new" flag to only allow creation of new
branches. One reason behind this proposed change is that "set" usually allows
both "creation" and "update". However, we also need a typo-safe version of
"set" to not create new branches by accident.

"jj branch move" is useful when advancing ancestor branches. Let's say you've
added a couple of commits on top of an existing PR branch, you can advance the
branch by "jj branch move --from 'heads(::@- & branches())' --to @-". If this
pattern is super common, maybe we can add --advance flag for short.

One drawback of this change is that "git branch --move" is equivalent to
"jj branch rename". I personally don't find this is confusing, but it's true
that "move" sometimes means "rename".
yuja added a commit that referenced this issue Jun 18, 2024
… name

This basically supersedes the current "branch set" command. The plan is to turn
"branch set" into an "upsert" command, and deprecate "branch create". (#3584)
Maybe we can also add "branch set --new" flag to only allow creation of new
branches. One reason behind this proposed change is that "set" usually allows
both "creation" and "update". However, we also need a typo-safe version of
"set" to not create new branches by accident.

"jj branch move" is useful when advancing ancestor branches. Let's say you've
added a couple of commits on top of an existing PR branch, you can advance the
branch by "jj branch move --from 'heads(::@- & branches())' --to @-". If this
pattern is super common, maybe we can add --advance flag for short.

One drawback of this change is that "git branch --move" is equivalent to
"jj branch rename". I personally don't find this is confusing, but it's true
that "move" sometimes means "rename".
@yuja
Copy link
Collaborator

yuja commented Jun 18, 2024

I've added jj branch move --from=REV|NAME, which can be used in place of typo-safe jj branch set command #3895, As the next step, I'll probably add jj branch set --allow-new flag.

I was thinking of introducing the following changes, but (1) can be footgun for existing users. So even if we plan to change the default behavior, it's better to keep the current behavior (with deprecation warning) for a moment.

  1. make jj branch set do upsert by default (i.e. --allow-new by default)
  2. (maybe) add jj branch set --new (or --deny-move) option to support "never move, just create" use case
  3. (maybe) deprecate jj branch create

Reasons for (1):

  • set sometimes adds new item (e.g. jj config set)
  • For tag management commands, I feel set/delete is more natural than create/move/delete (because tags are usually immovable.)

Reasons for (2) and (3):

I'm getting less sure about (2) and (3).


Another more controversial idea is:

  1. rename the current jj branch create to jj branch set
  2. add jj branch set --allow-move (or --allow-forward) option to support "upsert" use case

The motivation for the rename is that "create" has stronger implication than "set", so it's difficult to extend the behavior by adding command-line flags. However, we want flags because it's easier to type than replacing command name.


Related discussions:

yuja added a commit to yuja/jj that referenced this issue Jun 20, 2024
Since "set <thing>" often adds a <thing> as needed, it make some sense that
"branch set" does upsert. The added flag helps re-execute a failed "set"
command to create new branch.

I'm hoping to make it the default by migrating existing "branch set" users to
"branch move", but changing the default behavior right now can cause problems.
So, let's add an opt-in flag first.

Closes martinvonz#3584
yuja added a commit to yuja/jj that referenced this issue Jun 21, 2024
Since "set <thing>" often adds a <thing> if not exists, it make some sense
that "branch set" does upsert. The current "branch set" use case is now covered
by "branch move", so it's okay to change the "set" behavior.

If new branch is created by "branch set", status message and hint will be
printed to help migration. The user should be able to undo creation if it was
a mistake.

Closes martinvonz#3584
@yuja yuja closed this as completed in 3c80e34 Jun 23, 2024
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jul 7, 2024
## [0.19.0] - 2024-07-03

### Breaking changes

* In revset aliases, top-level `kind:pattern` expression is now parsed as
  modifier. Surround with parentheses if it should be parsed as string/file
  pattern.

* Dropped support for automatic upgrade of repo formats used by versions before
  0.12.0.

* `jj fix` now defaults to the broader revset `-s reachable(@, mutable())`
  instead of `-s @`.

* Dropped support for deprecated `jj branch delete`/`forget` `--glob` option.

* `jj branch set` now creates new branch if it doesn't exist. Use `jj branch
  move` to ensure that the target branch already exists.
  [#3584](martinvonz/jj#3584)

### Deprecations

* Replacing `-l` shorthand for `--limit` with `-n` in `jj log`, `jj op log`
  and `jj obslog`.

* `jj split --siblings` is deprecated in favor of `jj split --parallel` (to
  match `jj parallelize`).

* A new `jj file` subcommand now replaces several existing uncategorized
  commands, which are deprecated.
  - `jj file show` replaces `jj cat`.
  - `jj file chmod` replaces `jj chmod`.
  - `jj file list` replaces `jj files`.

### New features

* Support background filesystem monitoring via watchman triggers enabled with
  the `core.watchman.register_snapshot_trigger = true` config.

* Show paths to config files when configuration errors occur.

* `jj fix` now supports configuring the default revset for `-s` using the
  `revsets.fix` config.

* The `descendants()` revset function now accepts an optional `depth` argument;
  like the `ancestors()` depth argument, it limits the depth of the set.

* Revset/template aliases now support function overloading.
  [#2966](martinvonz/jj#2966)

* Conflicted files are individually simplified before being materialized.

* The `jj file` subcommand now contains several existing file utilities.
  - `jj file show`, replacing `jj cat`.
  - `jj file chmod` replacing `jj chmod`.
  - `jj file list` replacing `jj files`.

* New command `jj branch move` let you update branches by name pattern or source
  revision.

* New diff option `jj diff --name-only` allows for easier shell scripting.

* In color-words diffs, hunks are now highlighted with underline. See [diff
  colors and styles](docs/config.md#diff-colors-and-styles) for customization.

* `jj git push -c <arg>` can now accept revsets that resolve to multiple
  revisions. This means that `jj git push -c xyz -c abc` is now equivalent to
  `jj git push -c 'all:(xyz | abc)'`.

* `jj prev` and `jj next` have gained a `--conflict` flag which moves you
  to the next conflict in a child commit.

* New command `jj git remote set-url` that sets the url of a git remote.

* Author timestamp is now reset when rewriting discardable commits (empty
  commits with no description) if authored by the current user.
  [#2000](martinvonz/jj#2000)

* `jj commit` now accepts `--reset-author` option to match `jj describe`.

* `jj squash` now accepts a `--keep-emptied` option to keep the source commit.

### Fixed bugs

* `jj git push` now ignores immutable commits when checking whether a
  to-be-pushed commit has conflicts, or has no description / committer / author
  set. [#3029](martinvonz/jj#3029)

* `jj` will look for divergent changes outside the short prefix set even if it
  finds the change id inside the short prefix set.
  [#2476](martinvonz/jj#2476)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers polish🪒🐃 Make existing features more convenient and more consistent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants