threads / patch / 60574
Subject: [PATCH 0/2] completion: refactor and support reftables backend
Stan HuHi,
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[PATCH 1/2] completion: refactor existence checks for special refs
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 <ps@pks.im> Reviewed-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Stan Hu <stanhu@gmail.com> --- 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[PATCH 2/2] completion: stop checking for reference existence via `test -f`
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 <ps@pks.im> Reviewed-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Stan Hu <stanhu@gmail.com> --- 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 SteinhardtRe: [PATCH 2/2] completion: stop checking for reference existence via `test -f`
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 HamanoRe: [PATCH 1/2] completion: refactor existence checks for special refs
Stan Hu <stanhu@gmail.com> 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 <ps@pks.im>
> Reviewed-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Stan Hu <stanhu@gmail.com>
> ---
> 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" ]
> +}Patrick SteinhardtRe: [PATCH 0/2] completion: refactor and support reftables backend
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 >
Stan Hu[PATCH v2 0/2] completion: refactor and support reftables backend
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[PATCH v2 1/2] completion: refactor existence checks for pseudorefs
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 <stanhu@gmail.com> --- 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
Patrick SteinhardtRe: [PATCH v2 1/2] completion: refactor existence checks for pseudorefs
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 <stanhu@gmail.com>
> ---
> 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[PATCH v2 2/2] completion: support pseudoref existence checks for reftables
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 <stanhu@gmail.com> --- 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
Stan Hu[PATCH v3 0/2] completion: refactor and support reftables backend
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[PATCH v3 1/2] completion: refactor existence checks for pseudorefs
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 <stanhu@gmail.com> --- 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
SZEDER GáborRe: [PATCH v3 1/2] completion: refactor existence checks for pseudorefs
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 <stanhu@gmail.com>
> ---
> 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
>
> Stan Hu[PATCH v3 2/2] completion: support pseudoref existence checks for reftables
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 <stanhu@gmail.com> --- 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 HamanoRe: [PATCH v3 0/2] completion: refactor and support reftables backend
Stan Hu <stanhu@gmail.com> 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.