You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Fabien COELHO <fa...@coelho.net> on 2005/11/08 13:11:27 UTC
[PATCH] improved bash completion v1
Please find attached a patch which helps to avoid typos in property names.
MESSAGE
Improve svn command bash auto-completion wrt property subcommands
- property names are completed
- revision properties are suggested --revprop and --revision ...
- possible values are suggested for some properties
- user-defined file and revision properties can be added
with environment variables SVN_FILE_PROPS and SVN_REV_PROPS
FILES
M tools/client-side/bash_completion
--
Fabien.
Re: [PATCH] improved bash completion v1
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 9 Nov 2005, Fabien COELHO wrote:
>
> Dear Julian,
>
> > Please read the guidelines for log messages at
> > <file:///home/julianfoad/src/subversion/www/hacking.html#log-messages>.
>
> Ok. The only issue is that I don't have access to your account;-)
> I guess it is on the web.
>
http://subversion.tigris.org/hacking.html
(Maybe you knew this, but I wasn't sure.)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improved bash completion v1
Posted by Fabien COELHO <fa...@coelho.net>.
Dear Julian,
> Please read the guidelines for log messages at
> <file:///home/julianfoad/src/subversion/www/hacking.html#log-messages>.
Ok. The only issue is that I don't have access to your account;-)
I guess it is on the web.
>> + # drop ":" separator to handle svn:* property names properly?
>
> "?" ? I assume you're asking reviewers whether this is a good idea.
> I don't know the answer.
I know it, it is that it *is* needed.
>> + # this seems to break file completion within file:// urls
>
> That's a pity.
Indeed.
Maybe it is a possible to go around that, but I have not found a way.
Hence the following snip I let, if someone had an idea...
>> + # file: and other url completion?
>> + #if [[ $cur == "file:*" ]] ; then
>> + # COMP_WORDBREAKS="$COMP_WORDBREAKS:"
>> + # return 0
>> + #fi
>
> Please try not to leave half-baked ideas in a patch submission.
> Either take out bits like this.
Indeed half-backed.
>> + if [[ $COMP_CWORD == 2 && $isPropCmd ]]
>> + then
>
> This assumes the property name is given immediately after the subcommand, but
> options are often given here instead.
Yes. svn help pset says:
usage: 1. propset PROPNAME [PROPVAL | -F VALFILE] PATH...
2. propset PROPNAME --revprop -r REV [PROPVAL | -F VALFILE] [URL]
So I'm coherent with the suggested syntax.
Also the svn commands allows options to be provided options just after
svn, the completion nevertheless only suggest subcommands there.
If you follow what is suggested by the completion the property name is
proposed just after the subcommand, so it fits.
Otherwise a full analysis of the command would be required and that would
not improve the code, although it would be possible.
It also helps because only prop names are suggested after the subcommands,
and then only options, otherwise we would have to suggest a mix.
Once the name is provided, you know whether it is a revision property so
that --revprop and --revision are mandatory and not, so it helps there
to.
>> + # possible completion values for properties
>> + local values="\' --file"
>> + if [[ $isPropCmd ]] ; then
>> + case ${COMP_WORDS[2]} in
>> + svn:keywords)
>> + values="Id Rev URL Date Author \'"
>
> If the idea of that single quote is to suggest to the user that they may wish
> to type a shell quoting character to start a list of keywords, I don't think
> that is the sort of thing completion should be doing.
Well, you don't have to chose it;-) Otherwise the completion is limited to
one keyword, where the syntax really expects any number of them, so I was
not happy with the one-keyword only completions.
>> + ;;
>> + svn:executable|svn:needs-lock)
>> + values='1'
>
> The canonical value for these is "*", not "1".
I know that, but * in a shell has other meanings... As it is just a
defined/not defined issue, I thought that a non ambiguous 1 would fit.
Or I can suggest something "'*'", but 1 looks better to me.
>> + svn:mime-type) + values='text/plain text/html text/xml text/rtf text/ \
>> + image/png image/gif image/jpeg image/tiff image/ \
>> + audio/ video/ application/'
>
> For the last three, Bash appends a space to the completion. Can we stop
> it doing that? It must be possible, because it doesn't append a space
> after a directory name. For "text/" and "image/" it doesn't, because
> there are alternative completions.
Yes. For directories, this may be because there are several files within
the directory? I can look into it.
> So one crude way would be to make sure there is at least one
> alternative completion in each category. "application/octet-stream" is an
> obvious one.
Yes.
>> + if [[ $isRevProp && $COMP_CWORD == 6 &&
>> + ${COMP_WORDS[1]} == @($psCmds) &&
>> + ${COMP_WORDS[3]} = '--revprop' &&
>> + ${COMP_WORDS[4]} = '--revision' ]]
>
> It looks like this makes far too many assumptions about the order in which
> the user specifies the options and parameters. It really needs to be more
> flexible if it is to be useful to people other than yourself.
It is the order the autocompletion suggests the stuff, and the order which
is described in the documentation. Also, I do not want to have to mix
options, property names or property values together, so I direct the
syntax. Maybe I can be less directive, but that needs more coding.
>> propdel|pdel|pd)
>> - cmdOpts="$qOpts -R --recursive $rOpts --revprop $pOpts"
>> + cmdOpts="$qOpts -R --recursive $rOpts $pOpts"
>> + [[ $isRevProp ]] && cmdOpts="$cmdOpts --revprop"
>
> This means "--revprop" is not suggested until AFTER the user has typed the
> property name. I usually put it BEFORE the property name. Is there any harm
> in it being allowed before the user types a property name?
Nearly everything is possible, just more cases to handle, and it is only
shell script;-) As mentionned above I chose to stick to the syntax
described by svn help.
> I think what you intended is that if the user types the name of a
> non-revision property, then you don't want "--revprop" to be suggested.
Yes.
> Perhaps you could rewrite this bit of code to do exactly that.
I can do something more general with more time;-)
>> + [[ $prev == @($optsParam) ]] && { prev=$opt ; continue ; }
>
> This sets "prev" to the value of the parameter, which could be anything,
Yes. I should have put prev=''.
> So, in conclusion, I like the idea but it needs a bit more work.
And a bit more time;-) I'm not sure I'll have time for all of your
suggestions, but I will have time for some, over the week-end.
If someone wants to improve over after that, fine with me;-)
Thanks for all your precise and motivated comments and debugging.
I'll resubmit later.
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] improved bash completion v1
Posted by Julian Foad <ju...@btopenworld.com>.
Fabien COELHO wrote:
>
> Please find attached a patch which helps to avoid typos in property names.
I like the idea.
> MESSAGE
>
> Improve svn command bash auto-completion wrt property subcommands
> - property names are completed
> - revision properties are suggested --revprop and --revision ...
> - possible values are suggested for some properties
> - user-defined file and revision properties can be added
> with environment variables SVN_FILE_PROPS and SVN_REV_PROPS
Please read the guidelines for log messages at
<file:///home/julianfoad/src/subversion/www/hacking.html#log-messages>.
> Index: tools/client-side/bash_completion
> ===================================================================
> --- tools/client-side/bash_completion (revision 17245)
> +++ tools/client-side/bash_completion (working copy)
> @@ -10,12 +10,17 @@
>
> shopt -s extglob
>
> +# env SVN_FILE_PROPS: other properties on files
> +# env SVN_REV_PROPS: other properties on revisions
> _svn()
> {
> local cur cmds cmdOpts pOpts mOpts rOpts qOpts nOpts optsParam opt
> local helpCmds optBase i
>
> COMPREPLY=()
> + # drop ":" separator to handle svn:* property names properly?
"?" ? I assume you're asking reviewers whether this is a good idea. I don't
know the answer.
> + # this seems to break file completion within file:// urls
That's a pity.
> + COMP_WORDBREAKS=${COMP_WORDBREAKS/:/}
> cur=${COMP_WORDS[COMP_CWORD]}
> + # some special partial help about --revision...
> + if [[ ${COMP_WORDS[COMP_CWORD-1]} = '--revision' ]] ; then
> + COMPREPLY=( $( compgen -W "HEAD BASE PREV COMMITTED 0 {" -- $cur ) )
> + return 0
> + fi
> +
> + # what about --author and others?
> +
> + # file: and other url completion?
> + #if [[ $cur == "file:*" ]] ; then
> + # COMP_WORDBREAKS="$COMP_WORDBREAKS:"
> + # return 0
> + #fi
Please try not to leave half-baked ideas in a patch submission. Either take
out bits like this, or say "I'd like comments on the ideas in this patch; it's
not ready for committing."
> @@ -38,8 +58,97 @@
> |--diff-cmd|--diff3-cmd|--editor-cmd|--old|--new|
> |--config-dir|--native-eol|--limit"
>
> - # if not typing an option, or if the previous option required a
> - # parameter, then fallback on ordinary filename expansion
> + # svn:* and other (env SVN_FILE_PROPS and SVN_REV_PROPS) properties
> + local svnProps revProps allProps psCmds propCmds
> +
> + # svn file properties
> + svnProps="svn:keywords svn:executable svn:needs-lock svn:externals"
> + svnProps="svn:ignore svn:eol-style svn:mime-type $svnProps"
> + [ "$SVN_FILE_PROPS" ] && svnProps="$svnProps $SVN_FILE_PROPS"
> +
> + # svn revision properties
> + revProps="svn:author svn:log svn:date"
> + [ "$SVN_REV_PROPS" ] && revProps="$revProps $SVN_REV_PROPS"
> +
> + # all properties
> + allProps="$svnProps $revProps"
> +
> + # subcommands that expect property names
> + psCmds='propset|pset|ps'
> + propCmds="$psCmds|propget|pget|pg|propedit|pedit|pe|propdel|pdel|pd"
> +
> + local isPropCmd=
> + [[ ${COMP_WORDS[1]} == @($propCmds) ]] && isPropCmd=1
> +
> + # provide allowed property names after property commands
> + if [[ $COMP_CWORD == 2 && $isPropCmd ]]
> + then
This assumes the property name is given immediately after the subcommand, but
options are often given here instead.
> + COMPREPLY=( $( compgen -W "$allProps" -- $cur ) )
> + return 0
> + fi
> +
> + # is it a revision property?
> + local isRevProp=
> + [[ $isPropCmd && ${COMP_WORDS[2]} == @(${revProps// /|}) ]] && \
> + isRevProp=1
> +
> + # possible completion values for properties
> + local values="\' --file"
> + if [[ $isPropCmd ]] ; then
> + case ${COMP_WORDS[2]} in
> + svn:keywords)
> + values="Id Rev URL Date Author \'"
If the idea of that single quote is to suggest to the user that they may wish
to type a shell quoting character to start a list of keywords, I don't think
that is the sort of thing completion should be doing.
> + ;;
> + svn:executable|svn:needs-lock)
> + values='1'
The canonical value for these is "*", not "1".
> + ;;
> + svn:eol-style)
> + values='native LF CR CRLF'
> + ;;
> + svn:mime-type)
> + values='text/plain text/html text/xml text/rtf text/ \
> + image/png image/gif image/jpeg image/tiff image/ \
> + audio/ video/ application/'
For the last three, Bash appends a space to the completion. Can we stop it
doing that? It must be possible, because it doesn't append a space after a
directory name.
For "text/" and "image/" it doesn't, because there are alternative completions.
So one crude way would be to make sure there is at least one alternative
completion in each category. "application/octet-stream" is an obvious one.
> + ;;
> + esac
> + fi
> +
> + # force --revprop option after revision properties
> + if [[ $COMP_CWORD == 3 && $isRevProp ]]
> + then
> + COMPREPLY=( $( compgen -W '--revprop' -- $cur ) )
> + return 0
> + fi
> +
> + # force --revision option after --revprop on revision properties
> + if [[ $COMP_CWORD == 4 && $isRevProp &&
> + ${COMP_WORDS[3]} = '--revprop' ]]
> + then
> + COMPREPLY=( $( compgen -W '--revision' -- $cur ) )
> + return 0
> + fi
> +
> + # handle pset values for revprops
> + if [[ $isRevProp && $COMP_CWORD == 6 &&
> + ${COMP_WORDS[1]} == @($psCmds) &&
> + ${COMP_WORDS[3]} = '--revprop' &&
> + ${COMP_WORDS[4]} = '--revision' ]]
It looks like this makes far too many assumptions about the order in which the
user specifies the options and parameters. It really needs to be more flexible
if it is to be useful to people other than yourself.
> + then
> + COMPREPLY=( $( compgen -W "$values" -- $cur ) )
> + return 0
> + fi
> +
> + # handle pset values for non rev props
> + if [[ $COMP_CWORD == 3 && ! $isRevProp &&
> + ${COMP_WORDS[1]} == @($psCmds) ]]
> + then
> + COMPREPLY=( $( compgen -W "$values" -- $cur ) )
> + return 0
> + fi
> +
> + # if not typing an option,
> + # or if the previous option required a parameter,
> + # then fallback on ordinary filename expansion
> helpCmds='help|--help|h|\?'
> if [[ ${COMP_WORDS[1]} != @($helpCmds) ]] && \
> [[ "$cur" != -* ]] || \
> @@ -126,22 +235,25 @@
> cmdOpts="$mOpts $rOpts $qOpts --force --editor-cmd $pOpts"
> ;;
> propdel|pdel|pd)
> - cmdOpts="$qOpts -R --recursive $rOpts --revprop $pOpts"
> + cmdOpts="$qOpts -R --recursive $rOpts $pOpts"
> + [[ $isRevProp ]] && cmdOpts="$cmdOpts --revprop"
This means "--revprop" is not suggested until AFTER the user has typed the
property name. I usually put it BEFORE the property name. Is there any harm
in it being allowed before the user types a property name?
I think what you intended is that if the user types the name of a non-revision
property, then you don't want "--revprop" to be suggested. Perhaps you could
rewrite this bit of code to do exactly that.
> ;;
> propedit|pedit|pe)
> - cmdOpts="$rOpts --revprop --encoding --editor-cmd $pOpts \
> - --force"
> + cmdOpts="$rOpts --encoding --editor-cmd $pOpts --force"
> + [[ $isRevProp ]] && cmdOpts="$cmdOpts --revprop"
> ;;
> propget|pget|pg)
> - cmdOpts="-R --recursive $rOpts --revprop --strict $pOpts"
> + cmdOpts="-R --recursive $rOpts --strict $pOpts"
> + [[ $isRevProp ]] && cmdOpts="$cmdOpts --revprop"
> ;;
> proplist|plist|pl)
> cmdOpts="-v --verbose -R --recursive $rOpts --revprop $qOpts \
> $pOpts"
> ;;
> propset|pset|ps)
> - cmdOpts="-F --file $qOpts --targets -R --recursive --revprop \
> + cmdOpts="-F --file $qOpts --targets -R --recursive \
> --encoding $pOpts $rOpts --force"
> + [[ $isRevProp ]] && cmdOpts="$cmdOpts --revprop"
> ;;
> resolved)
> cmdOpts="--targets -R --recursive $qOpts"
> @@ -168,11 +280,16 @@
> esac
>
> cmdOpts="$cmdOpts --help -h --config-dir"
> + local prev=''
>
> # take out options already given
> - for (( i=2; i<=$COMP_CWORD-1; ++i )) ; do
> - opt=${COMP_WORDS[$i]}
> + for opt in $COMP_WORDS
> + do
> + # skip if the previous option required a parameter
> + [[ $prev == @($optsParam) ]] && { prev=$opt ; continue ; }
This sets "prev" to the value of the parameter, which could be anything,
including something that looks like a Subversion option but isn't. For
example, the parameter to "-x" will typically look like a Subversion option.
So set "prev" to the empty string instead.
> + prev=$opt
>
> + # remove leading dashes and arguments
> case $opt in
> --*) optBase=${opt/=*/} ;;
> -*) optBase=${opt:0:2} ;;
> @@ -207,11 +324,6 @@
> cmdOpts=${cmdOpts/ -F / }
> ;;
> esac
> -
> - # skip next option if this one requires a parameter
> - if [[ $opt == @($optsParam) ]] ; then
> - ((++i))
> - fi
> done
>
> COMPREPLY=( $( compgen -W "$cmdOpts" -- $cur ) )
So, in conclusion, I like the idea but it needs a bit more work.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org