{"thread":{"id":"60554","subject":"[PATCH] git-prompt: stop manually parsing HEAD","startedAt":"2023-11-24T11:37:13Z","lastAt":"2024-01-04T14:47:58Z","messageCount":7,"participants":["Patrick Steinhardt","Eric Sunshine","SZEDER Gábor","Oswald Buddenhagen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485122","messageId":"cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im","threadId":"60554","inReplyTo":null,"subject":"[PATCH] git-prompt: stop manually parsing HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-24T11:37:06Z","receivedAt":"2023-11-24T11:37:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're manually parsing the HEAD reference in git-prompt to figure out\nwhether it is a symbolic or direct reference. This makes it intimately\ntied to the on-disk format we use to store references and will stop\nworking once we gain additional reference backends in the Git project.\n\nRefactor the code to always use git-symbolic-ref(1) to read HEAD, which\nis both simpler and compatible with alternate reference backends.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n contrib/completion/git-prompt.sh | 22 ++++++++++------------\n 1 file changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 2c030050ae..05de540e13 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -474,17 +474,10 @@ __git_ps1 ()\n \n \t\tif [ -n \"$b\" ]; then\n \t\t\t:\n-\t\telif [ -h \"$g/HEAD\" ]; then\n-\t\t\t# symlink symbolic ref\n-\t\t\tb=\"$(git symbolic-ref HEAD 2>/dev/null)\"\n \t\telse\n-\t\t\tlocal head=\"\"\n-\t\t\tif ! __git_eread \"$g/HEAD\" head; then\n-\t\t\t\treturn $exit\n-\t\t\tfi\n-\t\t\t# is it a symbolic ref?\n-\t\t\tb=\"${head#ref: }\"\n-\t\t\tif [ \"$head\" = \"$b\" ]; then\n+\t\t\tb=\"$(git symbolic-ref HEAD 2>/dev/null)\"\n+\n+\t\t\tif test -z \"$b\"; then\n \t\t\t\tdetached=yes\n \t\t\t\tb=\"$(\n \t\t\t\tcase \"${GIT_PS1_DESCRIBE_STYLE-}\" in\n@@ -498,9 +491,14 @@ __git_ps1 ()\n \t\t\t\t\tgit describe HEAD ;;\n \t\t\t\t(* | default)\n \t\t\t\t\tgit describe --tags --exact-match HEAD ;;\n-\t\t\t\tesac 2>/dev/null)\" ||\n+\t\t\t\tesac 2>/dev/null)\"\n+\n+\t\t\t\tif test -z \"$b\"; then\n+\t\t\t\t\ttest -z \"$short_sha\" && return $exit\n+\n+\t\t\t\t\tb=\"$short_sha...\"\n+\t\t\t\tfi\n \n-\t\t\t\tb=\"$short_sha...\"\n \t\t\t\tb=\"($b)\"\n \t\t\tfi\n \t\tfi\n-- \n2.43.0\n\n"},{"id":"485133","messageId":"CAPig+cSa9zxEhKXtXHrVuB3Eb6=vnD1Ppnfj59rA+nPEO-9j-w@mail.gmail.com","threadId":"60554","inReplyTo":"cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im","subject":"Re: [PATCH] git-prompt: stop manually parsing HEAD","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-11-24T18:09:03Z","receivedAt":"2023-11-24T18:09:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:\n> We're manually parsing the HEAD reference in git-prompt to figure out\n> whether it is a symbolic or direct reference. This makes it intimately\n> tied to the on-disk format we use to store references and will stop\n> working once we gain additional reference backends in the Git project.\n>\n> Refactor the code to always use git-symbolic-ref(1) to read HEAD, which\n> is both simpler and compatible with alternate reference backends.\n\nThis may get some push-back from Windows folks due to high\nprocess-creation cost on that platform. As I recall, over the years, a\ngood deal of effort has been put into reducing the number of programs\nrun each time the prompt is displayed, precisely because invoking Git\n(or other programs) multiple times became unbearably slow. In\nparticular, optimizations efforts have focussed on computing as much\nas possible within the shell itself rather than invoking external\nprograms for the same purpose. Thus, this seems to be taking a step\nbackwards in that regard for the common or status quo case.\n\nWould it be possible instead to, within shell, detect if the historic\nfile-based backend is being used in the current repository, thus\ncontinue using the existing shell code for that case, and only employ\ngit-symbolic-ref if some other backend is in use?\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n"},{"id":"485134","messageId":"20231124182803.GA11157@szeder.dev","threadId":"60554","inReplyTo":"CAPig+cSa9zxEhKXtXHrVuB3Eb6=vnD1Ppnfj59rA+nPEO-9j-w@mail.gmail.com","subject":"Re: [PATCH] git-prompt: stop manually parsing HEAD","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2023-11-24T18:28:03Z","receivedAt":"2023-11-24T18:28:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 24, 2023 at 01:09:03PM -0500, Eric Sunshine wrote:\n> On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:\n> > We're manually parsing the HEAD reference in git-prompt to figure out\n> > whether it is a symbolic or direct reference. This makes it intimately\n> > tied to the on-disk format we use to store references and will stop\n> > working once we gain additional reference backends in the Git project.\n> >\n> > Refactor the code to always use git-symbolic-ref(1) to read HEAD, which\n> > is both simpler and compatible with alternate reference backends.\n> \n> This may get some push-back from Windows folks due to high\n> process-creation cost on that platform. As I recall, over the years, a\n> good deal of effort has been put into reducing the number of programs\n> run each time the prompt is displayed, precisely because invoking Git\n> (or other programs) multiple times became unbearably slow. In\n> particular, optimizations efforts have focussed on computing as much\n> as possible within the shell itself rather than invoking external\n> programs for the same purpose. Thus, this seems to be taking a step\n> backwards in that regard for the common or status quo case.\n> \n> Would it be possible instead to, within shell, detect if the historic\n> file-based backend is being used in the current repository, thus\n> continue using the existing shell code for that case, and only employ\n> git-symbolic-ref if some other backend is in use?\n\nThanks for sharing my worries :)\n\nI sent a patch a while ago to Han-Wen to make our Bash prompt script\nwork with the reftable backend without incurring the overhead of extra\nsubshells or processes when using the files based refs backend.  He\npicked it up and used to include it in rerolls of the reftable patch\nseries; the last version of that patch is I believe at:\n\n  https://public-inbox.org/git/patch-v4-21.28-443bdebfb5d-20210823T120208Z-avarab@gmail.com/\n\n"},{"id":"485299","messageId":"ZWmMdjh4_25GIzjy@tanuki","threadId":"60554","inReplyTo":"20231124182803.GA11157@szeder.dev","subject":"Re: [PATCH] git-prompt: stop manually parsing HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-01T07:34:14Z","receivedAt":"2023-12-01T07:34:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 24, 2023 at 07:28:03PM +0100, SZEDER Gábor wrote:\n> On Fri, Nov 24, 2023 at 01:09:03PM -0500, Eric Sunshine wrote:\n> > On Fri, Nov 24, 2023 at 6:37 AM Patrick Steinhardt <ps@pks.im> wrote:\n> > > We're manually parsing the HEAD reference in git-prompt to figure out\n> > > whether it is a symbolic or direct reference. This makes it intimately\n> > > tied to the on-disk format we use to store references and will stop\n> > > working once we gain additional reference backends in the Git project.\n> > >\n> > > Refactor the code to always use git-symbolic-ref(1) to read HEAD, which\n> > > is both simpler and compatible with alternate reference backends.\n> > \n> > This may get some push-back from Windows folks due to high\n> > process-creation cost on that platform. As I recall, over the years, a\n> > good deal of effort has been put into reducing the number of programs\n> > run each time the prompt is displayed, precisely because invoking Git\n> > (or other programs) multiple times became unbearably slow. In\n> > particular, optimizations efforts have focussed on computing as much\n> > as possible within the shell itself rather than invoking external\n> > programs for the same purpose. Thus, this seems to be taking a step\n> > backwards in that regard for the common or status quo case.\n> > \n> > Would it be possible instead to, within shell, detect if the historic\n> > file-based backend is being used in the current repository, thus\n> > continue using the existing shell code for that case, and only employ\n> > git-symbolic-ref if some other backend is in use?\n> \n> Thanks for sharing my worries :)\n> \n> I sent a patch a while ago to Han-Wen to make our Bash prompt script\n> work with the reftable backend without incurring the overhead of extra\n> subshells or processes when using the files based refs backend.  He\n> picked it up and used to include it in rerolls of the reftable patch\n> series; the last version of that patch is I believe at:\n> \n>   https://public-inbox.org/git/patch-v4-21.28-443bdebfb5d-20210823T120208Z-avarab@gmail.com/\n\nFair enough, I'm sure I can roll something similar into my patch series.\nI do wonder whether it's fine to already submit those patches now where\nthe reftable backend doesn't exist yet. But I'd hope so, because it\nwould make it significantly easier for us to upstream the backend if we\ncan only focus on the backend itself, whereas all the other parts were\nalready addressed in preliminary refactorings.\n\nOne question though is what the right way to detect the reference format\nwould be. Reading HEAD and comparing it to \"ref: refs/heads/.invalid\" is\nokay for now, but doesn't really feel like a good fit in the long term\nas there has been discussion around dropping the requirement for HEAD to\nexist altogether [1] in the future. There are some alternatives:\n\n  - Check for the existence of `reftables/` via `test -d`. This is easy\n    enough to do, but also doesn't feel all that robust.\n\n  - Extend git-rev-parse(1) to support a new `--show-reference-format`\n    option. We already have `--show-object-format`, so this would be a\n    natural fit.\n\nIn the long term I'd aim for the second solution -- it's the most robust\nsolution and would also scale if there ever were additional backends.\nFurthermore, we already execute git-rev-parse(1) unconditionally anyway.\nSo there wouldn't be a performance hit here.\n\nWhile I plan to introduce the `extensions.refStorage` format before\nupstreaming the new backend itself, I think it's still going to be some\ntime until I submit that patch series. Until then, I'd say we simply use\nthe proposed way of parsing HEAD and second-guessing that it might\nindicate the reftable backend, like Stan also does at [2] for our Bash\ncompletion code. I'll make a mental note to refactor these once we have\nthe extension ready.\n\nPatrick\n\n[1]: <ZWcOvjGPVS_CMUAk@tanuki>\n[2]: <20231130202404.89791-1-stanhu@gmail.com>\n"},{"id":"486283","messageId":"ef4e36a5a40c369da138242a8fdc9e12a846613b.1704356313.git.ps@pks.im","threadId":"60554","inReplyTo":"cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im","subject":"[PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-04T08:21:53Z","receivedAt":"2024-01-04T08:21:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're manually parsing the HEAD reference in git-prompt to figure out\nwhether it is a symbolic or direct reference. This makes it intimately\ntied to the on-disk format we use to store references and will stop\nworking once we gain additional reference backends in the Git project.\n\nIdeally, we would refactor the code to exclusively use plumbing tools to\nread refs such that we do not have to care about the on-disk format at\nall. Unfortunately though, spawning processes can be quite expensive on\nsome systems like Windows. As the Git prompt logic may be executed quite\nfrequently we try very hard to spawn as few processes as possible. This\nrefactoring is thus out of question for now.\n\nInstead, condition the logic on the repository's ref format: if the repo\nuses the the \"files\" backend we can continue to use the old logic and\nread the respective files from disk directly. If it's anything else,\nthen we use git-symbolic-ref(1) to read the value of HEAD.\n\nThis change makes the Git prompt compatible with the upcoming \"reftable\"\nformat.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nThis patch depends on \"ps/refstorage-extension\", which is currently at\n1b2234079b (t9500: write \"extensions.refstorage\" into config,\n2023-12-29).\n\n contrib/completion/git-prompt.sh | 31 ++++++++++++++++++++++++-------\n 1 file changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 2c030050ae..71f179cba3 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -408,7 +408,7 @@ __git_ps1 ()\n \n \tlocal repo_info rev_parse_exit_code\n \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n-\t\t--is-bare-repository --is-inside-work-tree \\\n+\t\t--is-bare-repository --is-inside-work-tree --show-ref-format \\\n \t\t--short HEAD 2>/dev/null)\"\n \trev_parse_exit_code=\"$?\"\n \n@@ -421,6 +421,8 @@ __git_ps1 ()\n \t\tshort_sha=\"${repo_info##*$'\\n'}\"\n \t\trepo_info=\"${repo_info%$'\\n'*}\"\n \tfi\n+\tlocal ref_format=\"${repo_info##*$'\\n'}\"\n+\trepo_info=\"${repo_info%$'\\n'*}\"\n \tlocal inside_worktree=\"${repo_info##*$'\\n'}\"\n \trepo_info=\"${repo_info%$'\\n'*}\"\n \tlocal bare_repo=\"${repo_info##*$'\\n'}\"\n@@ -479,12 +481,25 @@ __git_ps1 ()\n \t\t\tb=\"$(git symbolic-ref HEAD 2>/dev/null)\"\n \t\telse\n \t\t\tlocal head=\"\"\n-\t\t\tif ! __git_eread \"$g/HEAD\" head; then\n-\t\t\t\treturn $exit\n-\t\t\tfi\n-\t\t\t# is it a symbolic ref?\n-\t\t\tb=\"${head#ref: }\"\n-\t\t\tif [ \"$head\" = \"$b\" ]; then\n+\n+\t\t\tcase \"$ref_format\" in\n+\t\t\tfiles)\n+\t\t\t\tif ! __git_eread \"$g/HEAD\" head; then\n+\t\t\t\t\treturn $exit\n+\t\t\t\tfi\n+\n+\t\t\t\tif [[ $head == \"ref: \"* ]]; then\n+\t\t\t\t\thead=\"${head#ref: }\"\n+\t\t\t\telse\n+\t\t\t\t\thead=\"\"\n+\t\t\t\tfi\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\thead=\"$(git symbolic-ref HEAD 2>/dev/null)\"\n+\t\t\t\t;;\n+\t\t\tesac\n+\n+\t\t\tif test -z \"$head\"; then\n \t\t\t\tdetached=yes\n \t\t\t\tb=\"$(\n \t\t\t\tcase \"${GIT_PS1_DESCRIBE_STYLE-}\" in\n@@ -502,6 +517,8 @@ __git_ps1 ()\n \n \t\t\t\tb=\"$short_sha...\"\n \t\t\t\tb=\"($b)\"\n+\t\t\telse\n+\t\t\t\tb=\"$head\"\n \t\t\tfi\n \t\tfi\n \tfi\n\n-- \n2.43.GIT\n\n"},{"id":"486289","messageId":"ZZabqfPDcA1jtmZS@ugly","threadId":"60554","inReplyTo":"ef4e36a5a40c369da138242a8fdc9e12a846613b.1704356313.git.ps@pks.im","subject":"Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2024-01-04T11:51:05Z","receivedAt":"2024-01-04T11:51:13Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:\n>--- a/contrib/completion/git-prompt.sh\n>+++ b/contrib/completion/git-prompt.sh\n>@@ -408,7 +408,7 @@ __git_ps1 ()\n> \n> \tlocal repo_info rev_parse_exit_code\n> \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n>-\t\t--is-bare-repository --is-inside-work-tree \\\n>+\t\t--is-bare-repository --is-inside-work-tree --show-ref-format \\\n> \t\t--short HEAD 2>/dev/null)\"\n>\nthat makes me wonder whether adding support for `--symbolic-ref HEAD` \nhere would not be the cleaner solution? and why stop there, and not add \na few more ps1 would need, like --upstream and --sequencer-state?  \n(though arguably, this overloading of `rev-parse` should be deprecated \nin favor of a new generalized `query` command, maybe even unified with \n`var`.)\n\n"},{"id":"486292","messageId":"ZZbFGTkM8aR7MXQu@tanuki","threadId":"60554","inReplyTo":"ZZabqfPDcA1jtmZS@ugly","subject":"Re: [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-04T14:47:53Z","receivedAt":"2024-01-04T14:47:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 04, 2024 at 12:51:05PM +0100, Oswald Buddenhagen wrote:\n> On Thu, Jan 04, 2024 at 09:21:53AM +0100, Patrick Steinhardt wrote:\n> > --- a/contrib/completion/git-prompt.sh\n> > +++ b/contrib/completion/git-prompt.sh\n> > @@ -408,7 +408,7 @@ __git_ps1 ()\n> > \n> > \tlocal repo_info rev_parse_exit_code\n> > \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n> > -\t\t--is-bare-repository --is-inside-work-tree \\\n> > +\t\t--is-bare-repository --is-inside-work-tree --show-ref-format \\\n> > \t\t--short HEAD 2>/dev/null)\"\n> > \n> that makes me wonder whether adding support for `--symbolic-ref HEAD` here\n> would not be the cleaner solution? and why stop there, and not add a few\n> more ps1 would need, like --upstream and --sequencer-state?  (though\n> arguably, this overloading of `rev-parse` should be deprecated in favor of a\n> new generalized `query` command, maybe even unified with `var`.)\n\nI'm on board with extending git-rev-parse(1) to support direct output of\nsymbolic refs without resolving them to an object ID. Indeed, we plan to\ntackle this lack of support soonish at GitLab. But given that such a\nfeature currently doesn't exist, and that I expect there to be some\ndiscussion around it, I'd rather want to postpone this to a later point\nso that we can meanwhile unblock the reftable backend.\n\nRegarding the other options like `--upstream` and `--sequencer-state`\nI'm less sure. As you say, git-rev-parse(1) is already quite loaded with\nsemi-related tools, and extending it even further like this is only\ngoing to make this state worse. I also wish for an \"informative\" tool\nthat queries repository-level information and state like you propose,\nbut would argue that this is also a bigger topic.\n\nSo... for now I'd like to keep the current version, but I certainly\nagree that the state can and should eventually be improved.\n\nPatrick\n"}]}