| Message ID | 20201219140621.1961760-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | completion: bash: a bunch of fixes | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > This is just the bug fixes from the previous series. > > These should be pretty obvious and straightforward. > > Felipe Contreras (4): > completion: bash: fix prefix detection in branch.* > completion: bash: add correct suffix in variables > completion: bash: fix for suboptions with value > completion: bash: fix for multiple dash commands It seems that this tickles some platform specific glitches in the tests (the detailed CI report can only be seen when logged in, it seems): https://github.com/git/git/runs/1597682180#step:5:35614 The expected output lines have a trailing SP each, while the actual output lines do not, but this failure happens only on macos job and not others.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > This is just the bug fixes from the previous series. > > > > These should be pretty obvious and straightforward. > > > > Felipe Contreras (4): > > completion: bash: fix prefix detection in branch.* > > completion: bash: add correct suffix in variables > > completion: bash: fix for suboptions with value > > completion: bash: fix for multiple dash commands > > It seems that this tickles some platform specific glitches in the > tests (the detailed CI report can only be seen when logged in, it > seems): > > https://github.com/git/git/runs/1597682180#step:5:35614 I found that output very hard to parse, but I think I understand the issue. It's interesting. Apaprently macOS uses zsh by default, and in zsh, this: local sfx echo "'${sfx- }'" Prints an empty string. That's because in zsh "local sfx" is effectively "local sfx=''" which in my opinion is a bug. I did catch this issue for zsh some time ago. I contacted the zsh developers a while ago, and that triggered a huge discussion, since they don't consider it a bug, but they are working on tentative changes. That's another story though. My previous series with 26 patches didn't trigger that issue because I have fixes on top of of __gitcomp so suffixes work correctly, and the code can eventually be changed to: local sfx=" " That makes the completion work correctly for both bash and zsh. I see 5 courses of action: 1. Drop the offending patch: this is wrong because the bug is still there, we are just not checking for it. 2. Add a BASH prereq just for that test, or test_expect_unstable (we would need to add extra code for both of those). 3. Add the fix, but not the test for the fix. 4. Backport my __gitcomp to make the code work for both shells. 5. Update the patch series to include all the changes up to the point that is fixed. For me obviously 1 and 5 require the least work, but in 1 the bug is still there, and in 5 the patches might get stuck more time than necessary. Either way I think the real problem is that not enough eyes are looking at these patches, so it's unclear if and when they will be merged. The issues are real though, and they are present in the current code. Cheers.
On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote: > Junio C Hamano wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > > > This is just the bug fixes from the previous series. > > > > > > These should be pretty obvious and straightforward. > > > > > > Felipe Contreras (4): > > > completion: bash: fix prefix detection in branch.* > > > completion: bash: add correct suffix in variables > > > completion: bash: fix for suboptions with value > > > completion: bash: fix for multiple dash commands > > > > It seems that this tickles some platform specific glitches in the > > tests (the detailed CI report can only be seen when logged in, it > > seems): > > > > https://github.com/git/git/runs/1597682180#step:5:35614 > > I found that output very hard to parse, but I think I understand the > issue. It's interesting. > > Apaprently macOS uses zsh by default, The completion and prompt tests are suppsed to be run by Bash, no matter what the default shell, or are skipped, or the setup is broken (a non-Bash shell sets $BASH, or 'exec bash' runs zsh). Our CI jobs use the default Bash version, which in the macOS jobs is v3.2. > and in zsh, this: > > local sfx > echo "'${sfx- }'" > > Prints an empty string. > > That's because in zsh "local sfx" is effectively "local sfx=''" which in > my opinion is a bug. Bash versions up to 4.0-alpha suffered from this bug as well; I believe the relevant changelog entry for 4.0-beta is this: e. Fixed a bug that caused local variables to be created with the empty string for a value rather than no value. So the default Bash version on macOS still has this bug, thus __gitcomp_nl_append() is invoked with an empty string sfx parameter instead of a space, causing the test failure. I can reproduce the test failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it passes with v4.0 and later versions. > I see 5 courses of action: > > 1. Drop the offending patch: this is wrong because the bug is still > there, we are just not checking for it. > 2. Add a BASH prereq just for that test, or test_expect_unstable (we > would need to add extra code for both of those). > 3. Add the fix, but not the test for the fix. I'm for this option 3: this patch does fix a bug for users of Bash v4.0 or later, while it doesn't change the behavior with v3.2 or earlier (and with zsh, if I understand correctly). OTOH, the test doesn't seem to be all that useful: while it does demonstrate the issue, it checks only one of those callsites that passed the wrong suffix, and, more importantly, it doesn't protect us from adding another callsites with similarly wrong suffex in the future. In any case, the commit message should note that the fix doesn't work with all Bash versions and why.
SZEDER Gábor wrote: > On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote: > > and in zsh, this: > > > > local sfx > > echo "'${sfx- }'" > > > > Prints an empty string. > > > > That's because in zsh "local sfx" is effectively "local sfx=''" which in > > my opinion is a bug. > > Bash versions up to 4.0-alpha suffered from this bug as well; I believe > the relevant changelog entry for 4.0-beta is this: > > e. Fixed a bug that caused local variables to be created with the empty > string for a value rather than no value. > > So the default Bash version on macOS still has this bug, thus > __gitcomp_nl_append() is invoked with an empty string sfx parameter > instead of a space, causing the test failure. I can reproduce the > test failure on Linux using Bash v3.2 (and v3.1 and v3.0), and it > passes with v4.0 and later versions. Ahhh, interesting. So I'm not the only one who considers it a bug. > > I see 5 courses of action: > > > > 1. Drop the offending patch: this is wrong because the bug is still > > there, we are just not checking for it. > > 2. Add a BASH prereq just for that test, or test_expect_unstable (we > > would need to add extra code for both of those). > > 3. Add the fix, but not the test for the fix. > > I'm for this option 3: this patch does fix a bug for users of Bash > v4.0 or later, while it doesn't change the behavior with v3.2 or > earlier (and with zsh, if I understand correctly). OTOH, the test > doesn't seem to be all that useful: while it does demonstrate the > issue, it checks only one of those callsites that passed the wrong > suffix, and, more importantly, it doesn't protect us from adding > another callsites with similarly wrong suffex in the future. I'm fine with that option. > In any case, the commit message should note that the fix doesn't work > with all Bash versions and why. Yes. I will send a re-roll (unless somebody objects beforehand).
SZEDER Gábor <szeder.dev@gmail.com> writes: > On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote: > ... >> I see 5 courses of action: >> >> 1. Drop the offending patch: this is wrong because the bug is still >> there, we are just not checking for it. >> 2. Add a BASH prereq just for that test, or test_expect_unstable (we >> would need to add extra code for both of those). >> 3. Add the fix, but not the test for the fix. > > I'm for this option 3: this patch does fix a bug for users of Bash > v4.0 or later, while it doesn't change the behavior with v3.2 or > earlier (and with zsh, if I understand correctly). OTOH, the test > doesn't seem to be all that useful: while it does demonstrate the > issue, it checks only one of those callsites that passed the wrong > suffix, and, more importantly, it doesn't protect us from adding > another callsites with similarly wrong suffex in the future. Yeah, I might have preferred, if we didn't read your "doesn't seem to be all that useful", to keep the test with prereq on bash 4, but I think either way is fine. > In any case, the commit message should note that the fix doesn't work > with all Bash versions and why. Very true. Thanks, all.
Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > On Wed, Dec 23, 2020 at 07:38:12AM -0600, Felipe Contreras wrote: > > ... > >> I see 5 courses of action: > >> > >> 1. Drop the offending patch: this is wrong because the bug is still > >> there, we are just not checking for it. > >> 2. Add a BASH prereq just for that test, or test_expect_unstable (we > >> would need to add extra code for both of those). > >> 3. Add the fix, but not the test for the fix. > > > > I'm for this option 3: this patch does fix a bug for users of Bash > > v4.0 or later, while it doesn't change the behavior with v3.2 or > > earlier (and with zsh, if I understand correctly). OTOH, the test > > doesn't seem to be all that useful: while it does demonstrate the > > issue, it checks only one of those callsites that passed the wrong > > suffix, and, more importantly, it doesn't protect us from adding > > another callsites with similarly wrong suffex in the future. > > Yeah, I might have preferred, if we didn't read your "doesn't seem > to be all that useful", to keep the test with prereq on bash 4, but > I think either way is fine. Even if we add a prereq on BASH4, he is right that the test wouldn't be all that useful, because it's checking only for one conditional branch, and the function has quite a few, from the top of my head there are about 10. A more useuful test would at least add one check for each one of the cases. It still would be dependent on the current implementation, but would be more useful. I can add that in a future patch series, once the other issues are resolved. Cheers.