Re: [PATCH] - Added pre command and custom argument support to git submodule recurse command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 6, 2008, 10:42 UTC
- Message-ID
- <7vlk4w9lri.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1204788817-22720-3-git-send-email-imyousuf@gmail.com>
imyousuf@gmail.com writes:
Show 7 quoted lines
> 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?"
Show 7 quoted lines
> 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?
Show 9 quoted lines
> 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:
Show 14 quoted lines
> 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.
Show 7 quoted lines
> +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?
Show 7 quoted lines
> +}
> +
> +# 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,...
Show 12 quoted lines
> # 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.