git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: SZEDER Gábor <szeder.dev@gmail.com>
To: Benjamin Fuchs <email@benjaminfuchs.de>
Cc: SZEDER Gábor <szeder.dev@gmail.com>,
	sbeller@google.com, sandals@crustytoothpaste.net,
	ville.skytta@iki.fi, git@vger.kernel.org
Subject: Re: [PATCH 2/4] git-prompt.sh: rework of submodule indicator
Date: Tue, 31 Jan 2017 19:06:33 +0100
Message-ID: <20170131180633.22369-1-szeder.dev@gmail.com> (raw)
In-Reply-To: <1485809065-11978-3-git-send-email-email@benjaminfuchs.de>


> Rework of the first patch. The prompt now will look like this:
> (+name:master). I tried to considere all suggestions.
> Tests still missing.
> 
> Signed-off-by: Benjamin Fuchs <email@benjaminfuchs.de>
> ---
>  contrib/completion/git-prompt.sh | 49 ++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 4c82e7f..c44b9a2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -95,8 +95,10 @@
>  # 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.
> +# set GIT_PS1_SHOWSUBMODULE to a nonempty value. In this case the name
> +# of the submodule will be prepended to the branch name (e.g. module:master).
> +# The name will be prepended by "+" if the currently checked out submodule
> +# commit does not match the SHA-1 found in the index of the containing repository.
>  
>  # check whether printf supports -v
>  __git_printf_supports_v=
> @@ -288,30 +290,27 @@ __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 ()
> -{
> -	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)"
> -	module_name="$(basename "$git_dir")"
> -	parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> /dev/null)"
> -	if [[ -n $parent_git ]]; then
> -		# 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)
> -    fi
> -    return 1
> -}
> -
> +# __git_ps1_submodule
> +#
> +# $1 - git_dir path
> +#
> +# Returns the name of the submodule if we are currently inside one. The name
> +# will be prepended by "+" if the currently checked out submodule commit does
> +# not match the SHA-1 found in the index of the containing repository.
> +# NOTE: git_dir looks like in a ...
> +# - submodule: "GIT_PARENT/.git/modules/PATH_TO_SUBMODULE"
> +# - parent: "GIT_PARENT/.git" (exception "." in .git)
>  __git_ps1_submodule ()
>  {
> -	__git_is_submodule && printf "sub:"
> +	local git_dir="$1"
> +	local submodule_name="$(basename "$git_dir")"
> +	if [ "$submodule_name" != ".git" ] && [ "$submodule_name" != "." ]; then
> +		local parent_top="${git_dir%.git*}"
> +		local submodule_top="${git_dir#*modules}"
> +		local status=""
> +		git diff -C "$parent_top" --no-ext-diff --ignore-submodules=dirty --quiet -- "$submodule_top" 2>/dev/null || status="+"

This 'git diff' has to read the index of the parent repository, right?
That can be potentially very expensive, if the parent repository, and
thus its index, is big.

You might want to provide finer granularity controls and separate the
"$PWD is in a submodule" indicator from the "submodule commit doesn't
match" indicator.  Even if someone is in general interested in the
former, he might have some huge repositories where he would prefer to
disable the latter, because executing 'git diff' would make the prompt
lag.

I'm not sure we need brand new control knobs, though.  Perhaps we can
get away by checking the env and config variables used for the dirty
index and worktree status indicator, after all they, too, are about
whether to run 'git diff' for the prompt in a repository or not.

> +		printf "$status$submodule_name:"
> +	fi
>  }
>  
>  # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
> @@ -545,7 +544,7 @@ __git_ps1 ()
>  
>  	local sub=""
>  	if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
> -		sub="$(__git_ps1_submodule)"
> +		sub="$(__git_ps1_submodule $g)"

In Bash, and in shells in general, the visibility of variables works
differently than in "regular" programming languages:

  - Any variable existing in a caller, even the ones declared as
    'local' in the caller, are visible in all callees.
    
    This means you don't have to pass $g as parameter to
    __git_ps1_submodule(), because you can access it inside that
    function directly.  This has the benefit that there is one less
    place where you can forget to quote a path variable :)

  - If the callee modifies any variable that isn't declared as local
    in the callee, then the caller will see the modified value of that
    variable, unless the callee was invoked in a subshell.

    Here your sole purpose is to set $sub to something like "+sub:",
    which is determined in __git_ps1_submodule().  You don't need the
    command substitution for that, you can set $sub directly in
    __git_ps1_submodule(), sparing the overhead of fork()ing a
    subshell.  That's important for the sake of git users on Windows.

