Re: [PATCH v3 3/3] ci: stop linking built-ins to the dashed versions

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2020, 17:18 UTC
Message-ID: <xmqq8sdkyypj.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2009081332020.54@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 16 quoted lines

>> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> > index 6c27b886b8..1df9402c3b 100755
>> > --- a/ci/run-build-and-tests.sh
>> > +++ b/ci/run-build-and-tests.sh
>> > @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
>> >  *) ln -s "$cache_dir/.prove" t/.prove;;
>> >  esac
>> >
>> > -make
>> > +make SKIP_DASHED_BUILT_INS=YesPlease
>>
>> Please make sure that this Makefile knob is set in all jobs building
>> and testing Git, or justify in the commit message why it isn't.
>
> The intention was to set it in all jobs (but the jury, AKA Junio, is still
> out on that). Did I not do that?

I already said I understood and agreed with your reasoning why this should be done everywhere, and as far as I can see, the "make" invocation we see above is before the job specific case statement starts doing things differently, and applies to everybody.

If I were "still out on" anything, it is that the proposed log message of 3/3 does not explain well why this has a (good) effect on the running of tests, and caused both Szeder and I confusion. The log message needs a bit more polishing.

I think the primary cause of the confusion is that it is not clear to readers that the early three paragraphs refer to the building (hardlinking) of git-foo in the source tree. Because the primary goal of the Makefile change in 2/3 is to stop hardlinking git-foo in the installed location, it is easy for readers to mistakenly think that these paragraphs still talk about the git-foo binaries in the installed directory and miss the fact that we also make them in the source directory without SKIP_DASHED_BUILT_INS.

Thanks.