# [PATCH 0/2] completion: refactor and support reftables backend 15 messages from 2023-11-30 to 2024-01-13. Participants: Stan Hu, Patrick Steinhardt, Junio C Hamano, SZEDER Gábor. Thread: https://gitlist.dev/t/60574 ## Stan Hu, 2023-11-30 20:24 Subject: [PATCH 0/2] completion: refactor and support reftables backend Message-ID: <20231130202404.89791-1-stanhu@gmail.com> URL: https://gitlist.dev/e/20231130202404.89791-1-stanhu%40gmail.com ``` Hi, This patch series refactors and updates git-completion.bash to become compatible with the upcoming reftables backend. Stan Hu (2): completion: refactor existence checks for special refs completion: stop checking for reference existence via `test -f` contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) -- 2.42.0 ``` ## Stan Hu, 2023-11-30 20:24 Subject: [PATCH 1/2] completion: refactor existence checks for special refs Message-ID: <20231130202404.89791-2-stanhu@gmail.com> URL: https://gitlist.dev/e/20231130202404.89791-2-stanhu%40gmail.com In-Reply-To: <20231130202404.89791-1-stanhu@gmail.com> ``` In preparation for the reftable backend, this commit introduces a '__git_ref_exists' function that continues to use 'test -f' to determine whether a given ref exists in the local filesystem. Each caller of '__git_ref_exists' must call '__git_find_repo_path` first. '_git_restore' already used 'git rev-parse HEAD', but to use '__git_ref_exists' insert a '__git_find_repo_path' at the start. Reviewed-by: Patrick Steinhardt Reviewed-by: Christian Couder Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 13a39ebd2e..9fbdc13f9a 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,6 +122,15 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } +# Runs git in $__git_repo_path to determine whether a ref exists. +# 1: The ref to search +__git_ref_exists () +{ + local ref=$1 + + [ -f "$__git_repo_path/$ref" ] +} + # Removes backslash escaping, single quotes and double quotes from a word, # stores the result in the variable $dequoted_word. # 1: The word to dequote. @@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options _git_cherry_pick () { __git_find_repo_path - if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then + if __git_ref_exists CHERRY_PICK_HEAD; then __gitcomp "$__git_cherry_pick_inprogress_options" return fi @@ -2067,7 +2076,7 @@ _git_log () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_ref_exists MERGE_HEAD; then merge="--merge" fi case "$prev,$cur" in @@ -2934,6 +2943,7 @@ _git_reset () _git_restore () { + __git_find_repo_path case "$prev" in -s) __git_complete_refs @@ -2952,7 +2962,7 @@ _git_restore () __gitcomp_builtin restore ;; *) - if __git rev-parse --verify --quiet HEAD >/dev/null; then + if __git_ref_exists HEAD; then __git_complete_index_file "--modified" fi esac @@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options _git_revert () { __git_find_repo_path - if [ -f "$__git_repo_path"/REVERT_HEAD ]; then + if __git_ref_exists REVERT_HEAD; then __gitcomp "$__git_revert_inprogress_options" return fi @@ -3592,7 +3602,7 @@ __gitk_main () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_ref_exists MERGE_HEAD; then merge="--merge" fi case "$cur" in -- 2.42.0 ``` ## Stan Hu, 2023-11-30 20:24 Subject: [PATCH 2/2] completion: stop checking for reference existence via `test -f` Message-ID: <20231130202404.89791-3-stanhu@gmail.com> URL: https://gitlist.dev/e/20231130202404.89791-3-stanhu%40gmail.com In-Reply-To: <20231130202404.89791-2-stanhu@gmail.com> ``` In contrib/completion/git-completion.bash, there are a bunch of instances where we read special refs like HEAD, MERGE_HEAD, REVERT_HEAD, and others via the filesystem. However, the upcoming reftable refs backend won't use '.git/HEAD' at all but instead will write an invalid refname as placeholder for backwards compatibility, which will break the git-completion script. Update the '__git_ref_exists' function to: 1. Recognize the placeholder '.git/HEAD' written by the reftable backend (its content is specified in the reftable specs). 2. If reftable is in use, use 'git rev-parse' to determine whether the given ref exists. 3. Otherwise, continue to use 'test -f' to check for the ref's filename. Reviewed-by: Patrick Steinhardt Reviewed-by: Christian Couder Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9fbdc13f9a..f5b630ba99 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,12 +122,35 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } +# Helper function to read the first line of a file into a variable. +# __git_eread requires 2 arguments, the file path and the name of the +# variable, in that order. +# +# This is taken from git-prompt.sh. +__git_eread () +{ + test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1" +} + # Runs git in $__git_repo_path to determine whether a ref exists. # 1: The ref to search __git_ref_exists () { local ref=$1 + # If the reftable is in use, we have to shell out to 'git rev-parse' + # to determine whether the ref exists instead of looking directly in + # the filesystem to determine whether the ref exists. Otherwise, use + # Bash builtins since executing Git commands are expensive on some + # platforms. + if __git_eread "$__git_repo_path/HEAD" head; then + b="${head#ref: }" + if [ "$b" == "refs/heads/.invalid" ]; then + __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null + return $? + fi + fi + [ -f "$__git_repo_path/$ref" ] } -- 2.42.0 ``` ## Patrick Steinhardt, 2023-12-01 07:49 Subject: Re: [PATCH 0/2] completion: refactor and support reftables backend Message-ID: URL: https://gitlist.dev/e/ZWmP9WmdaHp2Z6hh%40tanuki In-Reply-To: <20231130202404.89791-1-stanhu@gmail.com> ``` On Thu, Nov 30, 2023 at 12:24:02PM -0800, Stan Hu wrote: > Hi, > > This patch series refactors and updates git-completion.bash to become > compatible with the upcoming reftables backend. Hi Stan! Disclaimer up front: I've already reviewed these patches internally at GitLab. Regarding the format of this patch submission: in the Git project we typically use "shallow" threads, where every mail is a reply to the head of the series (in this case the cover letter). You can configure these by setting "format.thread=shallow" in your Git configuration. That's an easy thing to miss though, as MyFirstContribution.txt doesn't point this out at all. I wonder whether we want to amend it to say so? Patrick > Stan Hu (2): > completion: refactor existence checks for special refs > completion: stop checking for reference existence via `test -f` > > contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 5 deletions(-) > > -- > 2.42.0 > ``` ## Patrick Steinhardt, 2023-12-01 07:49 Subject: Re: [PATCH 2/2] completion: stop checking for reference existence via `test -f` Message-ID: URL: https://gitlist.dev/e/ZWmP-5rgGntYQYMt%40tanuki In-Reply-To: <20231130202404.89791-3-stanhu@gmail.com> ``` On Thu, Nov 30, 2023 at 12:24:04PM -0800, Stan Hu wrote: > In contrib/completion/git-completion.bash, there are a bunch of > instances where we read special refs like HEAD, MERGE_HEAD, > REVERT_HEAD, and others via the filesystem. However, the upcoming > reftable refs backend won't use '.git/HEAD' at all but instead will > write an invalid refname as placeholder for backwards compatibility, > which will break the git-completion script. > > Update the '__git_ref_exists' function to: > > 1. Recognize the placeholder '.git/HEAD' written by the reftable > backend (its content is specified in the reftable specs). > 2. If reftable is in use, use 'git rev-parse' to determine whether the > given ref exists. > 3. Otherwise, continue to use 'test -f' to check for the ref's filename. Nit, not worth a reroll: you already document this in the code, but I think it could help to also briefly explain why we're going through all of these hoops here instead of just using git-rev-parse(1) everywhere in the commit message. Patrick ``` ## Junio C Hamano, 2023-12-03 13:15 Subject: Re: [PATCH 1/2] completion: refactor existence checks for special refs Message-ID: URL: https://gitlist.dev/e/xmqqedg34ecc.fsf%40gitster.g In-Reply-To: <20231130202404.89791-2-stanhu@gmail.com> ``` Stan Hu writes: > In preparation for the reftable backend, this commit introduces a > '__git_ref_exists' function that continues to use 'test -f' to > determine whether a given ref exists in the local filesystem. I do not think git_ref_exists is a good name for what this one does. The name adds undue cognitive load on readers. As far as I can tell, with this helper function, you are interested in handling only pseudorefs like $GIT_DIR/FOOBAR_HEAD (oh, retitle the patch to call them "pseudorefs", not "special refs", by the way), and that is why you can get away with a simple [ -f "$__git_repo_path/$ref" ] without bothering to check the packed-refs file. The checks this patch replace to calls to this helper functions in the original make it clear, simply because they spell out what they are checking, like "CHERRY_PICK_HEAD", why a mere filesystem check was sufficient, but once you give an overly generic name like "ref-exists", it becomes tempting to (ab|mis)use it to check for branches and tags, which is not your intention at all, and the implementation does not work well for that purpose. > Each caller of '__git_ref_exists' must call '__git_find_repo_path` > first. '_git_restore' already used 'git rev-parse HEAD', but to use > '__git_ref_exists' insert a '__git_find_repo_path' at the start. To whom do you think the above piece of information is essential for them to work? Whoever updates the completion script, finds existing use of __git_ref_exists, and thinks the helper would be useful for their own use. To them, the above needs be in the in-code comment to make it discoverable. It is OK to have it also in the proposed log message, but it is not as essential, especially if you have it in-code anyway. Another thing you would need to make sure that the potential users of this helpers understand is of course this is meant to be used only on pseudorefs. You can of course do so with an additional in-code comment, but giving a more appropriate name to the helper would be the easiest and simplest, e.g. __git_pseudoref_exists or something. > Reviewed-by: Patrick Steinhardt > Reviewed-by: Christian Couder > Signed-off-by: Stan Hu > --- > contrib/completion/git-completion.bash | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 13a39ebd2e..9fbdc13f9a 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -122,6 +122,15 @@ __git () > ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null > } > > +# Runs git in $__git_repo_path to determine whether a ref exists. > +# 1: The ref to search > +__git_ref_exists () > +{ > + local ref=$1 > + > + [ -f "$__git_repo_path/$ref" ] > +} ``` ## Stan Hu, 2023-12-07 06:06 Subject: [PATCH v2 0/2] completion: refactor and support reftables backend Message-ID: URL: https://gitlist.dev/e/cover.1701928891.git.stanhu%40gmail.com In-Reply-To: <20231130202404.89791-1-stanhu@gmail.com> ``` This patch series addresses the review feedback: 1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists'. 2. cleans up the commit messages to refer to pseudorefs instead of 'special refs'. Stan Hu (2): completion: refactor existence checks for pseudorefs completion: support pseudoref existence checks for reftables contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) -- 2.42.0 ``` ## Stan Hu, 2023-12-07 06:06 Subject: [PATCH v2 1/2] completion: refactor existence checks for pseudorefs Message-ID: <1c6a747691f36ede4224b6d4c2e0c8fd4c0575fd.1701928891.git.stanhu@gmail.com> URL: https://gitlist.dev/e/1c6a747691f36ede4224b6d4c2e0c8fd4c0575fd.1701928891.git.stanhu%40gmail.com In-Reply-To: ``` In preparation for the reftable backend, this commit introduces a '__git_pseudoref_exists' function that continues to use 'test -f' to determine whether a given pseudoref exists in the local filesystem. Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 13a39ebd2e..9fbdc13f9a 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,6 +122,15 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } +# Runs git in $__git_repo_path to determine whether a ref exists. +# 1: The ref to search +__git_ref_exists () +{ + local ref=$1 + + [ -f "$__git_repo_path/$ref" ] +} + # Removes backslash escaping, single quotes and double quotes from a word, # stores the result in the variable $dequoted_word. # 1: The word to dequote. @@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options _git_cherry_pick () { __git_find_repo_path - if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then + if __git_ref_exists CHERRY_PICK_HEAD; then __gitcomp "$__git_cherry_pick_inprogress_options" return fi @@ -2067,7 +2076,7 @@ _git_log () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_ref_exists MERGE_HEAD; then merge="--merge" fi case "$prev,$cur" in @@ -2934,6 +2943,7 @@ _git_reset () _git_restore () { + __git_find_repo_path case "$prev" in -s) __git_complete_refs @@ -2952,7 +2962,7 @@ _git_restore () __gitcomp_builtin restore ;; *) - if __git rev-parse --verify --quiet HEAD >/dev/null; then + if __git_ref_exists HEAD; then __git_complete_index_file "--modified" fi esac @@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options _git_revert () { __git_find_repo_path - if [ -f "$__git_repo_path"/REVERT_HEAD ]; then + if __git_ref_exists REVERT_HEAD; then __gitcomp "$__git_revert_inprogress_options" return fi @@ -3592,7 +3602,7 @@ __gitk_main () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_ref_exists MERGE_HEAD; then merge="--merge" fi case "$cur" in -- 2.42.0 ``` ## Stan Hu, 2023-12-07 06:06 Subject: [PATCH v2 2/2] completion: support pseudoref existence checks for reftables Message-ID: <9b0e090d91f99a1a0cd78f2a93939630ab44abd1.1701928891.git.stanhu@gmail.com> URL: https://gitlist.dev/e/9b0e090d91f99a1a0cd78f2a93939630ab44abd1.1701928891.git.stanhu%40gmail.com In-Reply-To: ``` In contrib/completion/git-completion.bash, there are a bunch of instances where we read pseudorefs, such as HEAD, MERGE_HEAD, REVERT_HEAD, and others via the filesystem. However, the upcoming reftable refs backend won't use '.git/HEAD' at all but instead will write an invalid refname as placeholder for backwards compatibility, which will break the git-completion script. Update the '__git_pseudoref_exists' function to: 1. Recognize the placeholder '.git/HEAD' written by the reftable backend (its content is specified in the reftable specs). 2. If reftable is in use, use 'git rev-parse' to determine whether the given ref exists. 3. Otherwise, continue to use 'test -f' to check for the ref's filename. Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 39 ++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 9fbdc13f9a..b2e407c7e7 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,12 +122,35 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } -# Runs git in $__git_repo_path to determine whether a ref exists. -# 1: The ref to search -__git_ref_exists () +# Helper function to read the first line of a file into a variable. +# __git_eread requires 2 arguments, the file path and the name of the +# variable, in that order. +# +# This is taken from git-prompt.sh. +__git_eread () +{ + test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1" +} + +# Runs git in $__git_repo_path to determine whether a pseudoref exists. +# 1: The pseudo-ref to search +__git_pseudoref_exists() { local ref=$1 + # If the reftable is in use, we have to shell out to 'git rev-parse' + # to determine whether the ref exists instead of looking directly in + # the filesystem to determine whether the ref exists. Otherwise, use + # Bash builtins since executing Git commands are expensive on some + # platforms. + if __git_eread "$__git_repo_path/HEAD" head; then + b="${head#ref: }" + if [ "$b" == "refs/heads/.invalid" ]; then + __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null + return $? + fi + fi + [ -f "$__git_repo_path/$ref" ] } @@ -1634,7 +1657,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options _git_cherry_pick () { __git_find_repo_path - if __git_ref_exists CHERRY_PICK_HEAD; then + if __git_pseudoref_exists CHERRY_PICK_HEAD; then __gitcomp "$__git_cherry_pick_inprogress_options" return fi @@ -2076,7 +2099,7 @@ _git_log () __git_find_repo_path local merge="" - if __git_ref_exists MERGE_HEAD; then + if __git_pseudoref_exists MERGE_HEAD; then merge="--merge" fi case "$prev,$cur" in @@ -2962,7 +2985,7 @@ _git_restore () __gitcomp_builtin restore ;; *) - if __git_ref_exists HEAD; then + if __git_pseudoref_exists HEAD; then __git_complete_index_file "--modified" fi esac @@ -2973,7 +2996,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options _git_revert () { __git_find_repo_path - if __git_ref_exists REVERT_HEAD; then + if __git_pseudoref_exists REVERT_HEAD; then __gitcomp "$__git_revert_inprogress_options" return fi @@ -3602,7 +3625,7 @@ __gitk_main () __git_find_repo_path local merge="" - if __git_ref_exists MERGE_HEAD; then + if __git_pseudoref_exists MERGE_HEAD; then merge="--merge" fi case "$cur" in -- 2.42.0 ``` ## Patrick Steinhardt, 2023-12-08 08:24 Subject: Re: [PATCH v2 1/2] completion: refactor existence checks for pseudorefs Message-ID: URL: https://gitlist.dev/e/ZXLSpyE5ZW0ANmsi%40tanuki In-Reply-To: <1c6a747691f36ede4224b6d4c2e0c8fd4c0575fd.1701928891.git.stanhu@gmail.com> ``` On Wed, Dec 06, 2023 at 10:06:39PM -0800, Stan Hu wrote: > In preparation for the reftable backend, this commit introduces a > '__git_pseudoref_exists' function that continues to use 'test -f' to > determine whether a given pseudoref exists in the local filesystem. > > Signed-off-by: Stan Hu > --- > contrib/completion/git-completion.bash | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 13a39ebd2e..9fbdc13f9a 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -122,6 +122,15 @@ __git () > ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null > } > > +# Runs git in $__git_repo_path to determine whether a ref exists. > +# 1: The ref to search > +__git_ref_exists () I first thought that you missed Junio's point that `__git_ref_exists` may better be renamed to something lkie `__git_pseudoref_exists`. But you do indeed change the name in the second patch. I'd propose to instead squash the rename into the first patch so that the series becomes easier to read. Patrick ``` ## Stan Hu, 2023-12-19 22:14 Subject: [PATCH v3 0/2] completion: refactor and support reftables backend Message-ID: URL: https://gitlist.dev/e/cover.1703022850.git.stanhu%40gmail.com In-Reply-To: <20231130202404.89791-1-stanhu@gmail.com> ``` This patch series addresses the review feedback: 1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in the first refactor patch. Stan Hu (2): completion: refactor existence checks for pseudorefs completion: support pseudoref existence checks for reftables contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 5 deletions(-) -- 2.42.0 ``` ## Stan Hu, 2023-12-19 22:14 Subject: [PATCH v3 1/2] completion: refactor existence checks for pseudorefs Message-ID: <29c928649aba7dfce4dab1b5d923bc23b7af2d32.1703022850.git.stanhu@gmail.com> URL: https://gitlist.dev/e/29c928649aba7dfce4dab1b5d923bc23b7af2d32.1703022850.git.stanhu%40gmail.com In-Reply-To: ``` In preparation for the reftable backend, this commit introduces a '__git_pseudoref_exists' function that continues to use 'test -f' to determine whether a given pseudoref exists in the local filesystem. Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 13a39ebd2e..8edd002eed 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,6 +122,15 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } +# Runs git in $__git_repo_path to determine whether a pseudoref exists. +# 1: The pseudo-ref to search +__git_pseudoref_exists () +{ + local ref=$1 + + [ -f "$__git_repo_path/$ref" ] +} + # Removes backslash escaping, single quotes and double quotes from a word, # stores the result in the variable $dequoted_word. # 1: The word to dequote. @@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options _git_cherry_pick () { __git_find_repo_path - if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then + if __git_pseudoref_exists CHERRY_PICK_HEAD; then __gitcomp "$__git_cherry_pick_inprogress_options" return fi @@ -2067,7 +2076,7 @@ _git_log () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_pseudoref_exists MERGE_HEAD; then merge="--merge" fi case "$prev,$cur" in @@ -2934,6 +2943,7 @@ _git_reset () _git_restore () { + __git_find_repo_path case "$prev" in -s) __git_complete_refs @@ -2952,7 +2962,7 @@ _git_restore () __gitcomp_builtin restore ;; *) - if __git rev-parse --verify --quiet HEAD >/dev/null; then + if __git_pseudoref_exists HEAD; then __git_complete_index_file "--modified" fi esac @@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options _git_revert () { __git_find_repo_path - if [ -f "$__git_repo_path"/REVERT_HEAD ]; then + if __git_pseudoref_exists REVERT_HEAD; then __gitcomp "$__git_revert_inprogress_options" return fi @@ -3592,7 +3602,7 @@ __gitk_main () __git_find_repo_path local merge="" - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then + if __git_pseudoref_exists MERGE_HEAD; then merge="--merge" fi case "$cur" in -- 2.42.0 ``` ## Stan Hu, 2023-12-19 22:14 Subject: [PATCH v3 2/2] completion: support pseudoref existence checks for reftables Message-ID: URL: https://gitlist.dev/e/e25a0c5c84dfeca7067fe803388ea3bcee58860c.1703022850.git.stanhu%40gmail.com In-Reply-To: ``` In contrib/completion/git-completion.bash, there are a bunch of instances where we read pseudorefs, such as HEAD, MERGE_HEAD, REVERT_HEAD, and others via the filesystem. However, the upcoming reftable refs backend won't use '.git/HEAD' at all but instead will write an invalid refname as placeholder for backwards compatibility, which will break the git-completion script. Update the '__git_pseudoref_exists' function to: 1. Recognize the placeholder '.git/HEAD' written by the reftable backend (its content is specified in the reftable specs). 2. If reftable is in use, use 'git rev-parse' to determine whether the given ref exists. 3. Otherwise, continue to use 'test -f' to check for the ref's filename. Signed-off-by: Stan Hu --- contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 8edd002eed..e21a39b406 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -122,12 +122,35 @@ __git () ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null } +# Helper function to read the first line of a file into a variable. +# __git_eread requires 2 arguments, the file path and the name of the +# variable, in that order. +# +# This is taken from git-prompt.sh. +__git_eread () +{ + test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1" +} + # Runs git in $__git_repo_path to determine whether a pseudoref exists. # 1: The pseudo-ref to search __git_pseudoref_exists () { local ref=$1 + # If the reftable is in use, we have to shell out to 'git rev-parse' + # to determine whether the ref exists instead of looking directly in + # the filesystem to determine whether the ref exists. Otherwise, use + # Bash builtins since executing Git commands are expensive on some + # platforms. + if __git_eread "$__git_repo_path/HEAD" head; then + b="${head#ref: }" + if [ "$b" == "refs/heads/.invalid" ]; then + __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null + return $? + fi + fi + [ -f "$__git_repo_path/$ref" ] } -- 2.42.0 ``` ## Junio C Hamano, 2023-12-20 00:48 Subject: Re: [PATCH v3 0/2] completion: refactor and support reftables backend Message-ID: URL: https://gitlist.dev/e/xmqqsf3xu2ai.fsf%40gitster.g In-Reply-To: ``` Stan Hu writes: > This patch series addresses the review feedback: > > 1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in > the first refactor patch. > > Stan Hu (2): > completion: refactor existence checks for pseudorefs > completion: support pseudoref existence checks for reftables > > contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 5 deletions(-) Thanks. Will queue. ``` ## SZEDER Gábor, 2024-01-13 21:07 Subject: Re: [PATCH v3 1/2] completion: refactor existence checks for pseudorefs Message-ID: <20240113210714.GC3000857@szeder.dev> URL: https://gitlist.dev/e/20240113210714.GC3000857%40szeder.dev In-Reply-To: <29c928649aba7dfce4dab1b5d923bc23b7af2d32.1703022850.git.stanhu@gmail.com> ``` On Tue, Dec 19, 2023 at 02:14:17PM -0800, Stan Hu wrote: > In preparation for the reftable backend, this commit introduces a > '__git_pseudoref_exists' function that continues to use 'test -f' to > determine whether a given pseudoref exists in the local filesystem. > > Signed-off-by: Stan Hu > --- > contrib/completion/git-completion.bash | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index 13a39ebd2e..8edd002eed 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -122,6 +122,15 @@ __git () > ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null > } > > +# Runs git in $__git_repo_path to determine whether a pseudoref exists. > +# 1: The pseudo-ref to search > +__git_pseudoref_exists () > +{ > + local ref=$1 > + > + [ -f "$__git_repo_path/$ref" ] This new helper function relies on $__git_repo_path being set, but it doesn't ensure that it's actually set by calling __git_find_repo_path. Instead, it relies on its callers calling __git_find_repo_path, although after this patch none of those callers directly access $__git_repo_path anymore. It would be better to call __git_find_repo_path in this helper to make it more self-contained, and then the now unnecessary __git_find_repo_path calls from the three callers can be removed. Yeah, I know it's late, because this patch is already in master, but this would be a good preparation step for eventual dedicated tests of __git_pseudoref_exists that came up in: https://public-inbox.org/git/20240113191749.GB3000857@szeder.dev/ > +} > + > # Removes backslash escaping, single quotes and double quotes from a word, > # stores the result in the variable $dequoted_word. > # 1: The word to dequote. > @@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options > _git_cherry_pick () > { > __git_find_repo_path > - if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then > + if __git_pseudoref_exists CHERRY_PICK_HEAD; then > __gitcomp "$__git_cherry_pick_inprogress_options" > return > fi > @@ -2067,7 +2076,7 @@ _git_log () > __git_find_repo_path > > local merge="" > - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then > + if __git_pseudoref_exists MERGE_HEAD; then > merge="--merge" > fi > case "$prev,$cur" in > @@ -2934,6 +2943,7 @@ _git_reset () > > _git_restore () > { > + __git_find_repo_path > case "$prev" in > -s) > __git_complete_refs > @@ -2952,7 +2962,7 @@ _git_restore () > __gitcomp_builtin restore > ;; > *) > - if __git rev-parse --verify --quiet HEAD >/dev/null; then > + if __git_pseudoref_exists HEAD; then > __git_complete_index_file "--modified" > fi > esac > @@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options > _git_revert () > { > __git_find_repo_path > - if [ -f "$__git_repo_path"/REVERT_HEAD ]; then > + if __git_pseudoref_exists REVERT_HEAD; then > __gitcomp "$__git_revert_inprogress_options" > return > fi > @@ -3592,7 +3602,7 @@ __gitk_main () > __git_find_repo_path > > local merge="" > - if [ -f "$__git_repo_path/MERGE_HEAD" ]; then > + if __git_pseudoref_exists MERGE_HEAD; then > merge="--merge" > fi > case "$cur" in > -- > 2.42.0 > > ```