| Message ID | pull.1288.git.1657789234416.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7253f7ca9fdc5a19f2a9ee5867a20182e10551d6 |
| Headers | show |
| Series | tests: fix incorrect --write-junit-xml code | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > Unfortunately, I noticed this regression no earlier than when I needed > to validate Git for Windows v2.37.1. Since v2.37.1 was an embargoed > release, I could not use GitHub Actions for the CI testing, so I had to > reinstate Git's Azure Pipeline. I wonder if it would make your life easier if the same GitHub Actions CI stuff were available for the Cabal repository we use for embargoed work, by allowing you to use the same validation for usual releases and the enbargoed ones? "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > t/test-lib-junit.sh | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh > index c959183c7e2..79c31c788b9 100644 > --- a/t/test-lib-junit.sh > +++ b/t/test-lib-junit.sh > @@ -46,7 +46,7 @@ finalize_test_case_output () { > shift > case "$test_case_result" in > ok) > - set "$*" > + set -- "$*" > ;; > failure) > junit_insert="<failure message=\"not ok $test_count -" > @@ -65,17 +65,17 @@ finalize_test_case_output () { > junit_insert="$junit_insert<system-err>$(xml_attr_encode \ > "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>" > fi > - set "$1" " $junit_insert" > + set -- "$1" " $junit_insert" > ;; > fixed) > - set "$* (breakage fixed)" > + set -- "$* (breakage fixed)" > ;; > broken) > - set "$* (known breakage)" > + set -- "$* (known breakage)" > ;; > skip) > message="$(xml_attr_encode --no-lf "$skipped_reason")" > - set "$1" " <skipped message=\"$message\" />" > + set -- "$1" " <skipped message=\"$message\" />" > ;; > esac OK. Ancient shells did not understand "--" and it was idiomatic to say "set x ...; shift", but we already do assume "set --" is usable everywhere we care about in many of our scripts and tests. Looks good to me. Thanks. Will queue.
On Thu, Jul 14 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> Unfortunately, I noticed this regression no earlier than when I needed >> to validate Git for Windows v2.37.1. Since v2.37.1 was an embargoed >> release, I could not use GitHub Actions for the CI testing, so I had to >> reinstate Git's Azure Pipeline. > > I wonder if it would make your life easier if the same GitHub > Actions CI stuff were available for the Cabal repository we use for > embargoed work, by allowing you to use the same validation for usual > releases and the enbargoed ones? Isn't it. Looking at e.g. https://github.com/git/cabal/actions/runs/2284056293 (private link) that's the CI run for the latest push to "master" (same as https://github.com/git/git/commit/e8005e4871f130c4e402ddca2032c111252f070a). So I didn't get this idea that we must have Azure to resurrect for "private CI", don't we have that already, and anyone can use it by re-pushing git.git to a private repo (the limitation being that on GitHub you can't make a private fork of a public repo)?
Hi Junio, On Thu, 14 Jul 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > Unfortunately, I noticed this regression no earlier than when I needed > > to validate Git for Windows v2.37.1. Since v2.37.1 was an embargoed > > release, I could not use GitHub Actions for the CI testing, so I had to > > reinstate Git's Azure Pipeline. > > I wonder if it would make your life easier It would make my life easier if Chesterton's Fences were left alone ;-) > if the same GitHub Actions CI stuff were available for the Cabal > repository we use for embargoed work, by allowing you to use the same > validation for usual releases and the enbargoed ones? GitHub Actions are available in the Cabal repository. The problem is that due to the private nature of said repository, the number of included build minutes is limited. I did manage to get GitHub to sponsor the Git project specifically to increase said build minutes. But that only scratches the problem at the surface. I still think that we need to slow the heck down with refactoring for refactoring's sake because it's not only the CI builds that are affected. I pay a lot of time to accommodate for those refactorings, and so do others, and the benefit of most of those refactorings escapes me. My best guess is that they adapt Git's source code to individual tastes, which is of course a losing game because too many individuals with too many different tastes are involved in the Git project. Unless we start valuing particular individuals' tastes over others', which I believe we should not do. You asked me in private to provide more reviews for those refactorings so that they see some push-back, but I lack the bandwidth for that. > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > t/test-lib-junit.sh | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh > > index c959183c7e2..79c31c788b9 100644 > > --- a/t/test-lib-junit.sh > > +++ b/t/test-lib-junit.sh > > @@ -46,7 +46,7 @@ finalize_test_case_output () { > > shift > > case "$test_case_result" in > > ok) > > - set "$*" > > + set -- "$*" > > ;; > > failure) > > junit_insert="<failure message=\"not ok $test_count -" > > @@ -65,17 +65,17 @@ finalize_test_case_output () { > > junit_insert="$junit_insert<system-err>$(xml_attr_encode \ > > "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>" > > fi > > - set "$1" " $junit_insert" > > + set -- "$1" " $junit_insert" > > ;; > > fixed) > > - set "$* (breakage fixed)" > > + set -- "$* (breakage fixed)" > > ;; > > broken) > > - set "$* (known breakage)" > > + set -- "$* (known breakage)" > > ;; > > skip) > > message="$(xml_attr_encode --no-lf "$skipped_reason")" > > - set "$1" " <skipped message=\"$message\" />" > > + set -- "$1" " <skipped message=\"$message\" />" > > ;; > > esac > > OK. Ancient shells did not understand "--" and it was idiomatic to > say "set x ...; shift", but we already do assume "set --" is usable > everywhere we care about in many of our scripts and tests. > > Looks good to me. > > Thanks. Will queue. Thank you, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > I still think that we need to slow the heck down with refactoring for > refactoring's sake because it's not only the CI builds that are affected. > I pay a lot of time to accommodate for those refactorings, and so do > others, and the benefit of most of those refactorings escapes me. Absolutely. > You asked me in private to provide more reviews for those refactorings so > that they see some push-back, but I lack the bandwidth for that. I do remember telling you to push back what you do not want to see in 'seen' and advance to 'next'. If everybody lacks the bandwidth for shooting down bad ideas and only has time to promote their own ideas, which are not guaranteed to be good ones, it does not lead to a good place X-<.
Hi Junio, On Mon, 8 Aug 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > You asked me in private to provide more reviews for those refactorings so > > that they see some push-back, but I lack the bandwidth for that. > > I do remember telling you to push back what you do not want to see > in 'seen' and advance to 'next'. If everybody lacks the bandwidth > for shooting down bad ideas and only has time to promote their own > ideas, which are not guaranteed to be good ones, it does not lead > to a good place X-<. The funny thing is that you're usually simply not picking up patches that do not get any reviews, but for these refactorings it is somehow different, and I do not understand why it needs to be different. I do not _want_ to spend time reviewing patch series of dubious benefit. And that would hold true even if I could justify spending that time, which I can't. There are so many more contributions that promise a much higher return of investment for reviewing them. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > The funny thing is that you're usually simply not picking up patches that > do not get any reviews, but for these refactorings it is somehow > different, and I do not understand why it needs to be different. Well, I am not sure if encouraging the maintainer to drop patches that are not reviewed is a good overall direction you would want to go in. But let's try that. Less work for me, less disruption to our tree, and the world may be quieter and more pleasant ;-)
Hi Junio, On Wed, 10 Aug 2022, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > The funny thing is that you're usually simply not picking up patches that > > do not get any reviews, but for these refactorings it is somehow > > different, and I do not understand why it needs to be different. > > Well, I am not sure if encouraging the maintainer to drop patches > that are not reviewed is a good overall direction you would want to > go in. I am not encouraging you to drop high-quality patches that have a clear benefit to the project and/or Git's users. You probably know me better than to assume that I would ever do such a thing. You probably meant this comment tongue-in-cheek, but I cannot really tell. It's about these long patch series that touch everything and their dog and the end result brings no clear benefit to users nor the Git project itself. I can easily imagine how wonderful it would be not to see such patch series ever again. ;-) > But let's try that. Less work for me, less disruption to our tree, > and the world may be quieter and more pleasant ;-) Amen! Ciao, Dscho
diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh index c959183c7e2..79c31c788b9 100644 --- a/t/test-lib-junit.sh +++ b/t/test-lib-junit.sh @@ -46,7 +46,7 @@ finalize_test_case_output () { shift case "$test_case_result" in ok) - set "$*" + set -- "$*" ;; failure) junit_insert="<failure message=\"not ok $test_count -" @@ -65,17 +65,17 @@ finalize_test_case_output () { junit_insert="$junit_insert<system-err>$(xml_attr_encode \ "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")</system-err>" fi - set "$1" " $junit_insert" + set -- "$1" " $junit_insert" ;; fixed) - set "$* (breakage fixed)" + set -- "$* (breakage fixed)" ;; broken) - set "$* (known breakage)" + set -- "$* (known breakage)" ;; skip) message="$(xml_attr_encode --no-lf "$skipped_reason")" - set "$1" " <skipped message=\"$message\" />" + set -- "$1" " <skipped message=\"$message\" />" ;; esac