{"thread":{"id":"60574","subject":"[PATCH 0/2] completion: refactor and support reftables backend","startedAt":"2023-11-30T20:24:22Z","lastAt":"2024-01-13T21:07:17Z","messageCount":15,"participants":["Stan Hu","Patrick Steinhardt","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"485289","messageId":"20231130202404.89791-1-stanhu@gmail.com","threadId":"60574","inReplyTo":null,"subject":"[PATCH 0/2] completion: refactor and support reftables backend","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-11-30T20:24:02Z","receivedAt":"2023-11-30T20:24:22Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"Hi,\n\nThis patch series refactors and updates git-completion.bash to become\ncompatible with the upcoming reftables backend.\n\nStan Hu (2):\n  completion: refactor existence checks for special refs\n  completion: stop checking for reference existence via `test -f`\n\n contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---\n 1 file changed, 38 insertions(+), 5 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"485290","messageId":"20231130202404.89791-2-stanhu@gmail.com","threadId":"60574","inReplyTo":"20231130202404.89791-1-stanhu@gmail.com","subject":"[PATCH 1/2] completion: refactor existence checks for special refs","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-11-30T20:24:03Z","receivedAt":"2023-11-30T20:24:26Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In preparation for the reftable backend, this commit introduces a\n'__git_ref_exists' function that continues to use 'test -f' to\ndetermine whether a given ref exists in the local filesystem.\n\nEach caller of '__git_ref_exists' must call '__git_find_repo_path`\nfirst. '_git_restore' already used 'git rev-parse HEAD', but to use\n'__git_ref_exists' insert a '__git_find_repo_path' at the start.\n\nReviewed-by: Patrick Steinhardt <ps@pks.im>\nReviewed-by: Christian Couder <christian.couder@gmail.com>\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 13a39ebd2e..9fbdc13f9a 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,6 +122,15 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n+# Runs git in $__git_repo_path to determine whether a ref exists.\n+# 1: The ref to search\n+__git_ref_exists ()\n+{\n+\tlocal ref=$1\n+\n+\t[ -f \"$__git_repo_path/$ref\" ]\n+}\n+\n # Removes backslash escaping, single quotes and double quotes from a word,\n # stores the result in the variable $dequoted_word.\n # 1: The word to dequote.\n@@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n _git_cherry_pick ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/CHERRY_PICK_HEAD ]; then\n+\tif __git_ref_exists CHERRY_PICK_HEAD; then\n \t\t__gitcomp \"$__git_cherry_pick_inprogress_options\"\n \t\treturn\n \tfi\n@@ -2067,7 +2076,7 @@ _git_log ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_ref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$prev,$cur\" in\n@@ -2934,6 +2943,7 @@ _git_reset ()\n \n _git_restore ()\n {\n+\t__git_find_repo_path\n \tcase \"$prev\" in\n \t-s)\n \t\t__git_complete_refs\n@@ -2952,7 +2962,7 @@ _git_restore ()\n \t\t__gitcomp_builtin restore\n \t\t;;\n \t*)\n-\t\tif __git rev-parse --verify --quiet HEAD >/dev/null; then\n+\t\tif __git_ref_exists HEAD; then\n \t\t\t__git_complete_index_file \"--modified\"\n \t\tfi\n \tesac\n@@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options\n _git_revert ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/REVERT_HEAD ]; then\n+\tif __git_ref_exists REVERT_HEAD; then\n \t\t__gitcomp \"$__git_revert_inprogress_options\"\n \t\treturn\n \tfi\n@@ -3592,7 +3602,7 @@ __gitk_main ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_ref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$cur\" in\n-- \n2.42.0\n\n"},{"id":"485291","messageId":"20231130202404.89791-3-stanhu@gmail.com","threadId":"60574","inReplyTo":"20231130202404.89791-2-stanhu@gmail.com","subject":"[PATCH 2/2] completion: stop checking for reference existence via `test -f`","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-11-30T20:24:04Z","receivedAt":"2023-11-30T20:24:27Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In contrib/completion/git-completion.bash, there are a bunch of\ninstances where we read special refs like HEAD, MERGE_HEAD,\nREVERT_HEAD, and others via the filesystem. However, the upcoming\nreftable refs backend won't use '.git/HEAD' at all but instead will\nwrite an invalid refname as placeholder for backwards compatibility,\nwhich will break the git-completion script.\n\nUpdate the '__git_ref_exists' function to:\n\n1. Recognize the placeholder '.git/HEAD' written by the reftable\n   backend (its content is specified in the reftable specs).\n2. If reftable is in use, use 'git rev-parse' to determine whether the\n    given ref exists.\n3. Otherwise, continue to use 'test -f' to check for the ref's filename.\n\nReviewed-by: Patrick Steinhardt <ps@pks.im>\nReviewed-by: Christian Couder <christian.couder@gmail.com>\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9fbdc13f9a..f5b630ba99 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,12 +122,35 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n+# Helper function to read the first line of a file into a variable.\n+# __git_eread requires 2 arguments, the file path and the name of the\n+# variable, in that order.\n+#\n+# This is taken from git-prompt.sh.\n+__git_eread ()\n+{\n+\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n+}\n+\n # Runs git in $__git_repo_path to determine whether a ref exists.\n # 1: The ref to search\n __git_ref_exists ()\n {\n \tlocal ref=$1\n \n+\t# If the reftable is in use, we have to shell out to 'git rev-parse'\n+\t# to determine whether the ref exists instead of looking directly in\n+\t# the filesystem to determine whether the ref exists. Otherwise, use\n+\t# Bash builtins since executing Git commands are expensive on some\n+\t# platforms.\n+\tif __git_eread \"$__git_repo_path/HEAD\" head; then\n+\t\tb=\"${head#ref: }\"\n+\t\tif [ \"$b\" == \"refs/heads/.invalid\" ]; then\n+\t\t\t__git -C \"$__git_repo_path\" rev-parse --verify --quiet \"$ref\" 2>/dev/null\n+\t\t\treturn $?\n+\t\tfi\n+\tfi\n+\n \t[ -f \"$__git_repo_path/$ref\" ]\n }\n \n-- \n2.42.0\n\n"},{"id":"485301","messageId":"ZWmP9WmdaHp2Z6hh@tanuki","threadId":"60574","inReplyTo":"20231130202404.89791-1-stanhu@gmail.com","subject":"Re: [PATCH 0/2] completion: refactor and support reftables backend","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-01T07:49:09Z","receivedAt":"2023-12-01T07:49:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 30, 2023 at 12:24:02PM -0800, Stan Hu wrote:\n> Hi,\n> \n> This patch series refactors and updates git-completion.bash to become\n> compatible with the upcoming reftables backend.\n\nHi Stan!\n\nDisclaimer up front: I've already reviewed these patches internally at\nGitLab.\n\nRegarding the format of this patch submission: in the Git project we\ntypically use \"shallow\" threads, where every mail is a reply to the head\nof the series (in this case the cover letter). You can configure these\nby setting \"format.thread=shallow\" in your Git configuration.\n\nThat's an easy thing to miss though, as MyFirstContribution.txt doesn't\npoint this out at all. I wonder whether we want to amend it to say so?\n\nPatrick\n\n> Stan Hu (2):\n>   completion: refactor existence checks for special refs\n>   completion: stop checking for reference existence via `test -f`\n> \n>  contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---\n>  1 file changed, 38 insertions(+), 5 deletions(-)\n> \n> -- \n> 2.42.0\n> \n"},{"id":"485300","messageId":"ZWmP-5rgGntYQYMt@tanuki","threadId":"60574","inReplyTo":"20231130202404.89791-3-stanhu@gmail.com","subject":"Re: [PATCH 2/2] completion: stop checking for reference existence via `test -f`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-01T07:49:15Z","receivedAt":"2023-12-01T07:49:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 30, 2023 at 12:24:04PM -0800, Stan Hu wrote:\n> In contrib/completion/git-completion.bash, there are a bunch of\n> instances where we read special refs like HEAD, MERGE_HEAD,\n> REVERT_HEAD, and others via the filesystem. However, the upcoming\n> reftable refs backend won't use '.git/HEAD' at all but instead will\n> write an invalid refname as placeholder for backwards compatibility,\n> which will break the git-completion script.\n> \n> Update the '__git_ref_exists' function to:\n> \n> 1. Recognize the placeholder '.git/HEAD' written by the reftable\n>    backend (its content is specified in the reftable specs).\n> 2. If reftable is in use, use 'git rev-parse' to determine whether the\n>     given ref exists.\n> 3. Otherwise, continue to use 'test -f' to check for the ref's filename.\n\nNit, not worth a reroll: you already document this in the code, but I\nthink it could help to also briefly explain why we're going through all\nof these hoops here instead of just using git-rev-parse(1) everywhere in\nthe commit message.\n\nPatrick\n"},{"id":"485336","messageId":"xmqqedg34ecc.fsf@gitster.g","threadId":"60574","inReplyTo":"20231130202404.89791-2-stanhu@gmail.com","subject":"Re: [PATCH 1/2] completion: refactor existence checks for special refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-03T13:15:15Z","receivedAt":"2023-12-03T13:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stan Hu <stanhu@gmail.com> writes:\n\n> In preparation for the reftable backend, this commit introduces a\n> '__git_ref_exists' function that continues to use 'test -f' to\n> determine whether a given ref exists in the local filesystem.\n\nI do not think git_ref_exists is a good name for what this one does.\nThe name adds undue cognitive load on readers.  As far as I can\ntell, with this helper function, you are interested in handling only\npseudorefs like $GIT_DIR/FOOBAR_HEAD (oh, retitle the patch to call\nthem \"pseudorefs\", not \"special refs\", by the way), and that is why\nyou can get away with a simple\n\n    [ -f \"$__git_repo_path/$ref\" ]\n\nwithout bothering to check the packed-refs file.  The checks this\npatch replace to calls to this helper functions in the original make\nit clear, simply because they spell out what they are checking, like\n\"CHERRY_PICK_HEAD\", why a mere filesystem check was sufficient, but\nonce you give an overly generic name like \"ref-exists\", it becomes\ntempting to (ab|mis)use it to check for branches and tags, which is\nnot your intention at all, and the implementation does not work well\nfor that purpose.\n\n> Each caller of '__git_ref_exists' must call '__git_find_repo_path`\n> first. '_git_restore' already used 'git rev-parse HEAD', but to use\n> '__git_ref_exists' insert a '__git_find_repo_path' at the start.\n\nTo whom do you think the above piece of information is essential for\nthem to work?  Whoever updates the completion script, finds existing\nuse of __git_ref_exists, and thinks the helper would be useful for\ntheir own use.  To them, the above needs be in the in-code comment\nto make it discoverable.  It is OK to have it also in the proposed\nlog message, but it is not as essential, especially if you have it\nin-code anyway.\n\nAnother thing you would need to make sure that the potential users\nof this helpers understand is of course this is meant to be used\nonly on pseudorefs.  You can of course do so with an additional\nin-code comment, but giving a more appropriate name to the helper\nwould be the easiest and simplest, e.g. __git_pseudoref_exists or\nsomething.\n\n> Reviewed-by: Patrick Steinhardt <ps@pks.im>\n> Reviewed-by: Christian Couder <christian.couder@gmail.com>\n> Signed-off-by: Stan Hu <stanhu@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 13a39ebd2e..9fbdc13f9a 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -122,6 +122,15 @@ __git ()\n>  \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n>  }\n>  \n> +# Runs git in $__git_repo_path to determine whether a ref exists.\n> +# 1: The ref to search\n> +__git_ref_exists ()\n> +{\n> +\tlocal ref=$1\n> +\n> +\t[ -f \"$__git_repo_path/$ref\" ]\n> +}\n"},{"id":"485408","messageId":"cover.1701928891.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"20231130202404.89791-1-stanhu@gmail.com","subject":"[PATCH v2 0/2] completion: refactor and support reftables backend","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-07T06:06:38Z","receivedAt":"2023-12-07T06:06:51Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"This patch series addresses the review feedback:\n\n1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists'.\n2. cleans up the commit messages to refer to pseudorefs instead of 'special refs'.\n\nStan Hu (2):\n  completion: refactor existence checks for pseudorefs\n  completion: support pseudoref existence checks for reftables\n\n contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---\n 1 file changed, 38 insertions(+), 5 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"485409","messageId":"1c6a747691f36ede4224b6d4c2e0c8fd4c0575fd.1701928891.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"cover.1701928891.git.stanhu@gmail.com","subject":"[PATCH v2 1/2] completion: refactor existence checks for pseudorefs","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-07T06:06:39Z","receivedAt":"2023-12-07T06:06:53Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In preparation for the reftable backend, this commit introduces a\n'__git_pseudoref_exists' function that continues to use 'test -f' to\ndetermine whether a given pseudoref exists in the local filesystem.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 13a39ebd2e..9fbdc13f9a 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,6 +122,15 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n+# Runs git in $__git_repo_path to determine whether a ref exists.\n+# 1: The ref to search\n+__git_ref_exists ()\n+{\n+\tlocal ref=$1\n+\n+\t[ -f \"$__git_repo_path/$ref\" ]\n+}\n+\n # Removes backslash escaping, single quotes and double quotes from a word,\n # stores the result in the variable $dequoted_word.\n # 1: The word to dequote.\n@@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n _git_cherry_pick ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/CHERRY_PICK_HEAD ]; then\n+\tif __git_ref_exists CHERRY_PICK_HEAD; then\n \t\t__gitcomp \"$__git_cherry_pick_inprogress_options\"\n \t\treturn\n \tfi\n@@ -2067,7 +2076,7 @@ _git_log ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_ref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$prev,$cur\" in\n@@ -2934,6 +2943,7 @@ _git_reset ()\n \n _git_restore ()\n {\n+\t__git_find_repo_path\n \tcase \"$prev\" in\n \t-s)\n \t\t__git_complete_refs\n@@ -2952,7 +2962,7 @@ _git_restore ()\n \t\t__gitcomp_builtin restore\n \t\t;;\n \t*)\n-\t\tif __git rev-parse --verify --quiet HEAD >/dev/null; then\n+\t\tif __git_ref_exists HEAD; then\n \t\t\t__git_complete_index_file \"--modified\"\n \t\tfi\n \tesac\n@@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options\n _git_revert ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/REVERT_HEAD ]; then\n+\tif __git_ref_exists REVERT_HEAD; then\n \t\t__gitcomp \"$__git_revert_inprogress_options\"\n \t\treturn\n \tfi\n@@ -3592,7 +3602,7 @@ __gitk_main ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_ref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$cur\" in\n-- \n2.42.0\n\n"},{"id":"485410","messageId":"9b0e090d91f99a1a0cd78f2a93939630ab44abd1.1701928891.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"cover.1701928891.git.stanhu@gmail.com","subject":"[PATCH v2 2/2] completion: support pseudoref existence checks for reftables","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-07T06:06:40Z","receivedAt":"2023-12-07T06:06:54Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In contrib/completion/git-completion.bash, there are a bunch of\ninstances where we read pseudorefs, such as HEAD, MERGE_HEAD,\nREVERT_HEAD, and others via the filesystem. However, the upcoming\nreftable refs backend won't use '.git/HEAD' at all but instead will\nwrite an invalid refname as placeholder for backwards compatibility,\nwhich will break the git-completion script.\n\nUpdate the '__git_pseudoref_exists' function to:\n\n1. Recognize the placeholder '.git/HEAD' written by the reftable\n   backend (its content is specified in the reftable specs).\n2. If reftable is in use, use 'git rev-parse' to determine whether the\n    given ref exists.\n3. Otherwise, continue to use 'test -f' to check for the ref's filename.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 39 ++++++++++++++++++++------\n 1 file changed, 31 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9fbdc13f9a..b2e407c7e7 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,12 +122,35 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n-# Runs git in $__git_repo_path to determine whether a ref exists.\n-# 1: The ref to search\n-__git_ref_exists ()\n+# Helper function to read the first line of a file into a variable.\n+# __git_eread requires 2 arguments, the file path and the name of the\n+# variable, in that order.\n+#\n+# This is taken from git-prompt.sh.\n+__git_eread ()\n+{\n+\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n+}\n+\n+# Runs git in $__git_repo_path to determine whether a pseudoref exists.\n+# 1: The pseudo-ref to search\n+__git_pseudoref_exists()\n {\n \tlocal ref=$1\n \n+\t# If the reftable is in use, we have to shell out to 'git rev-parse'\n+\t# to determine whether the ref exists instead of looking directly in\n+\t# the filesystem to determine whether the ref exists. Otherwise, use\n+\t# Bash builtins since executing Git commands are expensive on some\n+\t# platforms.\n+\tif __git_eread \"$__git_repo_path/HEAD\" head; then\n+\t\tb=\"${head#ref: }\"\n+\t\tif [ \"$b\" == \"refs/heads/.invalid\" ]; then\n+\t\t\t__git -C \"$__git_repo_path\" rev-parse --verify --quiet \"$ref\" 2>/dev/null\n+\t\t\treturn $?\n+\t\tfi\n+\tfi\n+\n \t[ -f \"$__git_repo_path/$ref\" ]\n }\n \n@@ -1634,7 +1657,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n _git_cherry_pick ()\n {\n \t__git_find_repo_path\n-\tif __git_ref_exists CHERRY_PICK_HEAD; then\n+\tif __git_pseudoref_exists CHERRY_PICK_HEAD; then\n \t\t__gitcomp \"$__git_cherry_pick_inprogress_options\"\n \t\treturn\n \tfi\n@@ -2076,7 +2099,7 @@ _git_log ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif __git_ref_exists MERGE_HEAD; then\n+\tif __git_pseudoref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$prev,$cur\" in\n@@ -2962,7 +2985,7 @@ _git_restore ()\n \t\t__gitcomp_builtin restore\n \t\t;;\n \t*)\n-\t\tif __git_ref_exists HEAD; then\n+\t\tif __git_pseudoref_exists HEAD; then\n \t\t\t__git_complete_index_file \"--modified\"\n \t\tfi\n \tesac\n@@ -2973,7 +2996,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options\n _git_revert ()\n {\n \t__git_find_repo_path\n-\tif __git_ref_exists REVERT_HEAD; then\n+\tif __git_pseudoref_exists REVERT_HEAD; then\n \t\t__gitcomp \"$__git_revert_inprogress_options\"\n \t\treturn\n \tfi\n@@ -3602,7 +3625,7 @@ __gitk_main ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif __git_ref_exists MERGE_HEAD; then\n+\tif __git_pseudoref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$cur\" in\n-- \n2.42.0\n\n"},{"id":"485459","messageId":"ZXLSpyE5ZW0ANmsi@tanuki","threadId":"60574","inReplyTo":"1c6a747691f36ede4224b6d4c2e0c8fd4c0575fd.1701928891.git.stanhu@gmail.com","subject":"Re: [PATCH v2 1/2] completion: refactor existence checks for pseudorefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-08T08:24:07Z","receivedAt":"2023-12-08T08:24:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 10:06:39PM -0800, Stan Hu wrote:\n> In preparation for the reftable backend, this commit introduces a\n> '__git_pseudoref_exists' function that continues to use 'test -f' to\n> determine whether a given pseudoref exists in the local filesystem.\n> \n> Signed-off-by: Stan Hu <stanhu@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 13a39ebd2e..9fbdc13f9a 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -122,6 +122,15 @@ __git ()\n>  \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n>  }\n>  \n> +# Runs git in $__git_repo_path to determine whether a ref exists.\n> +# 1: The ref to search\n> +__git_ref_exists ()\n\nI first thought that you missed Junio's point that `__git_ref_exists`\nmay better be renamed to something lkie `__git_pseudoref_exists`. But\nyou do indeed change the name in the second patch. I'd propose to\ninstead squash the rename into the first patch so that the series\nbecomes easier to read.\n\nPatrick\n"},{"id":"485830","messageId":"cover.1703022850.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"20231130202404.89791-1-stanhu@gmail.com","subject":"[PATCH v3 0/2] completion: refactor and support reftables backend","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-19T22:14:16Z","receivedAt":"2023-12-19T22:14:35Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"This patch series addresses the review feedback:\n\n1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in\n   the first refactor patch.\n\nStan Hu (2):\n  completion: refactor existence checks for pseudorefs\n  completion: support pseudoref existence checks for reftables\n\n contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---\n 1 file changed, 38 insertions(+), 5 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"485831","messageId":"29c928649aba7dfce4dab1b5d923bc23b7af2d32.1703022850.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"cover.1703022850.git.stanhu@gmail.com","subject":"[PATCH v3 1/2] completion: refactor existence checks for pseudorefs","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-19T22:14:17Z","receivedAt":"2023-12-19T22:14:36Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In preparation for the reftable backend, this commit introduces a\n'__git_pseudoref_exists' function that continues to use 'test -f' to\ndetermine whether a given pseudoref exists in the local filesystem.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 13a39ebd2e..8edd002eed 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,6 +122,15 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n+# Runs git in $__git_repo_path to determine whether a pseudoref exists.\n+# 1: The pseudo-ref to search\n+__git_pseudoref_exists ()\n+{\n+\tlocal ref=$1\n+\n+\t[ -f \"$__git_repo_path/$ref\" ]\n+}\n+\n # Removes backslash escaping, single quotes and double quotes from a word,\n # stores the result in the variable $dequoted_word.\n # 1: The word to dequote.\n@@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n _git_cherry_pick ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/CHERRY_PICK_HEAD ]; then\n+\tif __git_pseudoref_exists CHERRY_PICK_HEAD; then\n \t\t__gitcomp \"$__git_cherry_pick_inprogress_options\"\n \t\treturn\n \tfi\n@@ -2067,7 +2076,7 @@ _git_log ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_pseudoref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$prev,$cur\" in\n@@ -2934,6 +2943,7 @@ _git_reset ()\n \n _git_restore ()\n {\n+\t__git_find_repo_path\n \tcase \"$prev\" in\n \t-s)\n \t\t__git_complete_refs\n@@ -2952,7 +2962,7 @@ _git_restore ()\n \t\t__gitcomp_builtin restore\n \t\t;;\n \t*)\n-\t\tif __git rev-parse --verify --quiet HEAD >/dev/null; then\n+\t\tif __git_pseudoref_exists HEAD; then\n \t\t\t__git_complete_index_file \"--modified\"\n \t\tfi\n \tesac\n@@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options\n _git_revert ()\n {\n \t__git_find_repo_path\n-\tif [ -f \"$__git_repo_path\"/REVERT_HEAD ]; then\n+\tif __git_pseudoref_exists REVERT_HEAD; then\n \t\t__gitcomp \"$__git_revert_inprogress_options\"\n \t\treturn\n \tfi\n@@ -3592,7 +3602,7 @@ __gitk_main ()\n \t__git_find_repo_path\n \n \tlocal merge=\"\"\n-\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n+\tif __git_pseudoref_exists MERGE_HEAD; then\n \t\tmerge=\"--merge\"\n \tfi\n \tcase \"$cur\" in\n-- \n2.42.0\n\n"},{"id":"485832","messageId":"e25a0c5c84dfeca7067fe803388ea3bcee58860c.1703022850.git.stanhu@gmail.com","threadId":"60574","inReplyTo":"cover.1703022850.git.stanhu@gmail.com","subject":"[PATCH v3 2/2] completion: support pseudoref existence checks for reftables","fromName":"Stan Hu","fromEmail":"stanhu@gmail.com","sentAt":"2023-12-19T22:14:18Z","receivedAt":"2023-12-19T22:14:37Z","isPatch":true,"sender":{"key":"stanhu@gmail.com","avatar":"https://avatars.githubusercontent.com/u/963826?v=4"},"body":"In contrib/completion/git-completion.bash, there are a bunch of\ninstances where we read pseudorefs, such as HEAD, MERGE_HEAD,\nREVERT_HEAD, and others via the filesystem. However, the upcoming\nreftable refs backend won't use '.git/HEAD' at all but instead will\nwrite an invalid refname as placeholder for backwards compatibility,\nwhich will break the git-completion script.\n\nUpdate the '__git_pseudoref_exists' function to:\n\n1. Recognize the placeholder '.git/HEAD' written by the reftable\n   backend (its content is specified in the reftable specs).\n2. If reftable is in use, use 'git rev-parse' to determine whether the\n    given ref exists.\n3. Otherwise, continue to use 'test -f' to check for the ref's filename.\n\nSigned-off-by: Stan Hu <stanhu@gmail.com>\n---\n contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 8edd002eed..e21a39b406 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -122,12 +122,35 @@ __git ()\n \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n }\n \n+# Helper function to read the first line of a file into a variable.\n+# __git_eread requires 2 arguments, the file path and the name of the\n+# variable, in that order.\n+#\n+# This is taken from git-prompt.sh.\n+__git_eread ()\n+{\n+\ttest -r \"$1\" && IFS=$'\\r\\n' read -r \"$2\" <\"$1\"\n+}\n+\n # Runs git in $__git_repo_path to determine whether a pseudoref exists.\n # 1: The pseudo-ref to search\n __git_pseudoref_exists ()\n {\n \tlocal ref=$1\n \n+\t# If the reftable is in use, we have to shell out to 'git rev-parse'\n+\t# to determine whether the ref exists instead of looking directly in\n+\t# the filesystem to determine whether the ref exists. Otherwise, use\n+\t# Bash builtins since executing Git commands are expensive on some\n+\t# platforms.\n+\tif __git_eread \"$__git_repo_path/HEAD\" head; then\n+\t\tb=\"${head#ref: }\"\n+\t\tif [ \"$b\" == \"refs/heads/.invalid\" ]; then\n+\t\t\t__git -C \"$__git_repo_path\" rev-parse --verify --quiet \"$ref\" 2>/dev/null\n+\t\t\treturn $?\n+\t\tfi\n+\tfi\n+\n \t[ -f \"$__git_repo_path/$ref\" ]\n }\n \n-- \n2.42.0\n\n"},{"id":"485834","messageId":"xmqqsf3xu2ai.fsf@gitster.g","threadId":"60574","inReplyTo":"cover.1703022850.git.stanhu@gmail.com","subject":"Re: [PATCH v3 0/2] completion: refactor and support reftables backend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-20T00:48:37Z","receivedAt":"2023-12-20T00:48:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stan Hu <stanhu@gmail.com> writes:\n\n> This patch series addresses the review feedback:\n>\n> 1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in\n>    the first refactor patch.\n>\n> Stan Hu (2):\n>   completion: refactor existence checks for pseudorefs\n>   completion: support pseudoref existence checks for reftables\n>\n>  contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---\n>  1 file changed, 38 insertions(+), 5 deletions(-)\n\nThanks.  Will queue.\n"},{"id":"486766","messageId":"20240113210714.GC3000857@szeder.dev","threadId":"60574","inReplyTo":"29c928649aba7dfce4dab1b5d923bc23b7af2d32.1703022850.git.stanhu@gmail.com","subject":"Re: [PATCH v3 1/2] completion: refactor existence checks for pseudorefs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-01-13T21:07:14Z","receivedAt":"2024-01-13T21:07:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 19, 2023 at 02:14:17PM -0800, Stan Hu wrote:\n> In preparation for the reftable backend, this commit introduces a\n> '__git_pseudoref_exists' function that continues to use 'test -f' to\n> determine whether a given pseudoref exists in the local filesystem.\n> \n> Signed-off-by: Stan Hu <stanhu@gmail.com>\n> ---\n>  contrib/completion/git-completion.bash | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 13a39ebd2e..8edd002eed 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -122,6 +122,15 @@ __git ()\n>  \t\t${__git_dir:+--git-dir=\"$__git_dir\"} \"$@\" 2>/dev/null\n>  }\n>  \n> +# Runs git in $__git_repo_path to determine whether a pseudoref exists.\n> +# 1: The pseudo-ref to search\n> +__git_pseudoref_exists ()\n> +{\n> +\tlocal ref=$1\n> +\n> +\t[ -f \"$__git_repo_path/$ref\" ]\n\nThis new helper function relies on $__git_repo_path being set, but it\ndoesn't ensure that it's actually set by calling __git_find_repo_path.\nInstead, it relies on its callers calling __git_find_repo_path,\nalthough after this patch none of those callers directly access\n$__git_repo_path anymore.\n\nIt would be better to call __git_find_repo_path in this helper to make\nit more self-contained, and then the now unnecessary\n__git_find_repo_path calls from the three callers can be removed.\n\nYeah, I know it's late, because this patch is already in master, but\nthis would be a good preparation step for eventual dedicated tests of\n__git_pseudoref_exists that came up in:\n\n  https://public-inbox.org/git/20240113191749.GB3000857@szeder.dev/\n\n> +}\n> +\n>  # Removes backslash escaping, single quotes and double quotes from a word,\n>  # stores the result in the variable $dequoted_word.\n>  # 1: The word to dequote.\n> @@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options\n>  _git_cherry_pick ()\n>  {\n>  \t__git_find_repo_path\n> -\tif [ -f \"$__git_repo_path\"/CHERRY_PICK_HEAD ]; then\n> +\tif __git_pseudoref_exists CHERRY_PICK_HEAD; then\n>  \t\t__gitcomp \"$__git_cherry_pick_inprogress_options\"\n>  \t\treturn\n>  \tfi\n> @@ -2067,7 +2076,7 @@ _git_log ()\n>  \t__git_find_repo_path\n>  \n>  \tlocal merge=\"\"\n> -\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n> +\tif __git_pseudoref_exists MERGE_HEAD; then\n>  \t\tmerge=\"--merge\"\n>  \tfi\n>  \tcase \"$prev,$cur\" in\n> @@ -2934,6 +2943,7 @@ _git_reset ()\n>  \n>  _git_restore ()\n>  {\n> +\t__git_find_repo_path\n>  \tcase \"$prev\" in\n>  \t-s)\n>  \t\t__git_complete_refs\n> @@ -2952,7 +2962,7 @@ _git_restore ()\n>  \t\t__gitcomp_builtin restore\n>  \t\t;;\n>  \t*)\n> -\t\tif __git rev-parse --verify --quiet HEAD >/dev/null; then\n> +\t\tif __git_pseudoref_exists HEAD; then\n>  \t\t\t__git_complete_index_file \"--modified\"\n>  \t\tfi\n>  \tesac\n> @@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options\n>  _git_revert ()\n>  {\n>  \t__git_find_repo_path\n> -\tif [ -f \"$__git_repo_path\"/REVERT_HEAD ]; then\n> +\tif __git_pseudoref_exists REVERT_HEAD; then\n>  \t\t__gitcomp \"$__git_revert_inprogress_options\"\n>  \t\treturn\n>  \tfi\n> @@ -3592,7 +3602,7 @@ __gitk_main ()\n>  \t__git_find_repo_path\n>  \n>  \tlocal merge=\"\"\n> -\tif [ -f \"$__git_repo_path/MERGE_HEAD\" ]; then\n> +\tif __git_pseudoref_exists MERGE_HEAD; then\n>  \t\tmerge=\"--merge\"\n>  \tfi\n>  \tcase \"$cur\" in\n> -- \n> 2.42.0\n> \n> \n"}]}