threads / patch / 11548

patchSimplified the invocation of command action in submodule

Subject: [PATCH] Simplified the invocation of command action in submodule

## tl;dr

12 messages between Jan 9, 2008 and Jan 10, 2008. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

imyousuf@gmail.com· Jan 9, 2008, 03:59 UTC · lore
From: Imran M Yousuf <imran@smartitengineering.com>
- Simplified the invocation of action.
- Changed switch case based action invoke rather more direct command
invocation. Previously first switch case was used to go through $@ and
determine the action, i.e. add, init, update etc, and second switch case
just to invoke the action. It is modified to determine the action name in
the first case structure instead and later just invoke it.
Signed-off-by: Imran M Yousuf <imyousuf@smartitengineering.com>
---
 git-submodule.sh |   32 ++++++++++++--------------------
 1 files changed, 12 insertions(+), 20 deletions(-)
Show changes to git-submodule.sh +12 −20
diff --git a/git-submodule.sh b/git-submodule.sh
index ad9fe62..8a29382 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -16,6 +16,7 @@ update=
 status=
 quiet=
 cached=
+command=
 
 #
 # print stuff on stdout unless -q was specified
@@ -293,20 +294,23 @@ modules_list()
 	done
 }
 
+# command specifies the whole function name since 
+# one of theirs prefix is module not modules
 while test $# != 0
 do
 	case "$1" in
 	add)
 		add=1
+		command="module_$1"
 		;;
 	init)
-		init=1
+		command="modules_$1"
 		;;
 	update)
-		update=1
+		command="modules_$1"
 		;;
 	status)
-		status=1
+		command="modules_list"
 		;;
 	-q|--quiet)
 		quiet=1
@@ -320,7 +324,7 @@ do
 		branch="$2"; shift
 		;;
 	--cached)
-		cached=1
+		command="modules_list"
 		;;
 	--)
 		break
@@ -345,20 +349,8 @@ case "$add,$branch" in
 	;;
 esac
 
-case "$add,$init,$update,$status,$cached" in
-1,,,,)
-	module_add "$@"
-	;;
-,1,,,)
-	modules_init "$@"
-	;;
-,,1,,)
-	modules_update "$@"
-	;;
-,,,*,*)
-	modules_list "$@"
-	;;
-*)
+if [ -z $command ]; then 
 	usage
-	;;
-esac
+else
+	"$command" "$@"
+fi

-- 
1.5.3.7
Junio C Hamano· Jan 9, 2008, 08:19 UTC · re: imyousuf@gmail.com · lore

Re: [PATCH] Simplified the invocation of command action in submodule

imyousuf@gmail.com writes:
Show 9 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ad9fe62..8a29382 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -16,6 +16,7 @@ update=
>  status=
>  quiet=
>  cached=
> +command=

Doesn't the patch make some if not all of the above variables unused?

Show 9 quoted lines
>  	case "$1" in
>  	add)
>  		add=1
> +		command="module_$1"
>  		;;
>  	init)
> -		init=1
> +		command="modules_$1"
>  		;;
Does the remaining code still use $add?
Imran M Yousuf· Jan 9, 2008, 08:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] Simplified the invocation of command action in submodule

Hi Junio,

Firstly, $add is still used later in the code. Secondly, yes the variables should be deleted. Will make the change and send the patch again; I forgot to clean the unused variables from the declaration, sorry.

Best regards,
Imran
On Jan 9, 2008 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> imyousuf@gmail.com writes:
>
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index ad9fe62..8a29382 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
>
> Doesn't the patch make some if not all of the above variables
> unused?
>
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
>
> Does the remaining code still use $add?
>
-- 
Imran M Yousuf
Johannes Sixt· Jan 9, 2008, 08:59 UTC · re: imyousuf@gmail.com · lore

Re: [PATCH] Simplified the invocation of command action in submodule

