| Message ID | cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-prompt: stop manually parsing HEAD | expand |
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>
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/
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>
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
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(-)