[PATCH] git-prompt: stop manually parsing HEAD

[PATCH] git-prompt: stop manually parsing HEAD

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

We're manually parsing the HEAD reference in git-prompt to figure out
whether it is a symbolic or direct reference. This makes it intimately
tied to the on-disk format we use to store references and will stop
working once we gain additional reference backends in the Git project.

Refactor the code to always use git-symbolic-ref(1) to read HEAD, which
is both simpler and compatible with alternate reference backends.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/completion/git-prompt.sh | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 2c030050ae..05de540e13 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -474,17 +474,10 @@ __git_ps1 ()
 
 		if [ -n "$b" ]; then
 			:
-		elif [ -h "$g/HEAD" ]; then
-			# symlink symbolic ref
-			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
-			local head=""
-			if ! __git_eread "$g/HEAD" head; then
-				return $exit
-			fi
-			# is it a symbolic ref?
-			b="${head#ref: }"
-			if [ "$head" = "$b" ]; then
+			b="$(git symbolic-ref HEAD 2>/dev/null)"
+
+			if test -z "$b"; then
 				detached=yes
 				b="$(
 				case "${GIT_PS1_DESCRIBE_STYLE-}" in
@@ -498,9 +491,14 @@ __git_ps1 ()
 					git describe HEAD ;;
 				(* | default)
 					git describe --tags --exact-match HEAD ;;
-				esac 2>/dev/null)" ||
+				esac 2>/dev/null)"
+
+				if test -z "$b"; then
+					test -z "$short_sha" && return $exit
+
+					b="$short_sha..."
+				fi
 
-				b="$short_sha..."
 				b="($b)"
 			fi
 		fi
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] git-prompt: stop manually parsing HEAD

[Eric Sunshine]

On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> We're manually parsing the HEAD reference in git-prompt to figure out
> whether it is a symbolic or direct reference. This makes it intimately
> tied to the on-disk format we use to store references and will stop
> working once we gain additional reference backends in the Git project.
>
> Refactor the code to always use git-symbolic-ref(1) to read HEAD, which
> is both simpler and compatible with alternate reference backends.

This may get some push-back from Windows folks due to high
process-creation cost on that platform. As I recall, over the years, a
good deal of effort has been put into reducing the number of programs
run each time the prompt is displayed, precisely because invoking Git
(or other programs) multiple times became unbearably slow. In
particular, optimizations efforts have focussed on computing as much
as possible within the shell itself rather than invoking external
programs for the same purpose. Thus, this seems to be taking a step
backwards in that regard for the common or status quo case.

Would it be possible instead to, within shell, detect if the historic
file-based backend is being used in the current repository, thus
continue using the existing shell code for that case, and only employ
git-symbolic-ref if some other backend is in use?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Re: [PATCH] git-prompt: stop manually parsing HEAD

[SZEDER Gábor]

On Fri, Nov 24, 2023 at 01:09:03PM -0500, Eric Sunshine wrote:
> On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > We're manually parsing the HEAD reference in git-prompt to figure out
> > whether it is a symbolic or direct reference. This makes it intimately
> > tied to the on-disk format we use to store references and will stop
> > working once we gain additional reference backends in the Git project.
> >
> > Refactor the code to always use git-symbolic-ref(1) to read HEAD, which
> > is both simpler and compatible with alternate reference backends.
> 
> This may get some push-back from Windows folks due to high
> process-creation cost on that platform. As I recall, over the years, a
> good deal of effort has been put into reducing the number of programs
> run each time the prompt is displayed, precisely because invoking Git
> (or other programs) multiple times became unbearably slow. In
> particular, optimizations efforts have focussed on computing as much
> as possible within the shell itself rather than invoking external
> programs for the same purpose. Thus, this seems to be taking a step
> backwards in that regard for the common or status quo case.
> 
> Would it be possible instead to, within shell, detect if the historic
> file-based backend is being used in the current repository, thus
> continue using the existing shell code for that case, and only employ
> git-symbolic-ref if some other backend is in use?

Thanks for sharing my worries :)

I sent a patch a while ago to Han-Wen to make our Bash prompt script
work with the reftable backend without incurring the overhead of extra
subshells or processes when using the files based refs backend.  He
picked it up and used to include it in rerolls of the reftable patch
series; the last version of that patch is I believe at:

  https://public-inbox.org/git/patch-v4-21.28-443bdebfb5d-20210823T120208Z-avarab@gmail.com/

Re: [PATCH] git-prompt: stop manually parsing HEAD

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3976 bytes --]