imyousuf@gmail.com schrieb:
Show 41 quoted lines
> @@ -16,6 +16,7 @@ update=
>  status=
>  quiet=
>  cached=
> +command=
>
>  #
>  # print stuff on stdout unless -q was specified
> @@ -293,20 +294,23 @@ modules_list()
>  	done
>  }
>
> +# command specifies the whole function name since
> +# one of theirs prefix is module not modules
>  while test $# != 0
>  do
>  	case "$1" in
>  	add)
>  		add=1
> +		command="module_$1"
>  		;;
>  	init)
> -		init=1
> +		command="modules_$1"
>  		;;
>  	update)
> -		update=1
> +		command="modules_$1"
>  		;;
>  	status)
> -		status=1
> +		command="modules_list"
>  		;;
>  	-q|--quiet)
>  		quiet=1
> @@ -320,7 +324,7 @@ do
>  		branch="$2"; shift
>  		;;
>  	--cached)
> -		cached=1
> +		command="modules_list"
Don't remove cached=1 because otherwise --cached is effectively ignored.
Show 28 quoted lines
>  		;;
>  	--)
>  		break
> @@ -345,20 +349,8 @@ case "$add,$branch" in
>  	;;
>  esac
>
> -case "$add,$init,$update,$status,$cached" in
> -1,,,,)
> -	module_add "$@"
> -	;;
> -,1,,,)
> -	modules_init "$@"
> -	;;
> -,,1,,)
> -	modules_update "$@"
> -	;;
> -,,,*,*)
> -	modules_list "$@"
> -	;;
> -*)
> +if [ -z $command ]; then
>  	usage
> -	;;
> -esac
> +else
> +	"$command" "$@"
> +fi
- Previously 'git submodule' was equvalent to 'git submodule status', now
it is an error.
- Previously, passing --cached to add, init, or update was an error, now
it is not.
-- Hannes
Imran M Yousuf· Jan 9, 2008, 09:07 UTC · re: Johannes Sixt · lore

Re: [PATCH] Simplified the invocation of command action in submodule

I already saw that mistake Johannes, thank you for pointing it out.
On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 83 quoted lines
> imyousuf@gmail.com schrieb:
>
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
> >
> >  #
> >  # print stuff on stdout unless -q was specified
> > @@ -293,20 +294,23 @@ modules_list()
> >       done
> >  }
> >
> > +# command specifies the whole function name since
> > +# one of theirs prefix is module not modules
> >  while test $# != 0
> >  do
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
> >       update)
> > -             update=1
> > +             command="modules_$1"
> >               ;;
> >       status)
> > -             status=1
> > +             command="modules_list"
> >               ;;
> >       -q|--quiet)
> >               quiet=1
> > @@ -320,7 +324,7 @@ do
> >               branch="$2"; shift
> >               ;;
> >       --cached)
> > -             cached=1
> > +             command="modules_list"
>
> Don't remove cached=1 because otherwise --cached is effectively ignored.
>
> >               ;;
> >       --)
> >               break
> > @@ -345,20 +349,8 @@ case "$add,$branch" in
> >       ;;
> >  esac
> >
> > -case "$add,$init,$update,$status,$cached" in
> > -1,,,,)
> > -     module_add "$@"
> > -     ;;
> > -,1,,,)
> > -     modules_init "$@"
> > -     ;;
> > -,,1,,)
> > -     modules_update "$@"
> > -     ;;
> > -,,,*,*)
> > -     modules_list "$@"
> > -     ;;
> > -*)
> > +if [ -z $command ]; then
> >       usage
> > -     ;;
> > -esac
> > +else
> > +     "$command" "$@"
> > +fi
>
> - Previously 'git submodule' was equvalent to 'git submodule status', now
> it is an error.
>
> - Previously, passing --cached to add, init, or update was an error, now
> it is not.
>
> -- Hannes
>
-- 
Imran M Yousuf
Entrepreneur & Software Engineer
Smart IT Engineering
Dhaka, Bangladesh
Email: imran@smartitengineering.com
Mobile: +880-1711402557
Johannes Sixt· Jan 9, 2008, 09:15 UTC · re: Imran M Yousuf · lore

Re: [PATCH] Simplified the invocation of command action in submodule

-- Hannes

of them. "that mistake" with no clue on which one you mean when I pointed out three BTW, on this list we don't top-post. In particular not when you write only

Imran M Yousuf schrieb:
Show 5 quoted lines
> I already saw that mistake Johannes, thank you for pointing it out.
> 
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> imyousuf@gmail.com schrieb:
>>
[...]
Imran M Yousuf· Jan 9, 2008, 09:51 UTC · re: Johannes Sixt · lore

Re: [PATCH] Simplified the invocation of command action in submodule

