delta icon indicating copy to clipboard operation
delta copied to clipboard

Generalize delta subcommands: rg, diff (implicit), show

Open th1000s opened this issue 1 year ago • 2 comments

The possible command line now is this:

delta <delta-args> [SUBCMD <subcmd-args>]

If the entire command line fails to parse because SUBCMD is unknown, then try parsing <delta-args> only, and on success call SUBCMD and pipe input to delta.

Available are: delta rg .. => rg --json .. | delta delta show .. => git show --color=always .. | delta delta a b .. => diff a b .. | delta

The piping is not done by the shell, but delta, so the subcommands now are child processes of delta


Looking at the Call { Delta, .. } enum from the --help PR, and how the diff sub-command works (both how -@ is needed to add extra args, and how its ouput is fed back into delta when running as a child process) I had the idea of generalizing that.

Deltas rg integration is great, but rg --json .. | delta is a bit clunky, now it is just delta rg ... This also allows using delta without touching any git configs ("try before you edit anything"), as a proof of concept delta show is implemented here.

This is a bit ... creative with the clap command line parser, but it is all quite contained.

th1000s avatar Jul 22 '24 22:07 th1000s

Nice, very interesting!

  • I'm sure you considered this but is supporting delta git $subcommand a possibility also?
  • Are there any parse ambiguity challenges, e.g. if someone tries to diff a file named rg etc?

Some miscellaneous comments about subcommands; not sure how relevant to this PR:

  • there are various existing options that perhaps should be subcommands themselves, such as --show-colors, --show-config, --show-syntax-themes, --show-themes, --parse-ansi. Ought we to consider making those subcommands before 1.0?
  • Would be nice to have auto-generated shell completion for all this (I believe the trend nowadays is for that to be another subcommand, i.e. eval $(delta completion bash) Use https://crates.io/crates/clap_complete?).
  • Also maybe eval $(delta toggle side-by-side) and eval $(delta toggle line-numbers) mutating $DELTA_FEATURES?

dandavison avatar Jul 22 '24 23:07 dandavison

I'm sure you considered this but is supporting delta git $subcommand a possibility also?

I used --color=always, which is not accepted by all subcommands - but by calling git -c color.ui=always .. any subcommand should become callable. Just check if git-$subcommand is in $PATH.

Are there any parse ambiguity challenges, e.g. if someone tries to diff a file named rg etc?

Yes, and in that case I would give priority to the subcommand. By using ./rg this can ways be overwritten.

Some miscellaneous comments about subcommands; not sure how relevant to this PR:

there are various existing options that perhaps should be subcommands themselves, such as --show-colors, --show-config, --show-syntax-themes, --show-themes, --parse-ansi. Ought we to consider making those subcommands before 1.0?

I think these are not used that often, I would leave them as flags (these being located in src/subcommands/ notwithstanding).

Would be nice to have auto-generated shell completion for all this (I believe the trend nowadays is for that to be another subcommand, i.e. eval $(delta completion bash) Use https://crates.io/crates/clap_complete?).

Already present, see --generate-completion :) But that could also become a subcommand like that.

Also maybe eval $(delta toggle side-by-side) and eval $(delta toggle line-numbers) mutating $DELTA_FEATURES?

Sure, should be possible as well.

th1000s avatar Sep 30 '24 20:09 th1000s

I'm sure you considered this but is supporting delta git $subcommand a possibility also?

Now also possible!

However enabling all git-X commands directly would mean checking if "git $X" is a valid git subcommand first (while parsing the command line, repeatedly), so I use a specific list:

pub const SUBCOMMANDS: &[&str] = &[RG, "show", "log", "diff", "grep", "blame", GIT];

Are there any other useful commands? Also, can you check why the rg test fails on macOS?

th1000s avatar Nov 05 '24 10:11 th1000s

"show", "log", "diff", "grep", "blame" Are there any other useful commands?

The ones that are coming to mind are add for git add -p, and reflog and stash for git reflog -p and git stash list -p.

dandavison avatar Nov 05 '24 14:11 dandavison

Also, can you check why the rg test fails on macOS?

It fails because of https://github.com/dandavison/delta/blob/acb7c1294397c8c2ccd1a68c786261ea6dc22dd3/src/subcommands/generic_subcmd.rs#L127

https://docs.rs/grep-cli/latest/grep_cli/fn.resolve_binary.html

On non-Windows, this is a no-op.

This API has always seemed very surprising to me. I've opened https://github.com/BurntSushi/ripgrep/issues/2928 to ask about it and maybe make the docstring clearer.

dandavison avatar Nov 10 '24 15:11 dandavison

I use zsh and have eval "$(delta --generate-completion zsh 2>/dev/null)" in my shell config. It looks like there might be a shell completion challenge relating to this PR: normally, a command line like rg pattern xxx<TAB> completes xxx as a file path. But delta rg pattern xxx<TAB> is not doing that.

dandavison avatar Nov 10 '24 16:11 dandavison

The ones that are coming to mind are add for git add -p, and reflog and stash for git reflog -p and git stash list -p.

Done.

[The rg test fails on macOS fail] because of https://docs.rs/grep-cli/latest/grep_cli/fn.resolve_binary.html

Oh, thanks! Never would have suspected that, re-implemented some of try_resolve_binary() (which is not public!) in an almost-one-liner for the test.

shell completion

Indeed, I have also asked myself that for simpler stuff like writing my own git-foo command, the completion has to embed itself into the existing git completion.

th1000s avatar Nov 11 '24 23:11 th1000s

The ones that are coming to mind are add for git add -p, and reflog and stash for git reflog -p and git stash list -p.

And one more: I recently learned of the existence of checkout -p (from https://github.com/dandavison/delta/issues/1908)

dandavison avatar Nov 15 '24 14:11 dandavison

there are various existing options that perhaps should be subcommands themselves, such as --show-colors, --show-config, --show-syntax-themes, --show-themes, --parse-ansi. Ought we to consider making those subcommands before 1.0?

I think these are not used that often, I would leave them as flags (these being located in src/subcommands/ notwithstanding).

Also maybe eval $(delta toggle side-by-side) and eval $(delta toggle line-numbers) mutating $DELTA_FEATURES?

Sure, should be possible as well.

Just to mention one more -- I'm attracted to adding a "doctor" command that users can run to obtain a standard suite of diagnostics describing their environment, something like the direction started in https://github.com/dandavison/delta/pull/1193. Would something like that fit with the work in this branch as a delta doctor subcommand with doctor-specific arguments?

dandavison avatar Nov 15 '24 14:11 dandavison

checkout -p

Added.

The doctor or other subcommands can be added via clap directly, or --show-color can be become a show-color subcommand if you prefer that. These here are external (renamed it to that!) subcommands which have to side-step clap and pipe their output back into the parent process.

th1000s avatar Nov 16 '24 12:11 th1000s

This is slightly off-topic because I think this would be an internal rather than an external subcommand, but I just found myself doing delta /dev/null path/to/myfile in order to "cat" a file but with delta's hyperlinked line numbers (which I have set up to open in my IDE). So perhaps that suggests delta cat might be another entry in our subcommand namespace, with the green background color disabled, and the minus line number column removed.

dandavison avatar Nov 21 '24 21:11 dandavison

Apart from delta rg, the other commands mostly make trying out delta easier. But yes, they could just as well sit behind git for that. And I am not even against the redundant diff command, because well, a diff is a diff, and a delta is something not-quite-a-diff. But whatever you prefer.

I sometimes try to use delta 0123.patch, which doesn't work, until I remember delta < 0123.patch also does the job. Maybe default to something like that for a single argument?

th1000s avatar Nov 26 '24 21:11 th1000s

Apart from delta rg, the other commands mostly make trying out delta easier. But yes, they could just as well sit behind git for that.

Let's make them sit behind git for now. We can then watch how our subcommand namespace evolves, and of course it would always be possible to revisit the decision in the future.

dandavison avatar Nov 27 '24 00:11 dandavison

Done.

This is untested on Windows, but the process logic is from subcommands/diff.rs so I don't expect surprises so feel free to merge.

th1000s avatar Nov 27 '24 22:11 th1000s

Apologies if the problem is at my end but I think that something might have gone wrong in the last change? When I do delta git show xxx it looks like the show is being popped out of the command, yielding git -c color.ui=always xxx. I'm guessing related to that double "git" thing we were discussing above?

dandavison avatar Nov 28 '24 01:11 dandavison

No, you are perfectly right! Fixed, and not just testing rg but also git now. That was some hasty last-minute refactoring. Refactored that part again, the logic is much cleaner now.

Ah, the test doesn't work because the CI runners do a shallow --depth=1 clone, fix. And a new clippy with Rust v1.83, with a false positive zombie warning...

th1000s avatar Nov 28 '24 23:11 th1000s

Excellent, merged!

dandavison avatar Nov 29 '24 02:11 dandavison

Both git and ripgrep have special behavior when their output is redirected, aimed at creating predictable and parseable output when it's being consumed by a machine. Should we consider not invoking delta at all in that case, instead execing the external command?

dandavison avatar Nov 30 '24 10:11 dandavison

Excellent, merged!

Booo, squash merged again :)

You envision someone doing alias rg='delta rg', which would break on rg foo | rg bar? But then delta also needs the equivalent of --color=always|auto|none, but for terminal detection, so someone still can do delta rg > pretty-result.ansi.txt.

th1000s avatar Dec 02 '24 22:12 th1000s

Booo, squash merged again :)

