Re: [PATCH] - Updated usage and simplified sub-command action invocation
- From
Imran M Yousuf <imyousuf@gmail.com>
- Date
- Jan 10, 2008, 06:51 UTC
- Message-ID
- <7bfdc29a0801092251p3d46a3cau3db4d57c4f705043@mail.gmail.com>
- In-Reply-To
- <7v8x2y8ahw.fsf@gitster.siamese.dyndns.org>
On Jan 10, 2008 12:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 40 quoted lines
> imyousuf@gmail.com writes: > > > From: Imran M Yousuf <imyousuf@smartitengineering.com> > > > > - manual page of git-submodule and usage mentioned in git-subcommand.sh > > were not same, thus synchronized them. In doing so also had to change the > > way the subcommands were parsed. > > > > - Previous version did not allow commands such as "git-submodule add init > > update". Thus not satisfying the following case - > > > > mkdir g; mkdir f; cd g/ > > touch g.txt; echo "sample text for g.txt" >> ./g.txt; git-init; > > git-add g.txt; git-commit -a -m "First commit on g" > > cd ../f/; ln -s ../g/ init > > git-init; git-submodule add init update; > > git-commit -a -m "With module update" > > mkdir ../test; cd ../test > > git-clone ../f/; cd f > > git-submodule init update; git-submodule update update > > cd ../..; rm -rf ./f/ ./test/ ./g/ > > I find this too verbose with too little information. > > If I am reading you correctly, what you meant was that the way > command parser was structured made subcommand names such as > "init" and "update" reserved words, and it was impossible to use > them as arguments to commands. > > You could have said something like this instead: > > The command parser incorrectly made subcommand names to > git-submodule reserved, refusing them to be used as > parameters to subcommands. For example, > > $ git submodule add init update > > to add a submodule whose (symbolic) name is "init" and > that resides at path "update" was refused. >
I agree that your comment is better than, I will change it accordingly when resubmitting it.
Show 19 quoted lines
> That would have been much cleaner and easier on the reader than > having to decipher what the 20+ command shell script sequence > was doing. > > I do agree that the breakage is worth fixing, though. > > > +# Synopsis of this commands are as follows > > +# git-submodule [--quiet] [-b branch] add <repository> [<path>] > > +# git-submodule [--quiet] [--cached] [status] [--] [<path>...] > > +# git-submodule [--quiet] init [--] [<path>...] > > +# git-submodule [--quiet] update [--] [<path>...] > > I somehow feel that syntactically the original implementation > that allowed subcommand specific options to come before the > subcommand name was a mistake. It may be easier for users that > both "-b branch add" and "add -b branch" are accepted, but I > have to wonder if it would really hurt if we made "-b branch > add" a syntax error. >
I will recode it to have all options except for --quiet (which is inverse of -v or --verbose) be mentioned after the subcommand.
Show 26 quoted lines
> So how about reorganizing the top-level option parser like this: > > while : > do > case $# in 0) break ;; esac > case "$1" in > add | status | init | update) > # we have found subcommand. > command="$1" > shift > break ;; > --) > # end of parameters > shift > break ;; > --quiet) > quiet=1 > ;; > -*) > die "unknown option $1" > esac > shift > done > test -n "$command" || command=$default_command > module_$command "$@" >
Actually module_$command is not possible because only add's module is module_add rest are modules_$command. Thus I would require another if else and that was the original reason for not using it. Instead I should have (and will) used -
case "$1" in
add)
add=1
command="module_$1"
shift
break
;;
init|update|status)
init=1
command="modules_$1"
shift
check_for_terminator "$1"
break
;;Show 40 quoted lines
> And then make individual command implementations responsible for
> parsing their own options (and perhaps the common ones, to allow
> "git submodule add --quiet", but that is optional), like:
>
> module_add () {
> while :
> do
> case $# in 0) break ;; esac
> case "$1" in
> --cached)
> cached=1
> ;;
> -b | --branch)
> shift
> branch="$1"
> test -n "$branch" ||
> die "no branch name after -b?"
> ;;
> --)
> shift
> break
> ;;
> --quiet)
> quiet=1
> ;;
> -*)
> die "unknown option $1"
> esac
> shift
> done
> repo=$1
> path=$2
> ...
> }
>
> In the above illustration I did not bother eliminating cut&paste
> duplication, but there may be a better way to share the piece to
> parse common options across subcommands option parsers and the
> toplevel one.
>As add subcommand does not support --cached it should be considered in -*, just mentioning for your FYI, I got the point of module parsing their own arguments and I am in agreement.
>
I will make the necessary changes and resubmit the patch tomorrow.
Best regards,
-- Imran M Yousuf