On Fri, Nov 24, 2023 at 07:28:03PM +0100, SZEDER Gábor wrote:
> On Fri, Nov 24, 2023 at 01:09:03PM -0500, Eric Sunshine wrote:
> > On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:
> > > We're manually parsing the HEAD reference in git-prompt to figure out
> > > whether it is a symbolic or direct reference. This makes it intimately
> > > tied to the on-disk format we use to store references and will stop
> > > working once we gain additional reference backends in the Git project.
> > >
> > > Refactor the code to always use git-symbolic-ref(1) to read HEAD, which
> > > is both simpler and compatible with alternate reference backends.
> > 
> > This may get some push-back from Windows folks due to high
> > process-creation cost on that platform. As I recall, over the years, a
> > good deal of effort has been put into reducing the number of programs
> > run each time the prompt is displayed, precisely because invoking Git
> > (or other programs) multiple times became unbearably slow. In
> > particular, optimizations efforts have focussed on computing as much
> > as possible within the shell itself rather than invoking external
> > programs for the same purpose. Thus, this seems to be taking a step
> > backwards in that regard for the common or status quo case.
> > 
> > Would it be possible instead to, within shell, detect if the historic
> > file-based backend is being used in the current repository, thus
> > continue using the existing shell code for that case, and only employ
> > git-symbolic-ref if some other backend is in use?
> 
> Thanks for sharing my worries :)
> 
> I sent a patch a while ago to Han-Wen to make our Bash prompt script
> work with the reftable backend without incurring the overhead of extra
> subshells or processes when using the files based refs backend.  He
> picked it up and used to include it in rerolls of the reftable patch
> series; the last version of that patch is I believe at:
> 
>   https://public-inbox.org/git/patch-v4-21.28-443bdebfb5d-20210823T120208Z-avarab@gmail.com/

Fair enough, I'm sure I can roll something similar into my patch series.
I do wonder whether it's fine to already submit those patches now where
the reftable backend doesn't exist yet. But I'd hope so, because it
would make it significantly easier for us to upstream the backend if we
can only focus on the backend itself, whereas all the other parts were
already addressed in preliminary refactorings.

One question though is what the right way to detect the reference format
would be. Reading HEAD and comparing it to "ref: refs/heads/.invalid" is
okay for now, but doesn't really feel like a good fit in the long term
as there has been discussion around dropping the requirement for HEAD to
exist altogether [1] in the future. There are some alternatives:

  - Check for the existence of `reftables/` via `test -d`. This is easy
    enough to do, but also doesn't feel all that robust.

  - Extend git-rev-parse(1) to support a new `--show-reference-format`
    option. We already have `--show-object-format`, so this would be a
    natural fit.

In the long term I'd aim for the second solution -- it's the most robust
solution and would also scale if there ever were additional backends.
Furthermore, we already execute git-rev-parse(1) unconditionally anyway.
So there wouldn't be a performance hit here.

While I plan to introduce the `extensions.refStorage` format before
upstreaming the new backend itself, I think it's still going to be some
time until I submit that patch series. Until then, I'd say we simply use
the proposed way of parsing HEAD and second-guessing that it might
indicate the reftable backend, like Stan also does at [2] for our Bash
completion code. I'll make a mental note to refactor these once we have
the extension ready.

Patrick

[1]: <ZWcOvjGPVS_CMUAk@tanuki>
[2]: <20231130202404.89791-1-stanhu@gmail.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

We're manually parsing the HEAD reference in git-prompt to figure out
whether it is a symbolic or direct reference. This makes it intimately
tied to the on-disk format we use to store references and will stop
working once we gain additional reference backends in the Git project.

Ideally, we would refactor the code to exclusively use plumbing tools to
read refs such that we do not have to care about the on-disk format at
all. Unfortunately though, spawning processes can be quite expensive on
some systems like Windows. As the Git prompt logic may be executed quite
frequently we try very hard to spawn as few processes as possible. This
refactoring is thus out of question for now.

Instead, condition the logic on the repository's ref format: if the repo
uses the the "files" backend we can continue to use the old logic and
read the respective files from disk directly. If it's anything else,
then we use git-symbolic-ref(1) to read the value of HEAD.

This change makes the Git prompt compatible with the upcoming "reftable"
format.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

This patch depends on "ps/refstorage-extension", which is currently at
1b2234079b (t9500: write "extensions.refstorage" into config,
2023-12-29).

 contrib/completion/git-prompt.sh | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 2c030050ae..71f179cba3 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -408,7 +408,7 @@ __git_ps1 ()
 
 	local repo_info rev_parse_exit_code
 	repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
