Re: [PATCH v9 00/17] Upstreaming the Scalar command

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Nov 30 2021, Johannes Schindelin via GitGitGadget wrote:

> [...]
>  * The rebase on top of v2.34.0, which changed the default merge strategy to
>    ORT, should have changed the default for merge.renames to true. This is
>    now the case.
>  * Accommodate preemptively for ab/ci-updates which invalidates assumptions
>    made by this patch series that would still hold true with v2.34.0 but are
>    no longer valid in seen and would trigger CI build breakages.
> [...]
>   1:  3a2e28275f1 =  1:  3a2e28275f1 scalar: add a README with a roadmap
>   2:  50160d61a41 =  2:  50160d61a41 scalar: create a rudimentary executable
>   3:  74cd6410931 =  3:  74cd6410931 scalar: start documenting the command
>   4:  37231a4dd07 =  4:  37231a4dd07 scalar: create test infrastructure
>   5:  a39b9c81214 =  5:  a39b9c81214 cmake: optionally build `scalar`, too
>   6:  8e3542e43f7 !  6:  8c6762def30 ci: also run the `scalar` tests
>      @@ .github/workflows/main.yml: jobs:
>            - name: upload tracked files and build artifacts
>       
>        ## ci/run-build-and-tests.sh ##
>      -@@ ci/run-build-and-tests.sh: linux-gcc-4.8|pedantic)
>      - 	make test
>      - 	;;
>      - esac
>      -+make -C contrib/scalar test
>      +@@ ci/run-build-and-tests.sh: esac
>        
>        check_unignored_build_artifacts
>        
>      ++make && make -C contrib/scalar test
>      ++
>      + save_good_tree

This gets rid of the hard CI failure we saw in "seen" but still carries
forward the logic error noted in [1] and of the "pedantic"
compilation-only job now running tests, which isn't what that job is
supposed to be doing.

See the "Don't run the tests" comment in cebead1ebfb (ci: run a pedantic
build as part of the GitHub workflow, 2021-08-08). As noted before you
can see that at the tail-end of your own CI output[2] on top of
"master".

There's other seemingly unintended interactions with the ci/ code on
"master" here (which I also also noted in previous threads). FWIW the
naïve merge with ab/ci-updates happens to fix at least one of
those. I.e. the issue here of "linux-gcc" and "linux-clang" only running
the scalar tests in one half of their test modes, but running everything
else in both.

I think it's clear from past exchanges that you vehemently disagree with
my proposed direction for solving these and other issues in one fell
swoop[3], which is fair enough.

But I really don't think it's OK to continue to ignore reports from me
of specific issues in this series just because you don't like that
larger set of fixes [3].

I honestly don't care if you'd pick that up as-is, just as long as
outstanding issues it addressed are either fixed, or commit
messages/docs are updated to note that the bugs/behavior changes are
intentional.

In this case I think it would be fine to keep the patch as-is and have
the commit message argue for why the scalar tests should be a special
snowflake in being the only tests that are run in "pedantic".

Or to just fix the seemingly unintentional behavior change in some
smaller way. I think the "added thusly" comment I hade in [4] should be
the easiest way to do that (well, [3] is easier, but let's leave that
aside...).

1. https://lore.kernel.org/git/211122.86ee78yxts.gmgdl@evledraar.gmail.com/
2. https://github.com/gitgitgadget/git/runs/4292915519?check_suite_focus=true
3. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/
4. https://lore.kernel.org/git/211123.86ilwjujmd.gmgdl@evledraar.gmail.com/