| Message ID | pull.1374.git.git.1667669315.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add case insensitivity option to bash completion | expand |
Hi Alison, On Sat, Nov 05, 2022 at 05:28:33PM +0000, Alison Winters via GitGitGadget wrote: > Alison Winters (2): > completion: add optional ignore-case when matching refs > completion: add case-insensitive match of pseudorefs > > contrib/completion/git-completion.bash | 51 ++++++++++++++++++++++++-- > t/t9902-completion.sh | 32 ++++++++++++++++ > 2 files changed, 80 insertions(+), 3 deletions(-) It looks reasonable to me, but I'm far from an expert on the completion code ;-). CC'ing some folks who are going to be more familiar with it than me. Thanks, Taylor
"Alison Winters via GitGitGadget" <gitgitgadget@gmail.com> writes: > In 3bb16a8bf2 (tag, branch, for-each-ref: add --ignore-case for sorting and > filtering, 2016-12-04), support was added for filtering and sorting refs in > a case insensitive way. This is a behavior that seems appropriate to enable > with shell completion. Many shells provide case insensitive completion as an > option, even on filesystems that remain case sensitive. > > This patch adds a new variable that, when set, will allow Bash completion to > use the --ignore-case option to match refs. Additionally, some basic support > is implemented to match pseudorefs like HEAD. I did not try to figure out the reason but the topic with its tests seem to break in 'seen' the linux-cmake-ctest CI job. https://github.com/git/git/actions/runs/3570230611/jobs/6001013549 but the same test does not break under usual "make test". Can people who are interested in the cmake-ctest stuff take a look? It is tempting to eject the ab/cmake-nix-and-ci topic that is already in 'next', under the theory that what that topic does to the tests "works" for some tests but not for others, and this topic is an unfortunate collateral damage whose tests weren't something the other topic supports well. If the cmake-ctest stuff is in such a shape, then it may have been a bit premature to merge it down. Thanks.
On 2022-11-29 10:38, Junio C Hamano wrote: > I did not try to figure out the reason but the topic with its tests > seem to break in 'seen' the linux-cmake-ctest CI job. > > https://github.com/git/git/actions/runs/3570230611/jobs/6001013549 > > but the same test does not break under usual "make test". I cannot speak for how the changes of the ab/cmake-nix-and-ci impact the aw/complete-case-insensitive branch, but the failure seems to be pointing to a test that I have since changed in the v2 patch, on the suggestion of Szeder Gabor. The variable is no longer exported and the script is no longer sourced a second time. I don't know if those v2 changes would change the results of this test, but they might be a starting point for the other people CC:ed here to consider. Alison
On Tue, Nov 29 2022, Alison Winters wrote: > On 2022-11-29 10:38, Junio C Hamano wrote: >> I did not try to figure out the reason but the topic with its tests >> seem to break in 'seen' the linux-cmake-ctest CI job. >> >> https://github.com/git/git/actions/runs/3570230611/jobs/6001013549 >> >> but the same test does not break under usual "make test". > > I cannot speak for how the changes of the ab/cmake-nix-and-ci impact > the aw/complete-case-insensitive branch, but the failure seems to be > pointing to a test that I have since changed in the v2 patch, on the > suggestion of Szeder Gabor. The variable is no longer exported and > the script is no longer sourced a second time. I don't know if those > v2 changes would change the results of this test, but they might be a > starting point for the other people CC:ed here to consider. The breakage is due to a semantic conflict between the two, which is solved thusly: diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index d45d820da28..2aa128075ca 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2261,7 +2261,7 @@ test_expect_success 'checkout does not match ref names of a different case' ' test_expect_success 'checkout matches case insensitively with GIT_COMPLETION_IGNORE_CASE' ' ( - . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && + . "$GIT_SOURCE_DIR/contrib/completion/git-completion.bash" && GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE && test_completion "git checkout M" <<-\EOF main Z @@ -2279,7 +2279,7 @@ test_expect_success 'checkout completes pseudo refs' ' test_expect_success 'checkout completes pseudo refs case insensitively with GIT_COMPLETION_IGNORE_CASE' ' ( - . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && + . "$GIT_SOURCE_DIR/contrib/completion/git-completion.bash" && GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE && test_completion "git checkout h" <<-\EOF HEAD Z Junio: I don't think this warrants kicking out the cmake topic. This sort of problem is just going to occur while something like that is in-flight, or Alison would have otherwise presumably seen it as a CI failure if based off "master". With hindsight I could have made e74e51a9154 (cmake & test-lib.sh: add a $GIT_SOURCE_DIR variable, 2022-11-03) do the migration in two phases, but I didn't expect another topic to rely on the relatively obscure bits that were being copied to the $GIT_BUILD_DIR, we weren't exactly drowning in git-completion.bash patches...
On 2022-11-30 01:40, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Nov 29 2022, Alison Winters wrote: > >> On 2022-11-29 10:38, Junio C Hamano wrote: >>> I did not try to figure out the reason but the topic with its tests >>> seem to break in 'seen' the linux-cmake-ctest CI job. >>> >>> https://github.com/git/git/actions/runs/3570230611/jobs/6001013549 >>> >>> but the same test does not break under usual "make test". >> >> I cannot speak for how the changes of the ab/cmake-nix-and-ci impact >> the aw/complete-case-insensitive branch, but the failure seems to be >> pointing to a test that I have since changed in the v2 patch, on the >> suggestion of Szeder Gabor. The variable is no longer exported and >> the script is no longer sourced a second time. I don't know if those >> v2 changes would change the results of this test, but they might be a >> starting point for the other people CC:ed here to consider. > > The breakage is due to a semantic conflict between the two, which is > solved thusly: > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index d45d820da28..2aa128075ca 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -2261,7 +2261,7 @@ test_expect_success 'checkout does not match ref names of a different case' ' > > test_expect_success 'checkout matches case insensitively with GIT_COMPLETION_IGNORE_CASE' ' > ( > - . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && > + . "$GIT_SOURCE_DIR/contrib/completion/git-completion.bash" && > GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE && > test_completion "git checkout M" <<-\EOF > main Z > @@ -2279,7 +2279,7 @@ test_expect_success 'checkout completes pseudo refs' ' > > test_expect_success 'checkout completes pseudo refs case insensitively with GIT_COMPLETION_IGNORE_CASE' ' > ( > - . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" && > + . "$GIT_SOURCE_DIR/contrib/completion/git-completion.bash" && > GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE && > test_completion "git checkout h" <<-\EOF > HEAD Z In this case, I think the easiest solution will be to move the aw/complete-case-insensitive branch forward to the v2 patch (posted to the list @ 2022-11-21 0:26 UTC), which no longer includes this line. Alison
Alison Winters <alisonatwork@outlook.com> writes: > In this case, I think the easiest solution will be to move the > aw/complete-case-insensitive branch forward to the v2 patch (posted to > the list @ 2022-11-21 0:26 UTC), which no longer includes this line. Indeed. With <pull.1374.v2.git.git.1668990419.gitgitgadget@gmail.com> merged on top of a known-good subset of 'seen', cmake-ctest thing did not fail. https://github.com/git/git/actions/runs/3579752650/jobs/6021251201 Thanks. P.S. Ignore the osx-gcc job that was cancelled in the same run; as this is not the only topic I have to deal with, I submitted a trial merge into 'seen' of another topic after seeing cmake-ctest job finish, but because of recent CI change to cancel an earlier job when a new job is started, it got auto-cancelled (which is mostly a good thing for regular developer workflow, but not for me X-<)