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
Received on Wed Nov 9 15:25:12 2005