Oh I'm sorry, that is annoying. The button is usually defaulted to squash since we use it for other delta contributors so it is a bit of an easy mistake for me to make, especially because we only ever squash at work, so my brain sees "Squash and merge" as "This is the green merge button"... But perhaps now I will not forget again.

But then delta also needs the equivalent of --color=always|auto|none, but for terminal detection, so someone still can do delta rg > pretty-result.ansi.txt.

Perhaps, but I think that I might have a good counterargument to that: today, if a user wants to save delta output with ANSI escapes, they have to do git foo | delta > pretty-result.ansi.txt because git itself has tty detection that doesn't even invoke $GIT_PAGER when redirected. And since saving pretty colored output to disk is unusual, I don't think it has to be particularly ergonomic. So I think in fact it might make sense for us to keep a consistent rule that always, with delta, if you want to save pretty output to disk, then pipe explicitly to delta.

For rg, a user would have to supply --json explicitly: rg --json | delta > pretty-result.ansi.txt, or delta rg --json | delta > pretty-result.ansi.txt. So there'd be a bit of abstraction leakage since it kind of requires them to know that delta's rg integration is based on rg --json. But, at the end of the day, I do think "delta output is for humans" is a reasonable project design principle and that people that are saving ANSI escape codes in text files (or parsing them) probably can be trusted to figure out the details?

dandavison avatar Dec 02 '24 23:12 dandavison

Our git workflow uses Gerrit and is heavily based on "stacked PRs" (as it is now called) - so exactly the opposite, and clearly superior ;) No worries, I'll just leave multi-commit PRs as a draft/WIP and do the final merge myself.

Agreed, the direct exec subcommand PR is #1925, without an opt-out. Let's see if someone runs into that and comes up with another argument for --color=always.

th1000s avatar Dec 09 '24 23:12 th1000s