Re: [PATCH] git-prompt.sh: add submodule indicator

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 [off-list ref] 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 <redacted>
---
 contrib/completion/git-prompt.sh | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Tests?

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.

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