Support external subcommands: rg, diff, git-show (etc.) #1769

th1000s · 2024-07-22T22:20:03Z

Support external subcommands: rg, diff, git-show (etc.)

The possible command line now is:

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

If the entire command line fails to parse because SUBCMD is unknown,
then try (until the next arg fails) parsing only,
and then parse and call SUBCMD.., output is piped into delta.
Other subcommands also take precedence over the diff/git-diff mode
(delta a b, where e.g. a=show and b=HEAD), and any diff call gets
converted into an external subcommand first.

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

And various other git-CMDS: add, blame, checkout, diff, grep, log, reflog and stash.

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

Set calling process directly because delta started it

This info then takes precedence over whatever
start_determining_calling_process_in_thread() finds or rather
doesn't find.
(The simple yet generous SeqCst is used on purpose for the atomic operations.)

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.

dandavison · 2024-07-22T23:35:51Z

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?

th1000s · 2024-09-30T20:48:21Z

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 · 2024-11-05T10:29:21Z

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?

dandavison · 2024-11-05T14:05:53Z

"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 · 2024-11-10T15:47:37Z

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

It fails because of

delta/src/subcommands/generic_subcmd.rs

Line 127 in acb7c12

if grep_cli::resolve_binary("rg").is_err() {

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 BurntSushi/ripgrep#2928 to ask about it and maybe make the docstring clearer.

dandavison · 2024-11-10T16:12:39Z

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.

th1000s · 2024-11-11T23:18:16Z

src/subcommands/mod.rs

+// start subprocesses:
+// diff (fileA, fileB), and generic subcommands
+pub mod diff;
+mod generic_subcmd;


That name is a bit ... generic I think. Ideas?

th1000s · 2024-11-11T23:19:19Z

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.

dandavison · 2024-11-15T14:19:21Z

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 #1908)

dandavison · 2024-11-15T14:24:25Z

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 #1193. Would something like that fit with the work in this branch as a delta doctor subcommand with doctor-specific arguments?

th1000s · 2024-11-16T12:31:48Z

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.

dandavison

I've gone through the code changes now; all looks very nice. Please excuse/ignore the code golf suggestions -- that was just helping me get into the code.

Before we proceed let's revisit the overall argument here. For rg this is clearly an improvement: otherwise to make delta-rg integration ergonomic you have to write a shell wrapper.

Another possibility along the lines of the rg integration might be visualizing difftastic JSON output.

But what's your thinking about the utility of this for git xxx commands? delta integrates with git transparently via git's external pager mechanism, and many shell completion projects exist giving intricate completion over git commands. Is there a clear advantage to being able to invoke git commands prefixed by delta, especially given that it wouldn't have shell completion? Also, if we were to do it, are we sure that we want to inject 9 (currently) git command names into delta's top-level subcommand namespace? Or should it only support delta git xxx? delta diff in particular seems to have potentially confusing semantics given the presence of the delta a b diff. Sorry I've been slow to bring up these hesitations.

dandavison · 2024-11-20T02:57:35Z

src/cli.rs

+                if let Some(subcmd) = &minus_file {
+                    if let Some(arg) = subcmd.to_str() {


This is basically just code golf, done while trying to get into the substance of the PR. Feel free to ignore.

Suggested change

if let Some(subcmd) = &minus_file {

if let Some(arg) = subcmd.to_str() {

if let Some(arg) = minus_file.as_ref().and_then(|p| p.to_str()) {

Let's wait for if-let chains :)

dandavison · 2024-11-20T03:00:19Z

src/cli.rs

+            }
+            Ok(matches) => {
+                // subcommands take precedence over diffs
+                let minus_file = matches.get_one::<PathBuf>("minus_file").map(PathBuf::from);


Is our support for delta file_a file_b going to cause trouble when introducing standard clap subcommands?

Everything clap parses takes precedence, in fact, here intervention is required to "steal" this argument from the command line parser.

dandavison · 2024-11-20T03:04:41Z

src/subcommands/external.rs

+    for (i, arg) in args.iter().enumerate() {
+        let arg = if let Some(arg) = arg.to_str() {
+            arg
+        } else {
+            continue;
+        };


Sorry, more code golf suggestions:

Suggested change

for (i, arg) in args.iter().enumerate() {

let arg = if let Some(arg) = arg.to_str() {

arg

} else {

continue;

};

for (i, arg) in args.iter().filter_map(|a| a.to_str()).enumerate() {

Excellent, Done.

dandavison · 2024-11-20T03:25:07Z

src/subcommands/external.rs

+                        SubCmdKind::Rg
+                    } else {
+                        SubCmdKind::Git(
+                            args[i..]


This constructs Git("git") in the case of delta git show. Is that intentional / does it matter?

Yes, the single .pop() is not enough, fixed.

dandavison · 2024-11-20T03:42:57Z

src/subcommands/external.rs

+                                .to_string(),
+                        )
+                    };
+                    subcmd.extend(args[i + 1..].iter().map(|arg| arg.into()));


More ignorable code golf:

There might be an attractive alternative way to write the above using .split_first()? The best I've come up with is

let invalid_placeholder = OsString::from("?"); let (subsubcmd, rest) = args[i..] .split_first() .unwrap_or((&invalid_placeholder, &[])); let kind = if arg == RG { SubCmdKind::Rg } else { SubCmdKind::Git(subsubcmd.to_string_lossy().to_string()) }; subcmd.extend(rest.iter().cloned());

Done, merged with the above.

dandavison · 2024-11-20T03:48:31Z

src/subcommands/external.rs

+    }
+}
+
+pub fn extract(args: &[OsString], orig_error: Error) -> (ArgMatches, SubCommand) {


A docstring could be good. My attempt:

Suggested change

pub fn extract(args: &[OsString], orig_error: Error) -> (ArgMatches, SubCommand) {

/// Find the first arg that is a registered external subcommand and return a

/// tuple containing:

/// 0. The args prior to that point

/// 1. A SubCommand representing the external subcommand and its subsequent args

pub fn extract(args: &[OsString], orig_error: Error) -> (ArgMatches, SubCommand) {

dandavison · 2024-11-20T03:56:01Z

src/cli.rs

@@ -1283,31 +1274,55 @@ impl Opt {
                Call::Help(help)
            }
            Err(e) => {
-                e.exit();
+                // Calls `e.exit()` if error persists.
+                let (matches, subcmd) = subcommands::extract(args, e);


Is that... praise? It must be, so elegant!

dandavison · 2024-11-20T04:01:49Z

src/main.rs

+        }
+        let mut cmd = cmd.unwrap();
+
+        let cmd_stdout = cmd.stdout.as_mut().expect("Failed to open stdout");


We have a so-far-upheld tradition of using unwrap_or_else(|| panic(...)) because of my personal belief that expect() is confusing as reader and author, but I'd certainly understand if you wished to discontinue that tradition.

dandavison · 2024-11-21T21:46:15Z

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.

th1000s · 2024-11-26T21:59:55Z

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?

dandavison · 2024-11-27T00:21:33Z

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.

th1000s · 2024-11-27T22:52:38Z

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.

dandavison · 2024-11-28T01:42:37Z

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?

th1000s · 2024-11-28T23:00:16Z

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

The possible command line now is: delta <delta-args> [SUBCMD <subcmd-args>] If the entire command line fails to parse because SUBCMD is unknown, then try (until the next arg fails) parsing <delta-args> only, and then parse and call SUBCMD with args, its output is piped into delta. Other subcommands also take precedence over the diff/git-diff mode (`delta a b`, where e.g. a=git and b=show), and any diff call gets converted into an external subcommand first. Available are: delta rg .. => rg --json .. | delta delta a b .. => git diff --no-index a b .. | delta delta git show .. => git <color-on> show .. | delta and all other git-CMDS, of which add -p, blame, checkout -p, diff, grep, log -p, reflog -p, and stash show -p produce a diff. Because --json is automatically added for `delta rg ..`, it avoids the parsing ambiguities of and is easier to type than `rg .. | delta`. The piping is not done by the shell but delta, so the subcommands are child processes of delta.

This info then takes precedence over whatever start_determining_calling_process_in_thread() finds or rather doesn't find. (The simple yet generous SeqCst is used on purpose for the atomic operations.)

dandavison · 2024-11-29T02:30:08Z

Excellent, merged!

th1000s force-pushed the subcmds branch from 3d93723 to 6a3a316 Compare November 5, 2024 10:14

th1000s changed the title ~~Generalize delta subcommands: rg, diff (implicit), show~~ Generalize subcommands: rg, git-show (etc.), diff Nov 5, 2024

th1000s force-pushed the subcmds branch from 6a3a316 to acb7c12 Compare November 5, 2024 10:23

th1000s requested a review from dandavison November 5, 2024 10:29

th1000s force-pushed the subcmds branch from acb7c12 to 846e9c5 Compare November 11, 2024 23:08

th1000s marked this pull request as ready for review November 11, 2024 23:14

th1000s commented Nov 11, 2024

View reviewed changes

th1000s force-pushed the subcmds branch from 846e9c5 to dc623b0 Compare November 12, 2024 23:17

th1000s force-pushed the subcmds branch from dc623b0 to 6929280 Compare November 16, 2024 12:30

th1000s changed the title ~~Generalize subcommands: rg, git-show (etc.), diff~~ Support external subcommands: rg, diff, git-show (etc.) Nov 16, 2024

dandavison reviewed Nov 20, 2024

View reviewed changes

th1000s force-pushed the subcmds branch from 6929280 to 38f0a41 Compare November 26, 2024 21:58

th1000s force-pushed the subcmds branch from 38f0a41 to c23d8e8 Compare November 27, 2024 22:52

th1000s force-pushed the subcmds branch 2 times, most recently from 482affb to 53ae80c Compare November 28, 2024 22:56

th1000s force-pushed the subcmds branch from 53ae80c to e6ee528 Compare November 29, 2024 00:05

th1000s added 2 commits November 29, 2024 01:23

Set calling process directly because delta started it

315d197

This info then takes precedence over whatever start_determining_calling_process_in_thread() finds or rather doesn't find. (The simple yet generous SeqCst is used on purpose for the atomic operations.)

th1000s force-pushed the subcmds branch from e6ee528 to 315d197 Compare November 29, 2024 00:26

dandavison merged commit 31296e7 into dandavison:main Nov 29, 2024
13 checks passed

		if let Some(subcmd) = &minus_file {
		if let Some(arg) = subcmd.to_str() {

	if let Some(subcmd) = &minus_file {
	if let Some(arg) = subcmd.to_str() {
	if let Some(arg) = minus_file.as_ref().and_then(\|p\| p.to_str()) {

-pub fn extract(args: &[OsString], orig_error: Error) -> (ArgMatches, SubCommand) {
+/// Find the first arg that is a registered external subcommand and return a
+/// tuple containing:
+/// 0. The args prior to that point
+/// 1. A SubCommand representing the external subcommand and its subsequent args
+pub fn extract(args: &[OsString], orig_error: Error) -> (ArgMatches, SubCommand) {

Support external subcommands: rg, diff, git-show (etc.) #1769

Support external subcommands: rg, diff, git-show (etc.) #1769

Conversation

th1000s commented Jul 22, 2024 • edited Loading

dandavison commented Jul 22, 2024

th1000s commented Sep 30, 2024

th1000s commented Nov 5, 2024

dandavison commented Nov 5, 2024

dandavison commented Nov 10, 2024

dandavison commented Nov 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th1000s commented Nov 11, 2024

dandavison commented Nov 15, 2024

dandavison commented Nov 15, 2024

th1000s commented Nov 16, 2024

dandavison left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison commented Nov 21, 2024 • edited Loading

th1000s commented Nov 26, 2024

dandavison commented Nov 27, 2024

th1000s commented Nov 27, 2024

dandavison commented Nov 28, 2024 • edited Loading

th1000s commented Nov 28, 2024

dandavison commented Nov 29, 2024

th1000s commented Jul 22, 2024 •

edited

Loading

dandavison left a comment •

edited

Loading

dandavison Nov 20, 2024 •

edited

Loading

dandavison commented Nov 21, 2024 •

edited

Loading

dandavison commented Nov 28, 2024 •

edited

Loading