Skip to content

Conversation

bb010g
Copy link
Contributor

@bb010g bb010g commented Oct 7, 2022

Otherwise, stg email send -G '--subject=Latest patch' translates to git send-mail '--subject=Latest' 'patch', and the command errors with

error: `git send-email`: fatal: ambiguous argument 'patch': unknown revision or path not in the working tree.

Also, Zsh completion reuses the current git completion, whatever it is. Ideally, stg -C foo diff -D --bar -D --baz= would complete the same as git -C foo diff --bar --baz=, but it's currently limited to completing the same as git -C foo diff --baz=, so multi-word options won't complete properly (not that they would before). If Zsh provided a companion associative array to opt_args that contained indices for each value represented in opt_args, we could filter to the current word and provide virtually perfect completion, but this will involve submitting a patch to Zsh to enhance both comparguments and _arguments.

bb010g added 2 commits October 6, 2022 21:54
Otherwise, `stg email send -G '--subject=Latest patch'` translates to
`git send-mail '--subject=Latest' 'patch'`, and the command errors with
```
error: `git send-email`: fatal: ambiguous argument 'patch': unknown revision or path not in the working tree.
```
This isn't perfect, but it's better than before.
`stg diff --diff-opts=--diff-algorithm=` will present the correct menu,
and insertion of an option also works correctly.
@bb010g bb010g force-pushed the consistent-option-passthrough branch from 98b8b06 to 5aa13ae Compare October 10, 2022 18:43
@jpgrayson
Copy link
Collaborator

Thank you for this PR, @bb010g.

I am in agreement that for the --git-opts options to stg email send and stg email format it will be better to support a single git option per StGit --git-opts occurrence. This is a good use case to accommodate correctly.

For diff options, the Python implementation allows multiple space-separated options as the value to --diff-opts as well as allowing multiple occurrences of --diff-opts, which is why this was implemented the way it was. This relates to #187.

By only allowing a single git diff option per --diff-opts, there is a compatibility break with StGit 1.x. I am not sure how attached anyone might be to being able to specify multiple space-separated options, which has made me hesitant to make this change. That said, I agree that a 1:1 mapping between a --diff-opts occurrence and a git diff option is conceptually cleaner and solves the problem of supporting values with spaces.

A question I had was whether git diff options were exposed to the same problem you identified with git email send -G '--subject=Latest patch'? I.e. are there any git diff-tree options that may need space-separated values? Here is what I'm seeing:

  • --anchored=<text>
  • --word-diff-regex=<regex>
  • --color-words[=<regex>]
  • -S<string>
  • -G<regex>
  • --relative[=<path>]
  • -I<regex>, --ignore-matching-lines=<regex>
  • --line-prefix=<prefix>
  • --pretty[=<format>]

So yes, a lot of options that where a user may want a space separated value. I'm in agreement that git diff options should also support values with spaces in them. The most straightforward way of doing this is the change you've made to have the 1:1 mapping between StGit -O/--diff-opts occurrences and git diff options. I can imagine other, less straightforward approaches that involve StGit having its own shell-like quoting rules, but I think that path, in addition to being much more complicated to implement, will be worse for users than this small compatibility break with StGit 1.x.

So I am inclined to merge this change.

However, I am also going to do some follow-on changes:

  • Rename --diff-opts to --diff-opt. I will retain --diff-opts as a hidden alias.
  • Rename --git-opts to --git-opt. No alias for this one since it is new to StGit 2.0.
  • Repair the now broken tests.
  • Add tests for --diff-opt options with value containing spaces. E.g. --diff-opt=--line-prefix="x x x ".
  • Repair code formatting.
  • Improve the zsh completions to:
    • Comprehend already-specified --diff-opt and --git-opt values. I.e. so that already used options are excluded from the completions.
    • Only offer completions for git options and not arguments to git diff-tree/git send-email/git format-patch.

@jpgrayson jpgrayson merged commit 9002df7 into stacked-git:master Oct 11, 2022
jpgrayson added a commit that referenced this pull request Oct 11, 2022
The --diff-opts option no longer supports multiple options per occurrence.

Ref: #220
jpgrayson added a commit that referenced this pull request Oct 11, 2022
Since the former --diff-opts option now only supports one git option per
occurrence, the singular --diff-opt naming becomes more appropriate. The
former --diff-opts option name remains supported as a hidden alias to ease
compatibility with older versions of StGit.

For `stg email format` and `stg email send`, the --git-opts option is
renamed to --git-opt for the same reason. However, since these are new
commands for StGit 2.0, the plural naming is not provided as an alias.

This also corrects the zsh completion for --diff-opt to allow multiple
occurrences.

Ref: #220
@bb010g
Copy link
Contributor Author

bb010g commented Oct 11, 2022

Thank you!

If you're looking at making those sorts of changes, renaming --diff-opt to --diff-tree-opt for clarity might be a good idea, or at least referring to it in documentation as <diff-tree-opt>.

Improve the zsh completions to:

  • Comprehend already-specified --diff-opt and --git-opt values. I.e. so that already used options are excluded from the completions.

  • Only offer completions for git options and not arguments to git diff-tree/git send-email/git format-patch.

From what I discovered while writing this patch, I expect both of those to be rather challenging. For the former, you want to accumulate --diff-opt values up to that point using the same parsing strategy as _arguments, but without actually using _arguments (which is hard! _arguments parses via comparguments, which is a C-coded Zsh builtin), and for the latter, you want to somehow inform the behavior of a completion function that you have no knowledge of. These are both points that, to my knowledge, Zsh itself needs improvements on. All that said, best of luck solving these problems.

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