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: Enhance aliases to run multiple commands with conditional logic #3673

Open
kuchta opened this issue May 12, 2024 · 15 comments
Open

FR: Enhance aliases to run multiple commands with conditional logic #3673

kuchta opened this issue May 12, 2024 · 15 comments
Labels
enhancement New feature or request

Comments

@kuchta
Copy link

kuchta commented May 12, 2024

If aliases would be able to run multiple commands with conditional logic, many workflow specific commands (porcelain in git jargon) could be implemented by the user or his/her company.

Many current commands expect some idiomatic workflow which could be implemented as a default aliases if aliases allow for conditional logic and new command eg. is-same-revision <revision> <revision>

This change would externalize such workflow specific commands (and discussions) to user configurable defaults and be potentionaly replaced by user/company preferred workflow...

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label May 16, 2024
@PhilipMetzger
Copy link
Collaborator

This FR is probably a subset of #3262, as it is along the static/dynamic configuration discussion, which has been brought up many times in Discord.

@fowles
Copy link
Collaborator

fowles commented Jun 3, 2024

I actually just ran across this myself. I would very much like an alias that lets me do jj branch set main -r @- && jj git push -b main. Bonus points for allowing it to take the branch name as an argument or to figure it out from the log.

@matts1
Copy link
Collaborator

matts1 commented Jun 14, 2024

This feels somewhat related to #3575 and #3577.

I think we could do it with minimal changes to the templating language. We'd just have to swap out the builtins for a few different ones, and require the user to create a template returning a List<List<String>>. For example, I could write jj upload as:

[template-aliases]
'stack(x)' = 'surround("mutable() && ::(", ")", x)'

