| Message ID | 84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SoC 2024: clarify `test_path_is_*` conversion microproject | expand |
On Mon, Mar 4, 2024 at 10:17 AM Patrick Steinhardt <ps@pks.im> wrote: > > One of our proposed microprojects is to convert instances of `test -e` > and related functions to instead use `test_path_exists` or similar. This > conversion is only feasible when `test -e` is not used as part of a > control statement, as the replacement is used to _assert_ a condition > instead of merely testing for it. > > Clarify the microproject's description accordingly. Applied and pushed, thanks! I wonder if it would be better to create a PR in https://github.com/git/git.github.io/ and perhaps just send a link to it, rather than sending patches to the mailing list, as patches on the mailing list could be mistaken by tools and perhaps people as applying to the Git code base.
Patrick Steinhardt <ps@pks.im> writes: > One of our proposed microprojects is to convert instances of `test -e` > and related functions to instead use `test_path_exists` or similar. This > conversion is only feasible when `test -e` is not used as part of a > control statement, as the replacement is used to _assert_ a condition > instead of merely testing for it. > > Clarify the microproject's description accordingly. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > SoC-2024-Microprojects.md | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md > index 644c0a6..782441f 100644 > --- a/SoC-2024-Microprojects.md > +++ b/SoC-2024-Microprojects.md > @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea. > Find one test script that verifies the presence/absence of > files/directories with 'test -(e|f|d|...)' and replace them with the > appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper > -functions. > +functions. Note that this conversion does not directly apply to control > +flow constructs like `if test -e ./path; then ...; fi` because the > +replacements are intended to assert the condition instead of merely > +testing for it. Thanks for picking it up. Of course there is one case in which we should use test_path_* helpers to replace such an if...then...fi construct; e.g., c431a235 (t9146: replace test -d/-e/-f with appropriate test_path_is_* function, 2024-02-14) did exactly that. I am not sure how best to express that in the already crowded description above, though. Rewriting the existing test this way Find one test script that uses 'test [!] -(e|f|d|...)' to assert the presence/absense of files/directories to make the test fail directly with the exit status of such "test" commands, and replace them with the appropriate helper functions like `test_path_is_file`, that give more informative error messages when they fail. would exclude use of "test -e" as a conditional in control statements, so we could mention what c431a235 did as an exception to the rule, perhaps like Note that the above excludes "test -f" and friends used as a condition in control statements such as "if test -e path ...", but as an exception, if such a "if" statement just open-codes what these helpers do, replacing it is warranted. But that does not read very well, even to myself. Sigh.... Thanks.
diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md index 644c0a6..782441f 100644 --- a/SoC-2024-Microprojects.md +++ b/SoC-2024-Microprojects.md @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea. Find one test script that verifies the presence/absence of files/directories with 'test -(e|f|d|...)' and replace them with the appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper -functions. +functions. Note that this conversion does not directly apply to control +flow constructs like `if test -e ./path; then ...; fi` because the +replacements are intended to assert the condition instead of merely +testing for it. If you can't find one please tell us, along with the command you used to search, so that we can remove this microproject idea.
One of our proposed microprojects is to convert instances of `test -e` and related functions to instead use `test_path_exists` or similar. This conversion is only feasible when `test -e` is not used as part of a control statement, as the replacement is used to _assert_ a condition instead of merely testing for it. Clarify the microproject's description accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- SoC-2024-Microprojects.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)