On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 77 quoted lines
> imyousuf@gmail.com schrieb:
>
> > @@ -16,6 +16,7 @@ update=
> >  status=
> >  quiet=
> >  cached=
> > +command=
> >
> >  #
> >  # print stuff on stdout unless -q was specified
> > @@ -293,20 +294,23 @@ modules_list()
> >       done
> >  }
> >
> > +# command specifies the whole function name since
> > +# one of theirs prefix is module not modules
> >  while test $# != 0
> >  do
> >       case "$1" in
> >       add)
> >               add=1
> > +             command="module_$1"
> >               ;;
> >       init)
> > -             init=1
> > +             command="modules_$1"
> >               ;;
> >       update)
> > -             update=1
> > +             command="modules_$1"
> >               ;;
> >       status)
> > -             status=1
> > +             command="modules_list"
> >               ;;
> >       -q|--quiet)
> >               quiet=1
> > @@ -320,7 +324,7 @@ do
> >               branch="$2"; shift
> >               ;;
> >       --cached)
> > -             cached=1
> > +             command="modules_list"
>
> Don't remove cached=1 because otherwise --cached is effectively ignored.
>
> >               ;;
> >       --)
> >               break
> > @@ -345,20 +349,8 @@ case "$add,$branch" in
> >       ;;
> >  esac
> >
> > -case "$add,$init,$update,$status,$cached" in
> > -1,,,,)
> > -     module_add "$@"
> > -     ;;
> > -,1,,,)
> > -     modules_init "$@"
> > -     ;;
> > -,,1,,)
> > -     modules_update "$@"
> > -     ;;
> > -,,,*,*)
> > -     modules_list "$@"
> > -     ;;
> > -*)
> > +if [ -z $command ]; then
> >       usage
> > -     ;;
> > -esac
> > +else
> > +     "$command" "$@"
> > +fi
>
> - Previously 'git submodule' was equvalent to 'git submodule status', now
> it is an error.

Yes, I forgot to add that status is the default command. Thanks for pointing it out.

>
> - Previously, passing --cached to add, init, or update was an error, now
> it is not.

The usage statement and this behaviour is rather contradicting. The usage says that --cached can be used with all commands; so I am not sure whether using --cached with add should be an error or not. IMHO, if the previous implementation was right than the USAGE has to be changed, and if the previous implementation was incorrect, than if the default command is set to status than current implementation is right.

I would like to get comment on this until I fix the patch and resend it.
>
> -- Hannes
>
Thank you,
-- 
Imran M Yousuf
Johannes Sixt· Jan 9, 2008, 10:01 UTC · re: Imran M Yousuf · lore

Re: [PATCH] Simplified the invocation of command action in submodule

Imran M Yousuf schrieb:
Show 10 quoted lines
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> - Previously, passing --cached to add, init, or update was an error, now
>> it is not.
> 
> The usage statement and this behaviour is rather contradicting. The
> usage says that --cached can be used with all commands; so I am not
> sure whether using --cached with add should be an error or not. IMHO,
> if the previous implementation was right than the USAGE has to be
> changed, and if the previous implementation was incorrect, than if the
> default command is set to status than current implementation is right.

I prefer that the usage statement lists one line per sub-command with the flags that apply only to the sub-command. IOW, a usage statement that suggests that a flag applies to all sub-commands when in reality it doesn't is bogus, IMHO.

-- Hannes
Imran M Yousuf· Jan 9, 2008, 10:06 UTC · re: Johannes Sixt · lore

Re: [PATCH] Simplified the invocation of command action in submodule

On Jan 9, 2008 4:01 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 17 quoted lines
> Imran M Yousuf schrieb:
> > On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >> - Previously, passing --cached to add, init, or update was an error, now
> >> it is not.
> >
> > The usage statement and this behaviour is rather contradicting. The
> > usage says that --cached can be used with all commands; so I am not
> > sure whether using --cached with add should be an error or not. IMHO,
> > if the previous implementation was right than the USAGE has to be
> > changed, and if the previous implementation was incorrect, than if the
> > default command is set to status than current implementation is right.
>
> I prefer that the usage statement lists one line per sub-command with the
> flags that apply only to the sub-command. IOW, a usage statement that
> suggests that a flag applies to all sub-commands when in reality it
> doesn't is bogus, IMHO.
>

I think for this patch I will keep the usage intact and keep the implementation coherent with the current usage and add a comment in that place so that if required it can be changed in future.

> -- Hannes
>
>
-- 
Imran M Yousuf
Junio C Hamano· Jan 9, 2008, 10:27 UTC · re: Johannes Sixt · lore

Re: [PATCH] Simplified the invocation of command action in submodule

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 16 quoted lines
> Imran M Yousuf schrieb:
>> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> - Previously, passing --cached to add, init, or update was an error, now
>>> it is not.
>> 
>> The usage statement and this behaviour is rather contradicting. The
>> usage says that --cached can be used with all commands; so I am not
>> sure whether using --cached with add should be an error or not. IMHO,
>> if the previous implementation was right than the USAGE has to be
>> changed, and if the previous implementation was incorrect, than if the
>> default command is set to status than current implementation is right.
>
> I prefer that the usage statement lists one line per sub-command with the
> flags that apply only to the sub-command. IOW, a usage statement that
> suggests that a flag applies to all sub-commands when in reality it
> doesn't is bogus, IMHO.