[aliases]
upload = {
   args = {
         "fix": {
             "short": "-f",
             "long": "--fix",
             "type": "bool"
         },
         "lint": {
             "short": "-l",
             "long": "--lint",
             "type": "bool"
         },
         "revision": {
             "short": "-r",
             "long": "--revision",
             "type": "revset"
             "default": "@"
         }
   },
   "positional": True,
   commands = '
      [
          if(args.fix,  ["fix", "-r", stack("opts.revision)]),
          if(args.lint, ["lint", "-r", stack(opts.revision)])
          ["git", "push", "-r", opts.revision] ++ positional
      ]
   '
}

What do people think of this? I personally have a lot of use-cases that I'd be able to solve with something like this.

@yuja
Copy link
Collaborator

yuja commented Jun 15, 2024

[template-aliases]
'stack(x)' = 'surround("mutable() && ::(", ")", x)'

[aliases]
upload = {
   args = {
         "fix": {
             "short": "-f",
             "long": "--fix",
             "type": "bool"
         },
         "lint": {
             "short": "-l",
             "long": "--lint",
             "type": "bool"
         },
         "revision": {
             "short": "-r",
             "long": "--revision",
             "type": "revset"
             "default": "@"
         }
   },
   "positional": True,
   commands = '
      [
          if(args.fix,  ["fix", "-r", stack("opts.revision)]),
          if(args.lint, ["lint", "-r", stack(opts.revision)])
          ["git", "push", "-r", opts.revision] ++ positional
      ]
   '
}

My gut feeling is that this is too much as a command alias system (and I would probably want to use general-purpose language than template DSL at this complexity.) That said, I think it's not uncommon to chain commands conditionally (like foo && bar || baz), which can't be expressed well as a list of commands.

@matts1
Copy link
Collaborator

matts1 commented Jun 17, 2024

Yeah, I do agree that it does seem to be pushing the boundaries on what I'd feel comfortable using it for.

I think that extensions could definitely solve this use case pretty well, but they have the opposite problem. A general-purpose language can call the jj api, but it's really overkill for what's essentially a simple string transformation.

My personal opinion is that:

  • Aliases should have more power than they currently do
  • More complex things should be solved with extensions
  • We need to decide how much power is right to give aliases, and at what point we decide "this is sufficiently complex that an extension is required"

How about this? This seems far more simple, inspired by #3873

[aliases]
'upload()' = ["upload", "@"]
'upload(r, *args)' = [
    ["fix", "-r", "$r"],
    # What does "-r" do?
    ["lint", "-r", "$r"],
    # I know the *args isn't valid syntax, but I don't know what the right syntax would be
    ["git", "push", "-r", "$r", "$args"],
]

Alternatively, we could try something a little more python-like

[aliases]
'upload(revision="@", *args)' = [
    ["fix", "-r", "$revision"],
    ["lint", "-r", "$revision"],
    ["git", "push", "-r", "$revision", "$args"]
]

I'm personally thinking that foo && bar || baz may be better to put out of scope, and instead put that in the "requires you to write an extension" category. I think that this adds a lot of power, while keeping the congnitive overhead nice and low, and the syntax simple.

@yuja
Copy link
Collaborator

yuja commented Jun 17, 2024

My personal opinion is that:
Aliases should have more power than they currently do

  • More complex things should be solved with extensions
  • We need to decide how much power is right to give aliases, and at what point we decide "this is sufficiently complex that an extension is required"

Totally agree.

[aliases]
'upload()' = ["upload", "@"]
'upload(r, *args)' = [
    ["fix", "-r", "$r"],
    # What does "-r" do?
    ["lint", "-r", "$r"],
    # I know the *args isn't valid syntax, but I don't know what the right syntax would be
    ["git", "push", "-r", "$r", "$args"],
]

It looks much manageable (lack of --flag parsing will make things simpler), and I like it.

BTW, the current command aliases are expanded in a loop until converge. I think it's similar to C preprocessor. I'm not sure if the proposed aliases syntax can be compatible with the current substitution logic, and if we make it be evaluated like stacked function call, some user aliases might have to be adjusted.

I'm personally thinking that foo && bar || baz may be better to put out of scope, and instead put that in the "requires you to write an extension" category.

I think we'll at least need to support foo && bar && baz. Perhaps, the commands list will be executed in that way?

@matts1
Copy link
Collaborator

matts1 commented Jun 17, 2024

BTW, the current command aliases are expanded in a loop until converge. I think it's similar to C preprocessor. I'm not sure if the proposed aliases syntax can be compatible with the current substitution logic, and if we make it be evaluated like stacked function call, some user aliases might have to be adjusted.

I think it should be compatible. If you don't use parentheses in the alias name, it would expand to name(*args) = [[..., "$args"]]. For example, foo = ["bar", "baz"] would be expanded to 'foo(*args) = [["bar", "baz", "$args"]].

Then for the substitutions, instead of {"foo": ["bar", "baz"]}, we would have {"upload": {0: [[Format{"upload"}, Format{"@"}]], 1..: [[Format{"fix"}, Format{"-r"}, Format{"%s", 1}], ...]}

I think we'll at least need to support foo && bar && baz ... Perhaps, the commands list will be executed in that way?

Yes, that was my assumption on how they would be executed.

Also sorry, I accidentally clicked "edit" on your comment instead of "quote reply". It should be restored now.

@yuja
Copy link
Collaborator

yuja commented Jun 18, 2024

I think it should be compatible. If you don't use parentheses in the alias name, it would expand to name(*args) = [[..., "$args"]]. For example, foo = ["bar", "baz"] would be expanded to 'foo(*args) = [["bar", "baz", "$args"]].

Yeah, I just thought there might be a subtle behavior difference, but I don't have any particular example in mind.

With these aliases:

[aliases]
foo = ["--color", "always", "baz"]
baz = []

in the current cpp-like substitution logic, jj -R repo foo log will be substituted as follows:

-R repo foo log
# subcommand([-R repo foo log]) => foo
# subst(foo)
-R repo --color always baz log
# subcommand([-R repo --color always baz log]) => baz
# subst(baz)
-R repo --color always log
# subcommand([-R repo --color always log]) => log
# return

If it's stacked like normal function calls:

-R repo foo log
# subcommand([-R repo foo log]) => foo
# subst(foo, [log])
        --color always baz log
        # subcommand([--color always baz log]) => baz
        # subst(baz, [log])
                       log
                       # subcommand([log]) => log
                       # return [log]
        # return [--color always log]
# return [-R repo --color always log]

(for this example, the result is the same)

@matts1
Copy link
Collaborator

matts1 commented Jun 18, 2024

I've started working on a prototype, I'll try and send it out this week. It doesn't look too difficult.

@matts1
Copy link
Collaborator

matts1 commented Jun 26, 2024

Ok, it turned out to be more effort than I thought, but I learnt a lot, and had some ideas to improve it.

I was thinking of using the templating language to solve this. For example:

'upload(r, *args)' = """
    # We need support for lists as a first-class citizen of the templating language. At the moment, you can use lists, but you can't define them.
    command(["fix", "-r", r])
        .then(|r| ["lint", "--revision=" ++ r.commit_id])
        # We'd have to allow ++ to work on lists, or add some way to join two lists together
        .then(|| ["git", "push", "-r", r.commit_id] ++ args])
"""

This would allow us to solve two things:

Firstly, we could chain foo && bar || baz

'x()' = """
    command(["foo"]).then(|| ["bar"]).or(["baz"])
"""

Secondly, we can pipe the output of previous commands onto the current command. For example, duplicate would "return" the new commit:

'duplicate_onto_head(r)' = """
    command(["duplicate", "-r", r])
        .then(|duplicate| ["rebase", "-s", duplicate.commit_id, "-d", "@"])
""" 

What do people think of this? It seems to add some complexity, but it feels to me like it strikes the right balance between complexity and power. I also think that although it adds some complexity, using explicit functions instead of just lists of lists makes it more obvious.

@martinvonz
Copy link
Owner

I like your earlier proposal better because it was relatively simple. I'm a bit worried that if we try to add something like your most recent proposal, it's going to be a slippery slope to writing our own shell script language.

@matts1
Copy link
Collaborator

matts1 commented Jun 27, 2024

That seems pretty fair to me (it also simplifies implementation a lot). The question I have left is how we want to do variable formatting. My current thoughts are:

  • Similar to python format strings:
    • "{foo}" for the variable foo
    • "{{foo}}" for the literal "{foo}"
    • "--foo={foo}" would be valid
  • Suppose we had 'command(*args)' = ..., and we ran jj command a b c:
    • Do we allow --foo={args} (["foo", "--foo={args}"] -> ["foo", "--foo=a", "--foo=b", "--foo=c"])
    • Do we allow {args} when not at the end (["foo", "{args}", "bar"] -> ["foo", "a", "b", "c", "bar"])?

@yuja
Copy link
Collaborator

yuja commented Jun 27, 2024

  • Suppose we had 'command(*args)' = ..., and we ran jj command a b c:
    • Do we allow --foo={args} (["foo", "--foo={args}"] -> ["foo", "--foo=a", "--foo=b", "--foo=c"])
    • Do we allow {args} when not at the end (["foo", "{args}", "bar"] -> ["foo", "a", "b", "c", "bar"])?

If we don't need the former, I'll just special case "$args" (just like "$@" in sh.)

  • "$args" -> "$args[0]", "$args[1]", ..
  • "foo=$args" -> error?

BTW, I prefer $x than {x} because we already use dollar sign in merge-tools configuration.

@PhilipMetzger
Copy link
Collaborator

I also dislike the new proposal and also want to have a slimmed down syntax. But your new proposal probably belongs to the conversation in #3262.

matts1 added a commit to matts1/jj that referenced this issue Jul 3, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Which will require aliases to map to `Vec<Vec<String>>`
matts1 added a commit to matts1/jj that referenced this issue Jul 3, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Which will require aliases to map to `Vec<Vec<String>>`
matts1 added a commit to matts1/jj that referenced this issue Jul 3, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Which will require aliases to map to `Vec<Vec<String>>`
matts1 added a commit to matts1/jj that referenced this issue Oct 4, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Template aliases:
1) Start as Config::Value
2) Are converted to String
3) Are placed in the alias map
4) Expand to a TemplateExpression type via expand_defn.

However, command aliases:
1) Start as Config::Value
2) Are converted to Vec<Vec<String>>
3) Are placed in an alias map
4) Do not expand