-		--is-bare-repository --is-inside-work-tree \
+		--is-bare-repository --is-inside-work-tree --show-ref-format \
 		--short HEAD 2>/dev/null)"
 	rev_parse_exit_code="$?"
 
@@ -421,6 +421,8 @@ __git_ps1 ()
 		short_sha="${repo_info##*$'\n'}"
 		repo_info="${repo_info%$'\n'*}"
 	fi
+	local ref_format="${repo_info##*$'\n'}"
+	repo_info="${repo_info%$'\n'*}"
 	local inside_worktree="${repo_info##*$'\n'}"
 	repo_info="${repo_info%$'\n'*}"
 	local bare_repo="${repo_info##*$'\n'}"
@@ -479,12 +481,25 @@ __git_ps1 ()
 			b="$(git symbolic-ref HEAD 2>/dev/null)"
 		else
 			local head=""
-			if ! __git_eread "$g/HEAD" head; then
-				return $exit
-			fi
-			# is it a symbolic ref?
-			b="${head#ref: }"
-			if [ "$head" = "$b" ]; then
+
+			case "$ref_format" in
+			files)
+				if ! __git_eread "$g/HEAD" head; then
+					return $exit
+				fi
+
+				if [[ $head == "ref: "* ]]; then
+					head="${head#ref: }"
+				else
+					head=""
+				fi
+				;;
+			*)
+				head="$(git symbolic-ref HEAD 2>/dev/null)"
+				;;
+			esac
+
+			if test -z "$head"; then
 				detached=yes
 				b="$(
 				case "${GIT_PS1_DESCRIBE_STYLE-}" in
@@ -502,6 +517,8 @@ __git_ps1 ()
 
 				b="$short_sha..."
 				b="($b)"
+			else
+				b="$head"
 			fi
 		fi
 	fi

-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats

[Oswald Buddenhagen]

On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:
>--- a/contrib/completion/git-prompt.sh
>+++ b/contrib/completion/git-prompt.sh
>@@ -408,7 +408,7 @@ __git_ps1 ()
> 
> 	local repo_info rev_parse_exit_code
> 	repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
>-		--is-bare-repository --is-inside-work-tree \
>+		--is-bare-repository --is-inside-work-tree --show-ref-format \
> 		--short HEAD 2>/dev/null)"
>
that makes me wonder whether adding support for `--symbolic-ref HEAD` 
here would not be the cleaner solution? and why stop there, and not add 
a few more ps1 would need, like --upstream and --sequencer-state?  
(though arguably, this overloading of `rev-parse` should be deprecated 
in favor of a new generalized `query` command, maybe even unified with 
`var`.)

Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On Thu, Jan 04, 2024 at 12:51:05PM +0100, Oswald Buddenhagen wrote:
> On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:
> > --- a/contrib/completion/git-prompt.sh
> > +++ b/contrib/completion/git-prompt.sh
> > @@ -408,7 +408,7 @@ __git_ps1 ()
> > 
> > 	local repo_info rev_parse_exit_code
> > 	repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
> > -		--is-bare-repository --is-inside-work-tree \
> > +		--is-bare-repository --is-inside-work-tree --show-ref-format \
> > 		--short HEAD 2>/dev/null)"
> > 
> that makes me wonder whether adding support for `--symbolic-ref HEAD` here
> would not be the cleaner solution? and why stop there, and not add a few
> more ps1 would need, like --upstream and --sequencer-state?  (though
> arguably, this overloading of `rev-parse` should be deprecated in favor of a
> new generalized `query` command, maybe even unified with `var`.)

I'm on board with extending git-rev-parse(1) to support direct output of
symbolic refs without resolving them to an object ID. Indeed, we plan to
tackle this lack of support soonish at GitLab. But given that such a
feature currently doesn't exist, and that I expect there to be some
discussion around it, I'd rather want to postpone this to a later point
so that we can meanwhile unblock the reftable backend.

Regarding the other options like `--upstream` and `--sequencer-state`
I'm less sure. As you say, git-rev-parse(1) is already quite loaded with
semi-related tools, and extending it even further like this is only
going to make this state worse. I also wish for an "informative" tool
that queries repository-level information and state like you propose,
but would argue that this is also a bigger topic.

So... for now I'd like to keep the current version, but I certainly
agree that the state can and should eventually be improved.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]