Re: [PATCH] - Updated usage and simplified sub-command action invocation

On Jan 10, 2008 12:23 PM, Junio C Hamano [off-list ref] wrote:

imyousuf@gmail.com writes:

quoted

From: Imran M Yousuf <redacted>

- 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.

quoted

+# 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.

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