I'm not well-versed in submodules, so I won't comment on whether this is the right way to determine that a repository is a submodule, but I was surprised to see how much you have to work to find this out. My comments will mostly focus on how to eliminate or at least limit the scope of subshells and command executions, because fork()s and exec()s are rather expensive on Windows. On Fri, Jan 20, 2017 at 1:07 AM, Benjamin Fuchs <email@xxxxxxxxxxxxxxxx> wrote: > I expirienced that working with submodules can be confusing. This indicator > will make you notice very easy when you switch into a submodule. > The new prompt will look like this: (sub:master) > Adding a new optional env variable for the new feature. > > Signed-off-by: Benjamin Fuchs <email@xxxxxxxxxxxxxxxx> > --- > contrib/completion/git-prompt.sh | 37 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) Tests? > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > index 97eacd7..4c82e7f 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -93,6 +93,10 @@ > # directory is set up to be ignored by git, then set > # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the > # repository level by setting bash.hideIfPwdIgnored to "false". > +# > +# If you would like __git_ps1 to indicate that you are in a submodule, > +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before > +# the branch name. > > # check whether printf supports -v > __git_printf_supports_v= > @@ -284,6 +288,32 @@ __git_eread () > test -r "$f" && read "$@" <"$f" > } > > +# __git_is_submodule > +# Based on: > +# http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule > +__git_is_submodule () Use the '__git_ps1' prefix in the function name, like the other functions. > +{ > + local git_dir parent_git module_name path > + # Find the root of this git repo, then check if its parent dir is also a repo > + git_dir="$(git rev-parse --show-toplevel)" 1. This is a somewhat misleading variable name, because git_dir (with or without underscore or dash) refers to the path to the .git directory of a repository through the codebase, documentation and CLI options, not the top-level directory of the worktree. 2. In a bare repository or in the .git directory of a "regular" repository '--show-toplevel' doesn't output anything, leading to an empty $module_name below, which ultimately results in no submodule indicator. As fas as behavior is concerned, this is good in the bare repository case, because as I understand it there is no such thing as a bare submodule. I'm not sure whether the submodule indicator should be displayed in the ".git dir of a submodule" case. I leave it up to you and Stefan to discuss. However, the info about whether we are in a bare repository or .git dir is already availabe in certain variables, so we know upfront when the current repository can't be a submodule. In those cases this function should not be called only to spend several subshells and command executions to figure out what we already knew anyway. 3. The path to the .git directory of the current repository is already available in the (not particularly descriptively named) $g variable from __git_ps1. Perhaps you could use that variable instead, thus avoiding a subshell and executing a git command here. > + module_name="$(basename "$git_dir")" This is executed even when there is no repository in the parent directories, but it's only needed when there _is_ a repo up there. Please move it into the conditional below, to avoid a subshell and command execution in the main code path. Since this $git_dir comes directly from our own 'git rev-parse' do we have to be prepared for a Windows-style path there? If it were always a UNIX-style path, then we could strip all directories with shell parameter expansion, eliminating both the subshell and the basename execution. > + parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)" Style nit: no space after redirection, i.e. it should be '2>/dev/null'. More importantly, I don't think you really need this variable: 1. The existence of a parent git repository can be determined from 'git rev-parse's exit code alone. 2. When you run 'git submodule' below, you don't have to cd to the _top-level_ directory of the parent repository's worktree. You just have to cd to _any_ directory in the parent, and you can do that with 'git -C "$git_dir/.." submodule ...', without knowing the parent's top-level directory. This means that you don't need 'git rev-parse's output, thus there is no need for the command substitution. Yet another subshell spared! :) However, then you have to be careful with changing directories, and should write it as 'git -C "$git_dir/.." rev-parse ...'. > + if [[ -n $parent_git ]]; then Unquoted path. > + # List all the submodule paths for the parent repo > + while read path > + do > + if [[ "$path" != "$module_name" ]]; then continue; fi > + if [[ -d "$git_dir/../$path" ]]; then return 0; fi > + done < <(cd $parent_git && git submodule --quiet foreach 'echo $path' 2> /dev/null) Unquoted path and space after redirection again. This loop will get confused if one of the submodule paths contains a newline, though probably no one in their right mind would do something like that. Is it even possible to have a submodule name with newline in it? I wonder whether it would be possible to move the conditions from the loop body to the shell command evaluated by 'submodule foreach', thus eliminating the loop entirely and making submodule detection newline-safe. Or whether it's worth the trouble... > + fi > + return 1 Indent with spaces, please use tabs instead. > +} > + > +__git_ps1_submodule () > +{ > + __git_is_submodule && printf "sub:" > +} This function is unnecessary. First, it's so short that it's body can be safely inlined in its caller. Second, it _prints_ its output to stdout, forcing the caller to use a command substitution. You can simply assign the value to $sub based on __git_ps1_is_submodule()'s return value. > + > # __git_ps1 accepts 0 or 1 arguments (i.e., format string) > # when called from PS1 using command substitution > # in this mode it prints text to add to bash PS1 prompt (includes branch name) > @@ -513,8 +543,13 @@ __git_ps1 () > b="\${__git_ps1_branch_name}" > fi > > + local sub="" Thank you for _not_ introducing yet another one letter variable name :) > + if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then > + sub="$(__git_ps1_submodule)" > + fi > + > local f="$w$i$s$u" > - local gitstring="$c$b${f:+$z$f}$r$p" > + local gitstring="$c$sub$b${f:+$z$f}$r$p" > > if [ $pcmode = yes ]; then > if [ "${__git_printf_supports_v-}" != yes ]; then > -- > 2.7.4 >