I view the usage emitted by a command primarily as a quick reminder for people who are _already_ familiar with the command to help "was the option this command takes --foo or --bar? I can never remember which X-<" situation. The usage string is not a replacement of the manual page. For that reason, I generally prefer short and sweet one line usage for the whole command, even if it does not exactly capture mutually incompatible option combinations, _as long as_ the command itself is simple enough.

As you said, however, git-submodule is a command dispatcher on its own, and what its subcommands do are quite different, to the point that they probably should not even be sharing the option parser. One line per subcommand feels more appropriate.

By the way, Imran, if the current implementation declares a combination of "add" and "--cached" an error, and a new implementation does not, that's called a regression. Unless you can prove that the combination makes sense and the existing behaviour is a bug, in which case you can say the new implementation fixes the bug.

In this case, module_add does not even pay attention to $cached in the existing code. The choice is between (1) silently ignore user's expectation that "add --cached" would do something different from "add" without "--cached", or (2) tell the user that the combination does not make sense and error out. To people who _know_ what the command does, the choice between the two does not make much difference (they do not give ignored option, nor trigger the error), but to new people the latter is often easier to use.

Lars Hjemli· Jan 9, 2008, 10:24 UTC · re: Imran M Yousuf · lore

Re: [PATCH] Simplified the invocation of command action in submodule

On Jan 9, 2008 10:51 AM, Imran M Yousuf <imyousuf@gmail.com> wrote:
Show 13 quoted lines
> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> >
> > - Previously, passing --cached to add, init, or update was an error, now
> > it is not.
>
> The usage statement and this behaviour is rather contradicting. The
> usage says that --cached can be used with all commands; so I am not
> sure whether using --cached with add should be an error or not. IMHO,
> if the previous implementation was right than the USAGE has to be
> changed, and if the previous implementation was incorrect, than if the
> default command is set to status than current implementation is right.
>
> I would like to get comment on this until I fix the patch and resend it.

--cached only makes sense for the status subcommand, so the usage/manpage probably should have looked like this (except for the whitespace mangling...):

Show changes to Documentation/git-submodule.txt +4 −1
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index cffc6d4..331e806 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,7 +10,10 @@ SYNOPSIS
 --------
 [verse]
 'git-submodule' [--quiet] [-b branch] add <repository> [<path>]
-'git-submodule' [--quiet] [--cached] [status|init|update] [--] [<path>...]
+'git-submodule' [--quiet] [--cached] [status] [--] [<path>...]
+'git-submodule' [--quiet] init [--] [<path>...]
+'git-submodule' [--quiet] update [--] [<path>...]
+


 COMMANDS

-- 
1.5.3.7.1141.g4eb39
Imran M Yousuf· Jan 10, 2008, 03:05 UTC · re: Lars Hjemli · lore

Re: [PATCH] Simplified the invocation of command action in submodule

On Jan 9, 2008 4:24 PM, Lars Hjemli <lh@elementstorage.no> wrote:
Show 33 quoted lines
> On Jan 9, 2008 10:51 AM, Imran M Yousuf <imyousuf@gmail.com> wrote:
> > On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> > >
> > > - Previously, passing --cached to add, init, or update was an error, now
> > > it is not.
> >
> > The usage statement and this behaviour is rather contradicting. The
> > usage says that --cached can be used with all commands; so I am not
> > sure whether using --cached with add should be an error or not. IMHO,
> > if the previous implementation was right than the USAGE has to be
> > changed, and if the previous implementation was incorrect, than if the
> > default command is set to status than current implementation is right.
> >
> > I would like to get comment on this until I fix the patch and resend it.
>
> --cached only makes sense for the status subcommand, so the
> usage/manpage probably should have looked like this (except for the
> whitespace mangling...):
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index cffc6d4..331e806 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -10,7 +10,10 @@ SYNOPSIS
>  --------
>  [verse]
>  'git-submodule' [--quiet] [-b branch] add <repository> [<path>]
> -'git-submodule' [--quiet] [--cached] [status|init|update] [--] [<path>...]
> +'git-submodule' [--quiet] [--cached] [status] [--] [<path>...]
> +'git-submodule' [--quiet] init [--] [<path>...]
> +'git-submodule' [--quiet] update [--] [<path>...]
> +
>

This change makes a lot sense. Thus I will make sure that it is used in this manner :). Thanks a lot for clarifying it Lars. I wanted to know what is the purpose of '--'? If it is simply meant to be a separator than fine; else I would be grateful if you would please explain its purpose, so that I do not again implement wrongly :).

Show 5 quoted lines
>
>  COMMANDS
> --
> 1.5.3.7.1141.g4eb39
>
-- 
Imran M Yousuf

← back to recent threads