Thus, AliasesMap will need to support non-string values.
matts1 added a commit to matts1/jj that referenced this issue Oct 4, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Template aliases:
1) Start as Config::Value
2) Are converted to String
3) Are placed in the alias map
4) Expand to a TemplateExpression type via expand_defn.

However, command aliases:
1) Start as Config::Value
2) Are converted to Vec<Vec<String>>
3) Are placed in an alias map
4) Do not expand

Thus, AliasesMap will need to support non-string values.
matts1 added a commit to matts1/jj that referenced this issue Oct 4, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Template aliases:
1) Start as Config::Value
2) Are converted to String
3) Are placed in the alias map
4) Expand to a TemplateExpression type via expand_defn.

However, command aliases:
1) Start as Config::Value
2) Are converted to Vec<Vec<String>>
3) Are placed in an alias map
4) Do not expand

Thus, AliasesMap will need to support non-string values.
matts1 added a commit to matts1/jj that referenced this issue Oct 7, 2024
For martinvonz#3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Template aliases:
1) Start as Config::Value
2) Are converted to String
3) Are placed in the alias map
4) Expand to a TemplateExpression type via expand_defn.

However, command aliases:
1) Start as Config::Value
2) Are converted to Vec<Vec<String>>
3) Are placed in an alias map
4) Do not expand

Thus, AliasesMap will need to support non-string values.
matts1 added a commit that referenced this issue Oct 7, 2024
For #3673, we will have aliases such as:
```toml
'upload(revision)' = [
  ["fix", "-r", "$revision"],
  ["lint", "-r", "$revision"],
  ["git", "push", "-r", "$revision"],
]
```

Template aliases:
1) Start as Config::Value
2) Are converted to String
3) Are placed in the alias map
4) Expand to a TemplateExpression type via expand_defn.

However, command aliases:
1) Start as Config::Value
2) Are converted to Vec<Vec<String>>
3) Are placed in an alias map
4) Do not expand

Thus, AliasesMap will need to support non-string values.
@senekor
Copy link
Collaborator

senekor commented Nov 4, 2024

What is the benefit of this approach over the much simpler idea of jj util exec? The discussion here is already pushing against too much complexity, so whatever becomes the agreed-upon tradeoff between complexity and flexibility is going to be much less powerfull than simply shelling out to a user-provided program / script.

jj util exec was dicussed in #3001 and is already implemented in #4759.

I'm tried to check the discussion if my question was already discussed, this is the closest I found:

A general-purpose language can call the jj api, but it's really overkill for what's essentially a simple string transformation.

I don't quite follow this. People who are using jj are already using a shell language that they are familiar with. Telling them to just write that same language for more complicated aliases seems much simpler than inventing some new systems that users have to learn and maintainers have to support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants