From 3bf5ccf429561a1fbd64cc55b771e31aae15065c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Jan 2024 11:35:57 +0100 Subject: completion: discover repo path in `__git_pseudoref_exists ()` The helper function `__git_pseudoref_exists ()` expects that the repo path has already been discovered by its callers, which makes for a rather fragile calling convention. Refactor the function to discover the repo path itself to make it more self-contained, which also removes the need to discover the path in some of its callers. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'contrib/completion/git-completion.bash') diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 8c40ade494..06a9107449 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -138,6 +138,8 @@ __git_pseudoref_exists () { local ref=$1 + __git_find_repo_path + # 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 @@ -1656,7 +1658,6 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options _git_cherry_pick () { - __git_find_repo_path if __git_pseudoref_exists CHERRY_PICK_HEAD; then __gitcomp "$__git_cherry_pick_inprogress_options" return @@ -2966,7 +2967,6 @@ _git_reset () _git_restore () { - __git_find_repo_path case "$prev" in -s) __git_complete_refs @@ -2995,7 +2995,6 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options _git_revert () { - __git_find_repo_path if __git_pseudoref_exists REVERT_HEAD; then __gitcomp "$__git_revert_inprogress_options" return -- cgit v1.3 From 7b9cda2d3d7de794974ce6f29a2a838565cd338d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Jan 2024 11:36:06 +0100 Subject: completion: improve existence check for pseudo-refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improve the existence check along the following lines: - Stop stripping the "ref :" prefix and compare to the expected value directly. This allows us to drop a now-unused variable that was previously leaking into the user's shell. - Mark the "head" variable as local so that we don't leak its value into the user's shell. - Stop manually handling the `-C $__git_repo_path` option, which the `__git ()` wrapper aleady does for us. - In simlar spirit, stop redirecting stderr, which is also handled by the wrapper already. Suggested-by: SZEDER Gábor Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'contrib/completion/git-completion.bash') diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 06a9107449..d703e3e64f 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -137,6 +137,7 @@ __git_eread () __git_pseudoref_exists () { local ref=$1 + local head __git_find_repo_path @@ -146,9 +147,8 @@ __git_pseudoref_exists () # 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 + if [ "$head" == "ref: refs/heads/.invalid" ]; then + __git rev-parse --verify --quiet "$ref" return $? fi fi -- cgit v1.3 From 9a9c31135e6029dbda773f7271cea4648644eb8e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Jan 2024 11:36:11 +0100 Subject: completion: silence pseudoref existence check In 44dbb3bf29 (completion: support pseudoref existence checks for reftables, 2023-12-19), we have extended the Bash completion script to support future ref backends better by using git-rev-parse(1) to check for pseudo-ref existence. This conversion has introduced a bug, because even though we pass `--quiet` to git-rev-parse(1) it would still output the resolved object ID of the ref in question if it exists. Fix this by redirecting its stdout to `/dev/null` and add a test that catches this behaviour. Note that the test passes even without the fix for the "files" backend because we parse pseudo refs via the filesystem directly in that case. But the test will fail with the "reftable" backend. Helped-by: Jeff King Helped-by: Johannes Schindelin Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 2 +- t/t9902-completion.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) (limited to 'contrib/completion/git-completion.bash') diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index d703e3e64f..54ce58f73d 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -148,7 +148,7 @@ __git_pseudoref_exists () # platforms. if __git_eread "$__git_repo_path/HEAD" head; then if [ "$head" == "ref: refs/heads/.invalid" ]; then - __git rev-parse --verify --quiet "$ref" + __git rev-parse --verify --quiet "$ref" >/dev/null return $? fi fi diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 95ec762a74..56dc7343a2 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1933,6 +1933,14 @@ test_expect_success 'git checkout - --orphan with branch already provided comple EOF ' +test_expect_success 'git restore completes modified files' ' + test_commit A a.file && + echo B >a.file && + test_completion "git restore a." <<-\EOF + a.file + EOF +' + test_expect_success 'teardown after ref completion' ' git branch -d matching-branch && git tag -d matching-tag && @@ -2728,4 +2736,27 @@ test_expect_success '__git_complete' ' test_must_fail __git_complete ga missing ' +test_expect_success '__git_pseudoref_exists' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + sane_unset __git_repo_path && + + # HEAD points to an existing branch, so it should exist. + test_commit A && + __git_pseudoref_exists HEAD >output 2>&1 && + test_must_be_empty output && + + # CHERRY_PICK_HEAD does not exist, so the existence check should fail. + ! __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 && + test_must_be_empty output && + + # CHERRY_PICK_HEAD points to a commit, so it should exist. + git update-ref CHERRY_PICK_HEAD A && + __git_pseudoref_exists CHERRY_PICK_HEAD >output 2>&1 && + test_must_be_empty output + ) +' + test_done -- cgit v1.3 From 020e0a087f2101182343936b8d58f0b4c96e96df Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Jan 2024 11:36:15 +0100 Subject: completion: treat dangling symrefs as existing pseudorefs The `__git_pseudoref_exists ()` helper function back to git-rev-parse(1) in case the reftable backend is in use. This is not in the same spirit as the simple existence check that the "files" backend does though, because there we only check for the pseudo-ref to exist with `test -f`. With git-rev-parse(1) we not only check for existence, but also verify that the pseudo-ref resolves to an object, which may not be the case when the pseudo-ref points to an unborn branch. Fix this issue by using `git show-ref --exists` instead. Note that we do not have to silence stdout anymore as git-show-ref(1) will not print anything. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- contrib/completion/git-completion.bash | 2 +- t/t9902-completion.sh | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'contrib/completion/git-completion.bash') diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 54ce58f73d..6662db221d 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -148,7 +148,7 @@ __git_pseudoref_exists () # platforms. if __git_eread "$__git_repo_path/HEAD" head; then if [ "$head" == "ref: refs/heads/.invalid" ]; then - __git rev-parse --verify --quiet "$ref" >/dev/null + __git show-ref --exists "$ref" return $? fi fi diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 56dc7343a2..35eb534fdd 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2743,6 +2743,10 @@ test_expect_success '__git_pseudoref_exists' ' cd repo && sane_unset __git_repo_path && + # HEAD should exist, even if it points to an unborn branch. + __git_pseudoref_exists HEAD >output 2>&1 && + test_must_be_empty output && + # HEAD points to an existing branch, so it should exist. test_commit A && __git_pseudoref_exists HEAD >output 2>&1 && -- cgit v1.3