| Message ID | 20200928110517.24915-1-avoidr@posteo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-completion.bash: stash-show: add --patch-with-stat | expand |
Robert Karszniewicz <avoidr@posteo.de> writes: > Signed-off-by: Robert Karszniewicz <avoidr@posteo.de> > --- > contrib/completion/git-completion.bash | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 8be4a0316e..d98c731667 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -3016,7 +3016,10 @@ _git_stash () > list,--*) > __gitcomp "--name-status --oneline --patch-with-stat" > ;; > - show,--*|branch,--*) > + show,--*) > + __gitcomp "--patch-with-stat" > + ;; Why is --patch-with-stat so special? Without completion support for "--patch" and "--stat", typing "--<TAB>" after "git stash show" and seeing only "--patch-with-stat" (which has been made obsolete-but-still-kept synonym immediately after the other two were invented in 2005) would make a rather surprising experience to the end users. For "show" alone, it may make a lot of sense to complete "git stash show -<TAB>" and offer "-p". In any case, it might make more sense to do this instead, and then rethink what options make sense to these subcommands of "git stash". I do not think patch-with-stat should be among them. - list,--*) + list,--* | show,--*) __gitcomp "--name-status --oneline --patch-with-stat" Thanks.
Hi Junio, On Mon, Sep 28, 2020 at 11:43:11AM -0700, Junio C Hamano wrote: > In any case, it might make more sense to do this instead, and then > rethink what options make sense to these subcommands of "git stash". > I do not think patch-with-stat should be among them. Perhaps it would make sense to add --patch and --no-patch to $__git_diff_common_options and then use that list for `git stash show` since it's documented that all of the diff options are valid. Thanks, Denton
On Mon, Sep 28, 2020 at 11:43:11AM -0700, Junio C Hamano wrote: > Robert Karszniewicz <avoidr@posteo.de> writes: > > > Signed-off-by: Robert Karszniewicz <avoidr@posteo.de> > > --- > > contrib/completion/git-completion.bash | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > > index 8be4a0316e..d98c731667 100644 > > --- a/contrib/completion/git-completion.bash > > +++ b/contrib/completion/git-completion.bash > > @@ -3016,7 +3016,10 @@ _git_stash () > > list,--*) > > __gitcomp "--name-status --oneline --patch-with-stat" > > ;; > > - show,--*|branch,--*) > > + show,--*) > > + __gitcomp "--patch-with-stat" > > + ;; > > Why is --patch-with-stat so special? > > Without completion support for "--patch" and "--stat", typing > "--<TAB>" after "git stash show" and seeing only "--patch-with-stat" > (which has been made obsolete-but-still-kept synonym immediately > after the other two were invented in 2005) would make a rather Oh, I didn't know --patch-with-stat was obsoleted. It was recently added to `stash-list`, too. I can as well use "--patch --stat". > surprising experience to the end users. For "show" alone, it may > make a lot of sense to complete "git stash show -<TAB>" and offer > "-p". Does git complete short options at all? I only see long options completed. (I'm also very new to bash-completion) > > In any case, it might make more sense to do this instead, and then > rethink what options make sense to these subcommands of "git stash". > I do not think patch-with-stat should be among them. So shall I do a v2 as per your suggestion and replace "--patch-with-stat" with "--patch --stat"? > > - list,--*) > + list,--* | show,--*) > __gitcomp "--name-status --oneline --patch-with-stat" > > Thanks. Thank you.
Robert Karszniewicz <avoidr@posteo.de> writes: >> surprising experience to the end users. For "show" alone, it may >> make a lot of sense to complete "git stash show -<TAB>" and offer >> "-p". > > Does git complete short options at all? I only see long options > completed. (I'm also very new to bash-completion) I wouldn't personally recommend it, but I did see a patch that added support for one short option completion quite recently. As long as "--<TAB>" gets completed to often-used options, among which "--patch" and "--stat" are included, it would be OK not to react to "-<TAB>". >> In any case, it might make more sense to do this instead, and then >> rethink what options make sense to these subcommands of "git stash". >> I do not think patch-with-stat should be among them. > > So shall I do a v2 as per your suggestion and replace > "--patch-with-stat" with "--patch --stat"? I think Denton Liu offered a different suggestion; I didn't look at and compare which direction is the better one myself, but an approach that keeps the number of manually-maintained list of options low is almost always a good approach. Thanks.
On Wed, Sep 30, 2020 at 12:09:09PM -0700, Junio C Hamano wrote: > Robert Karszniewicz <avoidr@posteo.de> writes: > > So shall I do a v2 as per your suggestion and replace > > "--patch-with-stat" with "--patch --stat"? > > I think Denton Liu offered a different suggestion; I didn't look at > and compare which direction is the better one myself, but an > approach that keeps the number of manually-maintained list of > options low is almost always a good approach. Ok, I understand, then. Will take a look at it. Thank you.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 8be4a0316e..d98c731667 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -3016,7 +3016,10 @@ _git_stash () list,--*) __gitcomp "--name-status --oneline --patch-with-stat" ;; - show,--*|branch,--*) + show,--*) + __gitcomp "--patch-with-stat" + ;; + branch,--*) ;; branch,*) if [ $cword -eq 3 ]; then
Signed-off-by: Robert Karszniewicz <avoidr@posteo.de> --- contrib/completion/git-completion.bash | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)