Re: [PATCH] - Updated usage and simplified sub-command action invocation
- From
Imran M Yousuf <imyousuf@gmail.com>
- Date
- Jan 11, 2008, 09:09 UTC
- Message-ID
- <7bfdc29a0801110109v10135afmd57c604e0d23250d@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 58 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. > > 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.
Just a point in this regard, if we allow to have both "-b branch add" and "add -b branch" in that case there will be code redundancy as there will have to parsing of "-b branch" in the subcommand parsing and in the command module (module_add). I will be implementing now, to support both; with the exception for --quiet.
Show 68 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 "$@"
>
> 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.
>
>-- Imran M Yousuf