| Message ID | YI12n+1rbedpwyfW@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leftover bits from symlinked gitattributes, etc topics | expand |
On Sat, May 01 2021, Jeff King wrote: > Since GETTEXT_POISON does not exist anymore, there is no point warning > people about whether we should use test_i18ngrep. This is doubly > confusing because the comment was describing why it was OK to use grep, > but it got caught up in the mass conversion of 674ba34038 (fsck: mark > strings for translation, 2018-11-10). > > Note there are other uses of test_i18ngrep in this script which are now > obsolete; I'll save those for a mass-cleanup. My goal here was just to > fix the confusing comment in code I'm about to refactor. For what it's worth between [1] and [2] I'm not sure what to do about the test_i18ngrep cleanup. I think your patch below is fine, but the "test_i18ngrep" has mutated into a "grep with debugging", not just something needed for GETTEXT_POISON. So that part of your patch right now is making it less friendly for debugging. I don't care, and think if we want that we'd be better of scraping the trace ouput for such common cases and/or use "verbose grep [...]" and teach the "verbose" wrapper about these common cases, but knowing of that objection + having other outstanding things has been why I haven't sent s/test_i18ngrep/grep/g patches. 1. https://lore.kernel.org/git/20210120152132.GC8396@szeder.dev/ 2. https://lore.kernel.org/git/20210120182759.31102-1-avarab@gmail.com/ > Signed-off-by: Jeff King <peff@peff.net> > --- > t/t7415-submodule-names.sh | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh > index f70368bc2e..fef6561d80 100755 > --- a/t/t7415-submodule-names.sh > +++ b/t/t7415-submodule-names.sh > @@ -151,10 +151,9 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' > } | git mktree && > > # Check not only that we fail, but that it is due to the > - # symlink detector; this grep string comes from the config > - # variable name and will not be translated. > + # symlink detector > test_must_fail git fsck 2>output && > - test_i18ngrep gitmodulesSymlink output > + grep gitmodulesSymlink output > ) > '
On Mon, May 03, 2021 at 11:46:52AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Since GETTEXT_POISON does not exist anymore, there is no point warning > > people about whether we should use test_i18ngrep. This is doubly > > confusing because the comment was describing why it was OK to use grep, > > but it got caught up in the mass conversion of 674ba34038 (fsck: mark > > strings for translation, 2018-11-10). > > > > Note there are other uses of test_i18ngrep in this script which are now > > obsolete; I'll save those for a mass-cleanup. My goal here was just to > > fix the confusing comment in code I'm about to refactor. > > For what it's worth between [1] and [2] I'm not sure what to do about > the test_i18ngrep cleanup. I think your patch below is fine, but the > "test_i18ngrep" has mutated into a "grep with debugging", not just > something needed for GETTEXT_POISON. Yes, I can see some value in that. Though if that's what it's going to be, IMHO we should use it consistently and give a better name (test_grep or something). > So that part of your patch right now is making it less friendly for > debugging. I don't care, and think if we want that we'd be better of > scraping the trace ouput for such common cases and/or use "verbose grep > [...]" and teach the "verbose" wrapper about these common cases, but > knowing of that objection + having other outstanding things has been why > I haven't sent s/test_i18ngrep/grep/g patches. I was the one who introduced "verbose" long ago, and I did have dreams that people would do "verbose grep" everywhere. But in the end, we made "-x" a lot nicer to use, and I think that largely obsoletes it. Using "verbose" makes the non-x verbose output slightly nicer, perhaps. But not having to remember to sprinkle "verbose" through the test code (nor read it) seems like a bigger win to me. For the same reason, I'm pretty ambivalent about having test_grep. I suppose one could make a similar argument about "test_path_is_missing", etc, though some of those helpers are also encapsulating technique (e.g., test_dir_is_empty). -Peff
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh index f70368bc2e..fef6561d80 100755 --- a/t/t7415-submodule-names.sh +++ b/t/t7415-submodule-names.sh @@ -151,10 +151,9 @@ test_expect_success 'fsck detects symlinked .gitmodules file' ' } | git mktree && # Check not only that we fail, but that it is due to the - # symlink detector; this grep string comes from the config - # variable name and will not be translated. + # symlink detector test_must_fail git fsck 2>output && - test_i18ngrep gitmodulesSymlink output + grep gitmodulesSymlink output ) '
Since GETTEXT_POISON does not exist anymore, there is no point warning people about whether we should use test_i18ngrep. This is doubly confusing because the comment was describing why it was OK to use grep, but it got caught up in the mass conversion of 674ba34038 (fsck: mark strings for translation, 2018-11-10). Note there are other uses of test_i18ngrep in this script which are now obsolete; I'll save those for a mass-cleanup. My goal here was just to fix the confusing comment in code I'm about to refactor. Signed-off-by: Jeff King <peff@peff.net> --- t/t7415-submodule-names.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)