Re: [PATCH] - Added pre command and custom argument support to git submodule recurse command

[Junio C Hamano <gitster@pobox.com>]

imyousuf@gmail.com writes:

> From: Imran M Yousuf <imyousuf@smartitengineering.com>
>
> There is one scenario that has been put forward several times in
> discussion over the recurse command - it is that commands can have
> different arguments for different modules. For example for the same example
> mentioned above, one wants to check a_1 for submdoule a, while it wants to
> checkout d_2 for d. It can be achieved by using [-ca|--customized-argument].

Read this again, notice "for the same example mentioned above", and go
"Huh?"

> This results the script to prompt for user input, which will be passed as
> argument to the command for that module.
> 	git submodule recurse -ca checkout
> 	Working in mod a .......
> 	Please provide arguments for this module: a_1
> 	Working in mod d .......
> 	Please provide arguments for this module: a_1

Again, a blank line before the displayed script like this would make it
easier to read.

A single dash with two letters for an option name is somewhat
unconventional.  Shouldn't this be called interactive arguments, by the
way?

> It is usually helpful that when typing a command, being able to see some options
> come in handy. For example if I can see the available branches before checking
> out a branch that would be useful, IOW, if one could git branch before git
> checkout; it is now possible using the [-p|--pre-command] option. Using this
> command you can actually execute other git commands before specifying the
> arguments to the original command. E.g. if the above command is changed to,
> 	git submodule recurse -ca -p checkout
> it will prompt the user for the pre command until one is satisfied and later
> the user can actually use them in the argument.

Btw, can we please try to keep commit log messages readable?

The above "blob of text" could/should have more structure than being just 
one big block, and could have been structured as a few shorter paragraphs 
to make it easier to read.

I don't know about you guys, but I read a *lot* of emails (and commit
messages), and I hate seeing big blobs of text without structure. Give it
a few breaks to make it easier to read, like just making new paragraphs,
i.e. something like:

> When typing a command, being able to see some options come in handy.
> For example, if a command asks for an option to "git checkout", being
> able to run "git branch" to see what branches exist before answering
> might help the user.
>
> "git submodule recurse" allows this with the [-p|--pre-command]
> option. With this option, you can actually execute other git commands
> before specifying the arguments to the original command. E.g. if the
> above command is changed to:
>
>     git submodule recurse -ca -p checkout
>
> it will prompt the user for the pre command until one is satisfied and
> later the user can actually use them in the argument.

and now you have a bit of a breather space and some visual cues for where
you are in the text.

Yeah, maybe it's just me, but I like my whitespace. Ihaveareallyhardtime
readingtextthatdoesn'thavethepropermarkersforwhereconceptsstartandbegin, 
andthatreallydoesincludetheverticalwhitespacetoo.

By the way, I do not find your example particularly convincing.

> +do_pre_command()
> +{
> +	say "Starting pre-comamnd execution!"
> +	while :
> +	do
> +		(
> +			read -p "Please provide a git command: " pre_command

"read -p"?  That's not even in POSIX.  Please don't.

> +			test -z "$pre_command" || git "$pre_command"

I am not convinced.  Why do you limit it only to a git command?  Why do
you limit it only to a git command that does not take any parameters?  How
is this more useful over \C-z and returning to a shell, or examining the
situation in a different window/screen?

> +}
> +
> +# Take arguments from user to pass as custom arguments
> +get_custom_args()
> +{
> +	read -p "Please provide arguments for this module: " custom_args
> +}

Contrary to its name, it reads a _single_ argument,...

>  # Initializes the submodule if already not initialized
>  # and auto initialize is enabled
>  initialize_sub_module()
> @@ -460,8 +484,10 @@ traverse_module()
>  		# pwd is mentioned in order to enable the ser to distinguish
>  		# between same name modules, e.g. a/lib and b/lib.
>  		say "Working in mod $submod_path" @ `pwd` "with $@ ($#)"
> +		test -n "$pre_cmd" && do_pre_command
> +		test -n "$use_custom_args" && get_custom_args
>  		cmd_status=
> -		git "$@" || cmd_status=1
> +		git "$@" "$custom_args" || cmd_status=1

... and passes it as a single argument.

The overall structure of recursing into and running arbitrary commands
inside each submodule might be useful, but the implementation feels rather
too limiting.

Come to think of it, does it really matter that the command you run by
recursing into them is limited to "git-foo" command?  I do not see you are
taking advantage of it being a git command, so it feels like an arbitrary
restriction to me, too.