>  	fi
>  
>  	local f="$w$i$s$u"
-- 
2.7.4



  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-30 20:44 [PATCH 0/4] git-prompt.sh: Full patch for " Benjamin Fuchs
2017-01-30 20:44 ` [PATCH 1/4] git-prompt.sh: add " Benjamin Fuchs
2017-01-30 23:48   ` Junio C Hamano
2017-01-31  0:10     ` Benjamin Fuchs
2017-01-31  3:11       ` Junio C Hamano
2017-02-06  4:23         ` Stefan Beller
2017-02-06  5:55           ` Jacob Keller
2017-02-06 10:13             ` Stefan Beller
2017-01-30 20:44 ` [PATCH 2/4] git-prompt.sh: rework of " Benjamin Fuchs
2017-01-31 18:06   ` SZEDER Gábor [this message]
2017-01-30 20:44 ` [PATCH 3/4] git-prompt.sh: fix for submodule 'dirty' indicator Benjamin Fuchs
2017-01-30 20:44 ` [PATCH 4/4] git-prompt.sh: add tests for submodule indicator Benjamin Fuchs
2017-01-31 18:32   ` SZEDER Gábor
2017-01-31 22:06     ` Junio C Hamano
2017-01-31 22:12       ` Stefan Beller
2017-03-07  3:45         ` [RFC PATCH] rev-parse: add --show-superproject-working-tree Stefan Beller
2017-03-07  5:13           ` Junio C Hamano
2017-03-07  7:16             ` Junio C Hamano
2017-03-07  7:23               ` Junio C Hamano
2017-03-07 18:44           ` Junio C Hamano
2017-03-07 20:40             ` Stefan Beller
2017-03-07 22:49               ` Junio C Hamano
2017-03-08  0:56                 ` [PATCHv2] " Stefan Beller
2017-03-08  1:30                   ` Junio C Hamano
2017-03-08  6:01                   ` Junio C Hamano
2017-03-08 19:20                     ` [PATCHv3] " Stefan Beller
2017-03-08 22:28                       ` Junio C Hamano
2017-03-08 23:07                         ` [PATCHv4] " Stefan Beller
2017-03-08 23:51                           ` Junio C Hamano
2017-03-17 22:28                           ` Jonathan Nieder
2017-03-17 22:51                             ` [PATCH] Documentation/git-worktree: use working tree for trees on the file system Stefan Beller
2017-03-17 22:55                               ` Jonathan Nieder
2017-03-17 23:04                                 ` Stefan Beller
2017-03-18 17:24                                   ` Junio C Hamano
2017-03-18  1:47                                 ` Junio C Hamano
2017-03-18  1:36                               ` Junio C Hamano
2017-03-20 17:29                                 ` Stefan Beller
2017-03-20 18:12                                   ` Junio C Hamano
2017-03-20 18:50                                     ` Jonathan Nieder
2017-03-20 19:22                                       ` [PATCH 0/2] use "working trees" instead of "worktree" in our API Stefan Beller
2017-03-20 19:22                                         ` [PATCH 1/2] git.c: introduce --working-tree superseding --work-tree Stefan Beller
2017-03-20 19:58                                           ` Jonathan Nieder
2017-03-20 19:22                                         ` [PATCH 2/2] revparse: introduce --is-inside-working-tree Stefan Beller
2017-03-20 20:00                                           ` Jonathan Nieder
2017-03-20 19:37                                         ` [PATCH 0/2] use "working trees" instead of "worktree" in our API Junio C Hamano
2017-03-21 10:37                                       ` [PATCH] Documentation/git-worktree: use working tree for trees on the file system Duy Nguyen
2017-03-21 15:48                                         ` Junio C Hamano
2017-03-23 17:06                                           ` Michael J Gruber
2017-03-23 17:55                                             ` Junio C Hamano
2017-03-25 12:07                                               ` Duy Nguyen
2017-04-07 13:59                                                 ` Michael J Gruber
2017-04-07 16:14                                                   ` Jacob Keller
2017-03-25 12:05                                           ` Duy Nguyen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170131180633.22369-1-szeder.dev@gmail.com \
    --to=szeder.dev@gmail.com \
    --cc=email@benjaminfuchs.de \
    --cc=git@vger.kernel.org \
    --cc=sandals@crustytoothpaste.net \
    --cc=sbeller@google.com \
    --cc=ville.skytta@iki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox